Opened 4 months ago

Closed 4 months ago

#16861 closed Task (fixed)

[FF] Failing uploadimage test

Reported by: m.lewandowski Owned by: Tade0
Priority: Normal Milestone: CKEditor 4.7.0
Component: General Version:
Keywords: Cc:

Description

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)

Change History (15)

comment:1 Changed 4 months ago by m.lewandowski

  • Status changed from new to confirmed

comment:2 Changed 4 months ago by m.lewandowski

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

comment:3 Changed 4 months ago by m.lewandowski

  • Type changed from Bug to Task

comment:4 Changed 4 months ago by Tade0

  • Owner set to Tade0
  • Status changed from confirmed to assigned

comment:5 Changed 4 months ago by 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.

comment:6 Changed 4 months ago by Tade0

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

comment:7 Changed 4 months ago by Tade0

  • Status changed from assigned to review

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.

comment:8 Changed 4 months ago by t.jakut

  • Status changed from review to review_failed

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.

comment:9 Changed 4 months ago by Tade0

  • Status changed from review_failed to review

Refactored accordingly.

Changes pushed to branch:t/16861b.

comment:10 Changed 4 months ago by t.jakut

  • Status changed from review to review_failed

comment:11 Changed 4 months ago by Tade0

Rebased with major, modified the test.

Changes pushed to branch:t/16861b.

comment:12 Changed 4 months ago by m.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.

comment:13 Changed 4 months ago by t.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.

comment:14 Changed 4 months ago by Tade0

  • Status changed from review_failed to review

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

Changes pushed to branch:t/16861b.

comment:15 Changed 4 months ago by t.jakut

  • Resolution set to fixed
  • Status changed from review to closed

LGTM, merged to major with git:96bbe7e.

Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy