Bug 161634 – Web Inspector: [META] make Object.awaitEvent synchronously add an event listener, and adopt it in tests

• Status: ASSIGNED
• Component: Web Inspector

[Blaze Burg]

Currently, Object.awaitEvent adds a listener inside the promise constructor function argument, which is evaluated on the next tick. This means that if two awaitEvents are chained together, and the event we expect is dispatched synchronously, then the second event will likely be dispatched before the Promise is evaluated and adds its singleFireEventListener.

[Radar WebKit Bug Importer]

<rdar://problem/28173157>

[Devin Rousso]

Are we planning on using awaitEvent to replace all instances of singleFireEventListener in synchronous tests?

[Devin Rousso]

(In reply to comment #2)
> Are we planning on using awaitEvent to replace all instances of
> singleFireEventListener in synchronous tests?

Sorry, I mean asynchronous.

[Blaze Burg]

(In reply to comment #2)
> Are we planning on using awaitEvent to replace all instances of
> singleFireEventListener in synchronous tests?

I prefer it since UI tests in particular are going to use promises heavily, and singleFireEventListener is more callback-oriented. For existing tests that use AsyncTestSuite, it makes sense to covert them over. I have a patch in progress, so no need to go do it =)

[Devin Rousso]

(In reply to comment #4)
> I prefer it since UI tests in particular are going to use promises heavily,
> and singleFireEventListener is more callback-oriented. For existing tests
> that use AsyncTestSuite, it makes sense to covert them over. I have a patch
> in progress, so no need to go do it =)

Oh lol I do too :P  I was asking because there are some tests where the singleFireEventListener is saved to a variable.

[Blaze Burg]

Created attachment 288050 [details]
WIP

[Devin Rousso]

Created attachment 288099 [details]
Patch

[Build Bot]

Comment on attachment 288099 [details]
Patch

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

New failing tests:
http/tests/inspector/network/resource-timing.html
inspector/debugger/break-on-exception-throw-in-promise.html
inspector/debugger/csp-exceptions.html
inspector/console/command-line-api.html
inspector/console/messageAdded-from-named-evaluations.html
inspector/debugger/break-on-uncaught-exception.html
inspector/network/client-blocked-load.html

[Build Bot]

Created attachment 288109 [details]
Archive of layout-test-results from ews102 for mac-yosemite

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

[Build Bot]

Comment on attachment 288099 [details]
Patch

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

New failing tests:
inspector/debugger/break-on-uncaught-exception.html
http/tests/inspector/network/resource-timing.html
inspector/console/command-line-api.html
inspector/debugger/csp-exceptions.html
inspector/console/messageAdded-from-named-evaluations.html

[Build Bot]

Created attachment 288110 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 288099 [details]
Patch

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

New failing tests:
inspector/debugger/break-on-uncaught-exception.html
inspector/debugger/break-on-exception-throw-in-promise.html
inspector/debugger/csp-exceptions.html
inspector/console/command-line-api.html
inspector/model/frame-extra-scripts.html
inspector/console/messageAdded-from-named-evaluations.html

[Build Bot]

Created attachment 288111 [details]
Archive of layout-test-results from ews113 for mac-yosemite

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

[Blaze Burg]

Comment on attachment 288099 [details]
Patch

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

Needs some fixes to get EWS working. Make sure you run tests with --force to make sure some of the flaky ones don't have trivial regressions.

> LayoutTests/http/tests/inspector/network/copy-as-curl.html:99
> +            InspectorTest.evaluateInPage("createGETRequestWithSpecialURL()");

This definitely flows better with the trigger at the end.

> LayoutTests/http/tests/inspector/network/resource-timing.html:-27
> -                resource.singleFireEventListener(WebInspector.Resource.Event.ResponseReceived, (event) => {

This test is failing. My guess is that we need to awaitEvent for ResponseReceived and LoadingDidFinish before either fires. The LoadingDidFinish could fire on the event loop turn right after awaitEvent is fulfilled by dispatching ResponseReceived, but its reaction (the .then) won't run until the next loop, and thus miss the event.

Since the events are guaranteed to be in a particular order, we can fork the chain here once `resource` is available and return Promise.all(awaitEvent..., awaitEvent...). I guess we could check all the timing data after both events come through, but I think this test specifically checks that partial data is available.

> LayoutTests/inspector/console/clearMessages.html:20
> +            .then(resolve, reject);

This should be .then(reject, reject) as any control flow through the chain is unexpected.

> LayoutTests/inspector/console/command-line-api.html:56
> +    function addAPITestCase({title, input, setup}) {

hehe

> LayoutTests/inspector/console/command-line-api.html:88
> +        })

This test is failing, I think you changed the semantics here. It previously resolved when the listener was added, not when the event was fired.

> LayoutTests/inspector/console/messageAdded-from-named-evaluations.html:47
> +

This test started failing. The only obvious thing to me is removal of setTimeout, but I'm not sure why it was there to begin with.

> LayoutTests/inspector/css/generate-css-rule-string.html:57
>              } else {

You might as well flip this conditional to have an early exit in the error case so we can cut a level of indentation.

> LayoutTests/inspector/css/manager-preferredInspectorStyleSheetForFrame.html:27
> +            .then((event) => {

I don't understand the control flow of this test. What triggers the event?

EDIT: I see, it's indirectly triggered when the stylesheet is added, but before the callback is called by preferredInspectorStyleSheetForFrame. This seems really fragile, but we can fix it later.

This change seems to break this test, as nothing triggers the initial event.

> LayoutTests/inspector/css/manager-preferredInspectorStyleSheetForFrame.html:70
> +                    InspectorTest.expectThat(WebInspector.cssStyleManager.styleSheets.length === 2, "Should be two stylesheets.");

Same comments as above.

> LayoutTests/inspector/debugger/break-on-uncaught-exception-throw-in-promise.html:31
> +                InspectorTest.awaitEvent("AfterTestFunction").then((event) => {

Could add newline.

> LayoutTests/inspector/debugger/break-on-uncaught-exception.html:-56
> -            InspectorTest.evaluateInPage(`setTimeout(() => {

This test started timing out. I think removing the setTimeout broke it, but I'm not exactly sure why. Our tests shouldn't be so brittle...

> LayoutTests/inspector/debugger/break-on-uncaught-exception.html:65
> +            InspectorTest.awaitEvent("AfterTestFunction").then((event) => {

Newline.

> LayoutTests/inspector/debugger/break-on-uncaught-exception.html:71
> +            InspectorTest.evaluateInPage(`(() => {

I don't understand why this needs wrapped in an anonymous scope.

> LayoutTests/inspector/debugger/csp-exceptions.html:39
> +            InspectorTest.evaluateInPage("triggerCSPExceptionInsideScript()");

This test started failing. Maybe it's setTimeout?

> LayoutTests/inspector/model/scope-chain-node.html:71
> +                let promise = WebInspector.debuggerManager.awaitEvent(WebInspector.DebuggerManager.Event.Resumed);

Clever.

> LayoutTests/inspector/model/script-resource-relationship.html:50
> +            let script = null;

This test became flaky, for reasons unknown. You should check the output.

> LayoutTests/inspector/network/client-blocked-load.html:27
> +                return resource.awaitEvent(WebInspector.Resource.Event.LoadingDidFail);

This test became flaky. Like in another test, we may need to set up both awaitEvents before either comes and use Promise.all(). if ResourceWasAdded and LoadingDidFail were fired in the same event loop turn, the latter will be missed because the first waitEvent's reaction happens on a different event loop turn.

[Blaze Burg]

BTW, you should probably split this patch into smaller patches, otherwise we will have a really hard time landing it and addressing new flakes. It will also help for reviewing. I suggest iterating with EWS until the patch goes through green, then splitting into a patch per major directory. You can use this same bug for all of the sub patches.

[Devin Rousso]

Created attachment 288227 [details]
Patch

I think I fixed most of the tests.  When I run it locally, everything passes ¯\_(ツ)_/¯

(In reply to comment #15)
> BTW, you should probably split this patch into smaller patches, otherwise we
> will have a really hard time landing it and addressing new flakes. It will
> also help for reviewing. I suggest iterating with EWS until the patch goes
> through green, then splitting into a patch per major directory. You can use
> this same bug for all of the sub patches.

I'll do this per LayoutTests/inspector/* subdirectory once I get the changes to awaitEvent merged.

[Build Bot]

Comment on attachment 288227 [details]
Patch

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

New failing tests:
http/tests/inspector/network/resource-timing.html

[Build Bot]

Created attachment 288233 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 288227 [details]
Patch

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

New failing tests:
inspector/debugger/break-on-exception-throw-in-promise.html
http/tests/inspector/network/resource-timing.html

[Build Bot]

Created attachment 288237 [details]
Archive of layout-test-results from ews102 for mac-yosemite

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

[Build Bot]

Comment on attachment 288227 [details]
Patch

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

New failing tests:
inspector/model/frame-extra-scripts.html
inspector/debugger/break-on-exception-throw-in-promise.html
http/tests/inspector/network/resource-timing.html

[Build Bot]

Created attachment 288239 [details]
Archive of layout-test-results from ews116 for mac-yosemite

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

[Devin Rousso]

Created attachment 288243 [details]
Patch

[Build Bot]

Comment on attachment 288243 [details]
Patch

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

New failing tests:
inspector/debugger/break-on-exception-throw-in-promise.html

[Build Bot]

Created attachment 288248 [details]
Archive of layout-test-results from ews103 for mac-yosemite

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

[Build Bot]

Comment on attachment 288243 [details]
Patch

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

New failing tests:
inspector/model/frame-extra-scripts.html
inspector/debugger/break-on-exception-throw-in-promise.html

[Build Bot]

Created attachment 288250 [details]
Archive of layout-test-results from ews114 for mac-yosemite

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

[Blaze Burg]

Comment on attachment 288243 [details]
Patch

Still some flakiness on the bots. We should figure out the underlying cause so we don't regress a ton of things, and/or codify best practices for writing flake-free tests in our style guide.

[Devin Rousso]

(In reply to comment #28)
> Still some flakiness on the bots. We should figure out the underlying cause
> so we don't regress a ton of things, and/or codify best practices for
> writing flake-free tests in our style guide.

So it looks like there are only two tests left with issues:

 - inspector/model/frame-extra-scripts.html
This seems to be caused by the race between awaitEvent and evaluateInPage.  On my build, it passes with the current expected result (which I modified).  Does it make sense to move the callback for evaluateInPage to be part of awaitEvent?

 - inspector/debugger/break-on-exception-throw-in-promise.html
I'm not really sure why this times out.  It works just fine for me :/

[Devin Rousso]

Created attachment 288458 [details]
Patch

[Devin Rousso]

Created attachment 288493 [details]
Patch

[Build Bot]

Comment on attachment 288493 [details]
Patch

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

New failing tests:
inspector/debugger/break-on-exception-throw-in-promise.html

[Build Bot]

Created attachment 288498 [details]
Archive of layout-test-results from ews101 for mac-yosemite

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

[Build Bot]

Comment on attachment 288493 [details]
Patch

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

New failing tests:
inspector/debugger/break-on-exception-throw-in-promise.html

[Build Bot]

Created attachment 288499 [details]
Archive of layout-test-results from ews117 for mac-yosemite

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

[Blaze Burg]

Comment on attachment 288493 [details]
Patch

I think you should first land the behavior change, then start landing directory at a time. It's fine if we need to defer on converting debugger tests if they are too flaky, but the rest of this needs to get landed before it rots further.