Bug 128736 - inspector-protocol/debugger/setBreakpoint-dfg.html is flaky
Summary: inspector-protocol/debugger/setBreakpoint-dfg.html is flaky
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yury Semikhatsky
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-02-13 04:08 PST by Michal Pakula vel Rutka
Modified: 2019-10-30 21:55 PDT (History)
6 users (show)

See Also:


Attachments
Patch (17.15 KB, patch)
2019-10-10 10:30 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (18.58 KB, patch)
2019-10-10 11:56 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (4.55 KB, patch)
2019-10-25 23:06 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michal Pakula vel Rutka 2014-02-13 04:08:33 PST
Layout test inspector-protocol/debugger/setBreakpoint-dfg.html added in r162940 <http://trac.webkit.org/changeset/162940> is flaky on EFL bot:

http://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=inspector-protocol%2Fdebugger%2FsetBreakpoint-dfg.html

Diff:
--- /mnt/buildbot/WebKit-BuildSlave/efl-linux-64-release-wk2/build/layout-test-results/inspector-protocol/debugger/setBreakpoint-dfg-expected.txt
+++ /mnt/buildbot/WebKit-BuildSlave/efl-linux-64-release-wk2/build/layout-test-results/inspector-protocol/debugger/setBreakpoint-dfg-actual.txt
@@ -13,6 +13,6 @@
 dfgWithoutInline result: 502500
 Hit Breakpoint 2!
 Removed Breakpoint 2!
+dfgWithInline result: 504500
 PASS
-dfgWithInline result: 504500
Comment 1 Alexey Proskuryakov 2014-07-17 10:18:26 PDT
This happens on Mac too, with the same diff. Will move the expectation to cross-platform file.
Comment 2 Yury Semikhatsky 2019-10-10 10:30:56 PDT
Created attachment 380656 [details]
Patch
Comment 3 Yury Semikhatsky 2019-10-10 11:56:25 PDT
Created attachment 380668 [details]
Patch
Comment 4 Yury Semikhatsky 2019-10-23 11:01:12 PDT
Comment on attachment 380668 [details]
Patch

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

> LayoutTests/inspector/debugger/setBreakpoint-dfg.html:75
> +                        ProtocolTest.log("PASS");

Alternatively we can drop this line from the output altogether.
Comment 5 Devin Rousso 2019-10-25 15:49:46 PDT
Comment on attachment 380668 [details]
Patch

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

> LayoutTests/ChangeLog:8
> +        Reenabling the test on all platforms. Depending on theplatform implementation of EventLoop

Typo: "theplatform"

> LayoutTests/ChangeLog:11
> +        if InspectorFrontendAPI.dispatchMessageAsync actually delivered messages asynchronosly.

Typo: "asynchronosly"

> LayoutTests/ChangeLog:12
> +        With the current implementation though response to Debugger.resume

Why not change `InspectorProtocol.dispatchMessageFromBackend` then?  As you suggest, I'd expect that function to do some sort of batching with a `setTimeout`.

> LayoutTests/ChangeLog:26
> +        * platform/gtk/inspector-protocol/debugger/setBreakpoint-dfg-and-modify-local-expected.txt: Removed.
> +        * platform/gtk/inspector-protocol/debugger/setBreakpoint-dfg-expected.txt: Removed.
> +        * platform/gtk/inspector-protocol/dom/getAccessibilityPropertiesForNode-expected.txt: Removed.
> +        Removed platform/gtk/inspector-protocol altogether as there is no LayoutTests/inspector-protocol
> +        folder any more. Corresponding tests were either deleted or moved to LayoutTests/inspector a while ago.

This should be done in a separate commit.  I think it's fine to land this part of the change unreviewed, but it should still be separate.

> LayoutTests/inspector/debugger/setBreakpoint-dfg.html:74
> +                    setTimeout(() => {

What about adding an event handler at the top level?
```
    InspectorProtocol.eventHandler["Debugger.resumed"] = function(messageObject) {
        if (breakpointsHit < 2)
            return;

        ProtocolTest.assert(breakpointsHit === 2);
        ProtocolTest.log("PASS");
        ProtocolTest.completeTest();
    };
```

You could then simplify this to just be `InspectorProtocol.sendCommand("Debugger.resume");`.
Comment 6 Yury Semikhatsky 2019-10-25 23:06:19 PDT
Comment on attachment 380668 [details]
Patch

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

>> LayoutTests/ChangeLog:8
>> +        Reenabling the test on all platforms. Depending on theplatform implementation of EventLoop
> 
> Typo: "theplatform"

Done.

>> LayoutTests/ChangeLog:11
>> +        if InspectorFrontendAPI.dispatchMessageAsync actually delivered messages asynchronosly.
> 
> Typo: "asynchronosly"

Done.

>> LayoutTests/ChangeLog:12
>> +        With the current implementation though response to Debugger.resume
> 
> Why not change `InspectorProtocol.dispatchMessageFromBackend` then?  As you suggest, I'd expect that function to do some sort of batching with a `setTimeout`.

This would be a bigger effort which I don't have time for atm. I'm not sure that it's justified given that there is a desire to get rid of the custom frontend page for protocol tests.

>> LayoutTests/ChangeLog:26
>> +        folder any more. Corresponding tests were either deleted or moved to LayoutTests/inspector a while ago.
> 
> This should be done in a separate commit.  I think it's fine to land this part of the change unreviewed, but it should still be separate.

Extracted to https://bugs.webkit.org/show_bug.cgi?id=203453

>> LayoutTests/inspector/debugger/setBreakpoint-dfg.html:74
>> +                    setTimeout(() => {
> 
> What about adding an event handler at the top level?
> ```
>     InspectorProtocol.eventHandler["Debugger.resumed"] = function(messageObject) {
>         if (breakpointsHit < 2)
>             return;
> 
>         ProtocolTest.assert(breakpointsHit === 2);
>         ProtocolTest.log("PASS");
>         ProtocolTest.completeTest();
>     };
> ```
> 
> You could then simplify this to just be `InspectorProtocol.sendCommand("Debugger.resume");`.

Done.
Comment 7 Yury Semikhatsky 2019-10-25 23:06:40 PDT
Created attachment 382001 [details]
Patch
Comment 8 Devin Rousso 2019-10-30 21:09:30 PDT
Comment on attachment 382001 [details]
Patch

rs=me
Comment 9 WebKit Commit Bot 2019-10-30 21:54:54 PDT
Comment on attachment 382001 [details]
Patch

Clearing flags on attachment: 382001

Committed r251833: <https://trac.webkit.org/changeset/251833>
Comment 10 WebKit Commit Bot 2019-10-30 21:54:55 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2019-10-30 21:55:14 PDT
<rdar://problem/56770820>