Bug 162072 - Web Inspector: adopt Object.awaitEvent in LayoutTests/inspector/debugger
Summary: Web Inspector: adopt Object.awaitEvent in LayoutTests/inspector/debugger
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks: 161634
  Show dependency treegraph
 
Reported: 2016-09-16 10:50 PDT by Devin Rousso
Modified: 2016-12-13 15:32 PST (History)
4 users (show)

See Also:


Attachments
Patch (24.29 KB, patch)
2016-09-16 16:33 PDT, Devin Rousso
bburg: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-yosemite (1.07 MB, application/zip)
2016-09-16 17:31 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-yosemite (1.64 MB, application/zip)
2016-09-16 17:33 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2016-09-16 10:50:04 PDT
.
Comment 1 Radar WebKit Bug Importer 2016-09-16 10:50:43 PDT
<rdar://problem/28340067>
Comment 2 Devin Rousso 2016-09-16 16:33:06 PDT
Created attachment 289131 [details]
Patch
Comment 3 Build Bot 2016-09-16 17:31:40 PDT
Comment on attachment 289131 [details]
Patch

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

New failing tests:
inspector/debugger/break-on-exception-throw-in-promise.html
Comment 4 Build Bot 2016-09-16 17:31:42 PDT
Created attachment 289150 [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 5 Build Bot 2016-09-16 17:33:04 PDT
Comment on attachment 289131 [details]
Patch

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

New failing tests:
inspector/debugger/break-on-exception-throw-in-promise.html
Comment 6 Build Bot 2016-09-16 17:33:07 PDT
Created attachment 289151 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 7 Brian Burg 2016-09-19 08:28:14 PDT
Comment on attachment 289131 [details]
Patch

Please update test expectations as [Slow] for break-on-exception-throw-in-promise.html and resubmit to EWS. It might just be taking too long on these ports.
Comment 8 Brian Burg 2016-09-19 08:30:02 PDT
Comment on attachment 289131 [details]
Patch

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

> LayoutTests/inspector/debugger/break-on-exception-throw-in-promise.html:23
>                      InspectorTest.expectThat(WebInspector.debuggerManager.pauseReason === "exception", "Should pause for exception.");

You may want to sprinkle the magic to log protocol traffic or add sync logging so we can figure out why this dies.
Comment 9 Joseph Pecoraro 2016-09-19 11:33:26 PDT
Comment on attachment 289131 [details]
Patch

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

> LayoutTests/inspector/debugger/break-on-uncaught-exception-throw-in-promise.html:40
> +                InspectorTest.evaluateInPage(expression);
> +                InspectorTest.evaluateInPage(`setTimeout(() => { TestPage.dispatchEventToFrontend("AfterTestFunction"); }, 50)`);

I find this reordering, in all tests, harder to read the tests. Is it necessary?
Comment 10 Devin Rousso 2016-09-19 16:26:34 PDT
Comment on attachment 289131 [details]
Patch

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

>> LayoutTests/inspector/debugger/break-on-exception-throw-in-promise.html:23
>>                      InspectorTest.expectThat(WebInspector.debuggerManager.pauseReason === "exception", "Should pause for exception.");
> 
> You may want to sprinkle the magic to log protocol traffic or add sync logging so we can figure out why this dies.

While I do love your verbiage, I have no idea what you mean.  Can you explain?  Also, how do I mark a test as "slow"?

>> LayoutTests/inspector/debugger/break-on-uncaught-exception-throw-in-promise.html:40
>> +                InspectorTest.evaluateInPage(`setTimeout(() => { TestPage.dispatchEventToFrontend("AfterTestFunction"); }, 50)`);
> 
> I find this reordering, in all tests, harder to read the tests. Is it necessary?

Really?  I find that having the setup be before the call makes more sense.  In WebInspector code we typically create functions before they are used (not for prototype functions though).
Comment 11 Joseph Pecoraro 2016-09-19 17:32:12 PDT
(In reply to comment #10)
> Comment on attachment 289131 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=289131&action=review
> 
> >> LayoutTests/inspector/debugger/break-on-exception-throw-in-promise.html:23
> >>                      InspectorTest.expectThat(WebInspector.debuggerManager.pauseReason === "exception", "Should pause for exception.");
> > 
> > You may want to sprinkle the magic to log protocol traffic or add sync logging so we can figure out why this dies.
> 
> While I do love your verbiage, I have no idea what you mean.

We recently added:

    InspectorTest.debug()

Or:

    ProtocolTest.debug()

Which is now all you need to do to get the test to dump all the protocol traffic to stderr so it can be debugged when a test fails.


> Also, how do I mark a test as "slow"?

In the LayoutTests/TestExpectations you can add a line that has a grammar like:

    <bugzilla-link>? <release-type>? <test-path> <other-flags>?

In this case some examples of possible lines would be:

    inspector/debugger/break-on-exception-throw-in-promise.html [ Slow ]

    [ Debug ] inspector/debugger/break-on-exception-throw-in-promise.html [ Slow ]

    webkit.org/b/### [ Debug ] inspector/debugger/break-on-exception-throw-in-promise.html [ Slow ]

The later lines are all "way cooler" than the earlier ones.