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.
<rdar://problem/28173157>
Are we planning on using awaitEvent to replace all instances of singleFireEventListener in synchronous tests?
(In reply to comment #2) > Are we planning on using awaitEvent to replace all instances of > singleFireEventListener in synchronous tests? Sorry, I mean asynchronous.
(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 =)
(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.
Created attachment 288050 [details] WIP
Created attachment 288099 [details] Patch
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
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
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
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
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
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
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.
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.
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.
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
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
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
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
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
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
Created attachment 288243 [details] Patch
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
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
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
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
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.
(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 :/
Created attachment 288458 [details] Patch
Created attachment 288493 [details] Patch
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
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
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
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
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.