FrontendTestHarness._resendResults should clear output before re-adding results to the test output after navigation. Otherwise there is a race between TestPage.addResult commands that are sent after navigation started but before InspectorTest.testPageDidLoad is called again. Such in flight commands may be sent to the new page second time from _resendResults introducing flakiness to the tests that rely on navigation.
Created attachment 381619 [details] Patch
Created attachment 381637 [details] Patch
Comment on attachment 381637 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=381637&action=review r=me, with some style comments > LayoutTests/http/tests/inspector/resources/inspector-test.js:180 > + return; > + let output = this._resultElement; Style: missing newline > LayoutTests/http/tests/inspector/resources/inspector-test.js:182 > + while (output.firstChild) > + output.removeChild(output.firstChild); NIT: we normally just use `output.textContent = "";` to remove all children. > LayoutTests/inspector/debugger/breakpoint-action-eval.html:41 > + let reloadPromise = InspectorTest.awaitEvent(FrontendTestHarness.Event.TestPageDidLoad); NIT: I'd put this before the `InspectorTest.reloadPage` call, so that we're already listening for the event before we trigger anything on the backend > LayoutTests/inspector/debugger/breakpoint-action-eval.html:43 > + Promise.all([reloadPromise, breakpointPromise.promise]) > + .then(() => InspectorTest.evaluateInPage("runBreakpointActions()")); Style: we normally split this onto separate lines and don't indent the `.then(...)`: ``` Promise.all([ reloadPromise, breapointPromise.promise, ]) .then(() => { InspectorTest.evaluateInPage("runBreakpointActions()"); }); ``` > LayoutTests/inspector/debugger/breakpoint-action-log.html:53 > + let reloadPromise = InspectorTest.awaitEvent(FrontendTestHarness.Event.TestPageDidLoad); > + Promise.all([reloadPromise, breakpointPromise.promise]) > + .then(() => InspectorTest.evaluateInPage("runBreakpointActions()")) > + .then(resolve);; Ditto > LayoutTests/inspector/debugger/probe-manager-add-remove-actions.html:55 > + let reloadPromise = InspectorTest.awaitEvent(FrontendTestHarness.Event.TestPageDidLoad); > + Promise.all([reloadPromise, breakpointPromise.promise]) > + .then(() => InspectorTest.evaluateInPage("breakpointActions(12, {x:1,y:2})")); Ditto > LayoutTests/inspector/debugger/probe-manager-add-remove-actions.html:56 > + Style: unnecessary newline
Comment on attachment 381637 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=381637&action=review >> LayoutTests/http/tests/inspector/resources/inspector-test.js:180 >> + let output = this._resultElement; > > Style: missing newline Done. >> LayoutTests/http/tests/inspector/resources/inspector-test.js:182 >> + output.removeChild(output.firstChild); > > NIT: we normally just use `output.textContent = "";` to remove all children. Done. I was contemplating that, but then stackoverflow wisdom that iterating children and removing them one by one would be more performant convinced me otherwise :-) >> LayoutTests/inspector/debugger/breakpoint-action-eval.html:41 >> + let reloadPromise = InspectorTest.awaitEvent(FrontendTestHarness.Event.TestPageDidLoad); > > NIT: I'd put this before the `InspectorTest.reloadPage` call, so that we're already listening for the event before we trigger anything on the backend Done. >> LayoutTests/inspector/debugger/breakpoint-action-eval.html:43 >> + .then(() => InspectorTest.evaluateInPage("runBreakpointActions()")); > > Style: we normally split this onto separate lines and don't indent the `.then(...)`: > ``` > Promise.all([ > reloadPromise, > breapointPromise.promise, > ]) > .then(() => { > InspectorTest.evaluateInPage("runBreakpointActions()"); > }); > ``` Done. >> LayoutTests/inspector/debugger/breakpoint-action-log.html:53 >> + .then(resolve);; > > Ditto Done. >> LayoutTests/inspector/debugger/probe-manager-add-remove-actions.html:55 >> + .then(() => InspectorTest.evaluateInPage("breakpointActions(12, {x:1,y:2})")); > > Ditto Done. >> LayoutTests/inspector/debugger/probe-manager-add-remove-actions.html:56 >> + > > Style: unnecessary newline Removed.
Created attachment 381707 [details] Patch for landing
Comment on attachment 381707 [details] Patch for landing Clearing flags on attachment: 381707 Committed r251485: <https://trac.webkit.org/changeset/251485>
All reviewed patches have been landed. Closing bug.
<rdar://problem/56548679>