Bug 128736 – inspector-protocol/debugger/setBreakpoint-dfg.html is flaky

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Michal Pakula vel Rutka]

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

[Alexey Proskuryakov]

This happens on Mac too, with the same diff. Will move the expectation to cross-platform file.

[Yury Semikhatsky]

Created attachment 380656 [details]
Patch

[Yury Semikhatsky]

Created attachment 380668 [details]
Patch

[Yury Semikhatsky]

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.

[Devin Rousso]

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");`.

[Yury Semikhatsky]

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.

[Yury Semikhatsky]

Created attachment 382001 [details]
Patch

[Devin Rousso]

Comment on attachment 382001 [details]
Patch

rs=me

[WebKit Commit Bot]

Comment on attachment 382001 [details]
Patch

Clearing flags on attachment: 382001

Committed r251833: <https://trac.webkit.org/changeset/251833>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/56770820>