Bug 172401 - Many editing js-tests use waitUntilDone
Summary: Many editing js-tests use waitUntilDone
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: Safari 9
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords:
Depends on:
Blocks: 168961
  Show dependency treegraph
 
Reported: 2017-05-19 16:42 PDT by Alexey Proskuryakov
Modified: 2017-05-23 13:23 PDT (History)
5 users (show)

See Also:


Attachments
proposed patch (25.03 KB, patch)
2017-05-19 16:43 PDT, Alexey Proskuryakov
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-elcapitan (2.16 MB, application/zip)
2017-05-19 18:00 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews100 for mac-elcapitan (1.14 MB, application/zip)
2017-05-19 19:49 PDT, Build Bot
no flags Details
proposed patch (26.82 KB, patch)
2017-05-22 14:59 PDT, Alexey Proskuryakov
rniwa: review+
Details | Formatted Diff | Diff
patch for landing (26.82 KB, patch)
2017-05-22 15:27 PDT, Alexey Proskuryakov
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-cq-03 for mac-elcapitan (1.52 MB, application/zip)
2017-05-22 16:12 PDT, WebKit Commit Bot
no flags Details
patch for landing (26.71 KB, patch)
2017-05-22 17:58 PDT, Alexey Proskuryakov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2017-05-19 16:42:12 PDT
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.
Comment 1 Alexey Proskuryakov 2017-05-19 16:43:44 PDT
Created attachment 310724 [details]
proposed patch
Comment 2 Build Bot 2017-05-19 18:00:06 PDT
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
Comment 3 Build Bot 2017-05-19 18:00:07 PDT
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
Comment 4 Build Bot 2017-05-19 19:49:34 PDT
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
Comment 5 Build Bot 2017-05-19 19:49:35 PDT
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
Comment 6 Alexey Proskuryakov 2017-05-22 14:59:57 PDT
Created attachment 310927 [details]
proposed patch
Comment 7 Chris Dumez 2017-05-22 15:02:25 PDT
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.
Comment 8 Ryosuke Niwa 2017-05-22 15:03:49 PDT
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?
Comment 9 Alexey Proskuryakov 2017-05-22 15:27:47 PDT
Created attachment 310937 [details]
patch for landing
Comment 10 WebKit Commit Bot 2017-05-22 16:12:19 PDT
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
Comment 11 WebKit Commit Bot 2017-05-22 16:12:20 PDT
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
Comment 12 Alexey Proskuryakov 2017-05-22 17:58:05 PDT
Created attachment 310965 [details]
patch for landing

Not sure what happened here, let's try again without the spurious newlines.
Comment 13 Alexey Proskuryakov 2017-05-23 12:54:13 PDT
> 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.
Comment 14 WebKit Commit Bot 2017-05-23 13:23:27 PDT
Comment on attachment 310965 [details]
patch for landing

Clearing flags on attachment: 310965

Committed r217292: <http://trac.webkit.org/changeset/217292>
Comment 15 WebKit Commit Bot 2017-05-23 13:23:29 PDT
All reviewed patches have been landed.  Closing bug.