Bug 172401 – Many editing js-tests use waitUntilDone

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

js-tests are incompatible with testRunner.waitUntilDone(), yet there are hundreds of tests that mix these techniques.

The common symptom is that TEST COMPLETE is printed before the test is complete. But there may be more, including flakiness. Some of these tests are malformed in other ways, such as using js-test-pre.js without js-test-post.js.

[Alexey Proskuryakov]

Created attachment 310724 [details]
proposed patch

[Build Bot]

Comment on attachment 310724 [details]
proposed patch

Attachment 310724 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3779992

New failing tests:
editing/spelling/copy-paste-crash.html

[Build Bot]

Created attachment 310739 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 310724 [details]
proposed patch

Attachment 310724 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3780554

New failing tests:
editing/spelling/copy-paste-crash.html

[Build Bot]

Created attachment 310751 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Alexey Proskuryakov]

Created attachment 310927 [details]
proposed patch

[Chris Dumez]

Comment on attachment 310927 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=310927&action=review

> LayoutTests/ChangeLog:9
> +        of js-test-pre.js where possible.

Could you clarify what where possible means? I personally thought we had to use js-test-pre.js / js-test-post.js for all async tests but the first test below is async and you updated it to use js.test.js so I must be misinformed.

[Ryosuke Niwa]

Comment on attachment 310927 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=310927&action=review

> LayoutTests/editing/pasteboard/drag-link-with-data-transfer-adds-trusted-link-to-pasteboard.html:60
> +<div id="console"</div>

Nit: close div here?

[Alexey Proskuryakov]

Created attachment 310937 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 310937 [details]
patch for landing

Rejecting attachment 310937 [details] from commit-queue.

New failing tests:
editing/pasteboard/drag-link-with-data-transfer-adds-trusted-link-to-pasteboard.html
Full output: http://webkit-queues.webkit.org/results/3796593

[WebKit Commit Bot]

Created attachment 310949 [details]
Archive of layout-test-results from webkit-cq-03 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: webkit-cq-03  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Alexey Proskuryakov]

Created attachment 310965 [details]
patch for landing

Not sure what happened here, let's try again without the spurious newlines.

[Alexey Proskuryakov]

> Could you clarify what where possible means? I personally thought we had to use js-test-pre.js / js-test-post.js for all async tests but the first test below is async and you updated it to use js.test.js so I must be misinformed.

js-test.js makes tests substantially simpler (switching to it can remove up to 10 lines of code in certain cases, and lots of weirdness), so it's preferable.

I know of two cases where it shouldn't be used:

1. Tests with <video> elements, because the load event doesn't fire before DRT/WKTR consider the test complete.

2. Async tests that explicitly test the load event behavior - js-test.js adds a load event handler of its own.

[WebKit Commit Bot]

Comment on attachment 310965 [details]
patch for landing

Clearing flags on attachment: 310965

Committed r217292: <http://trac.webkit.org/changeset/217292>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.