Opened 5 months ago

Closed 5 months ago

#16861 closed Task (fixed)

[FF] Failing uploadimage test

Reported by: Marek 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 5 months ago by Marek Lewandowski

Status: newconfirmed

comment:2 Changed 5 months ago by Marek 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 5 months ago by Marek Lewandowski

Type: BugTask

comment:4 Changed 5 months ago by Tade0

Owner: set to Tade0
Status: confirmedassigned

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

Status: assignedreview

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 5 months ago by Tomasz Jakut

Status: reviewreview_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 5 months ago by Tade0

Status: review_failedreview

Refactored accordingly.

Changes pushed to branch:t/16861b.

comment:10 Changed 5 months ago by Tomasz Jakut

Status: reviewreview_failed

comment:11 Changed 5 months ago by Tade0

Rebased with major, modified the test.

Changes pushed to branch:t/16861b.

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

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

comment:14 Changed 5 months ago by Tade0

Status: review_failedreview

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

Changes pushed to branch:t/16861b.

comment:15 Changed 5 months ago by Tomasz Jakut

Resolution: fixed
Status: reviewclosed

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