[FF] Failing uploadimage test

[Marek Lewandowski]

Test ​test pasting images in the editor with uploadfile and uploadimage plugins fails under certain conditions, with following message:

tests/plugins/uploadfile/uploadfile test pasting images in the editor with uploadfile and uploadimage plugins

Values should be the same.
Expected: <p><img src="/tests/_assets/logo.png" style="height:1px; width:1px" /></p> (string)
Actual:   <p><img src="/tests/_assets/logo.png" style="height:0px; width:0px" /></p> (string)

For me it fails in 2 cases:

• running with devtools open
• cold browser boot, e.g. firefox http://tests.ckeditor.dev:1030/tests/plugins/uploadfile/uploadfile (just make sure no FF processes are active)

[Marek Lewandowski]

​On some other build I saw that tests/plugins/uploadfile/uploadfile has also similar issue. It looks like exact same thing.

[Tade0]

This tends to happen also in the dev version on my machine.

Difference is, on my machine it's enough to increase the delay between adding the image and checking its presence to get rid of this error entirely - this approach does not work in travis.

[Tade0]

Busy waiting for the image to change size doesn't work - perhaps the image never changes size to 1x1?

[Tade0]

I managed to get a 100% green build by waiting for the image load event in non-IE browsers.

My guess is that it should be enough for these tests to always pass.

Changes pushed to ​branch:t/16861b.

[Tomasz Jakut]

The fix itself looks good (I'm only wondering if switching to checking image.readyState in IEs woudln't be a clearer approach), however in the current shape it has a lot of duplication. I propose creating waitForImage function, which would take a callback as a parameter and be inside tests/plugins/uploadfile/_helpers directory.

[Tade0]

Refactored accordingly.

Changes pushed to ​branch:t/16861b.

[Tomasz Jakut]

Still one test tends to fail: ​http://tests.ckeditor.dev:1030/tests/plugins/uploadimage/uploadimage#tests%2Fplugins%2Fuploadimage%2Fuploadimage%20test%20paste%20img%20as%20html%20(integration%20test) – it's not using the new method of waiting for image.

[Tade0]

Rebased with major, modified the test.

Changes pushed to ​branch:t/16861b.

[Marek Lewandowski]

Guys, just one thought here: I see that you're patching the tests, please make sure that the issue is not in the code itself, rather than in tests.

[Tomasz Jakut]

@m.lewandowski the issue originates from the fact that our tests assume that browser loads images with Data URL source synchronously. It's true for Blink and Webkit, but not so true for IE and, apparently, Gecko (​demo). Therefore logic that should be run after image is loaded must be run asynchronously, preferably inside image's load event listener.

@Tade0 all tests seem to be stable now, but I found that there are two tests in uploadimage.js suite that still use old method of waiting for image:

• 'test classic with image1 (integration test)'
• 'test finish upload notification marked as important and is visible (#13032).'

Please check if these are the only ones left.

[Tade0]

Removed all remaining occurrences of wait( ..., delay) statements.

Changes pushed to ​branch:t/16861b.

[Tomasz Jakut]

LGTM, merged to major with ​git:96bbe7e.