Summary: | inspector-protocol/debugger/setBreakpoint-dfg.html is flaky | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michal Pakula vel Rutka <mpakulavelrutka> | ||||||||
Component: | Tools / Tests | Assignee: | Yury Semikhatsky <yurys> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, hi, joepeck, lucas.de.marchi, webkit-bug-importer, yurys | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Michal Pakula vel Rutka
2014-02-13 04:08:33 PST
This happens on Mac too, with the same diff. Will move the expectation to cross-platform file. Created attachment 380656 [details]
Patch
Created attachment 380668 [details]
Patch
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 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 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. Created attachment 382001 [details]
Patch
Comment on attachment 382001 [details]
Patch
rs=me
Comment on attachment 382001 [details] Patch Clearing flags on attachment: 382001 Committed r251833: <https://trac.webkit.org/changeset/251833> All reviewed patches have been landed. Closing bug. |