Bug 203262

Summary:

Web Inspector: frontend tests should clear output before resending results

Product:

WebKit

Reporter:

Yury Semikhatsky <yurys>

Component:

Web Inspector

Assignee:

Yury Semikhatsky <yurys>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, hi, inspector-bugzilla-changes, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

202704

Attachments:

Description	Flags
Patch	none
Patch	none
Patch for landing	none

Yury Semikhatsky

Reported 2019-10-22 12:25:29 PDT

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.

Attachments
Patch (9.73 KB, patch) 2019-10-22 15:25 PDT, Yury Semikhatsky	no flags	Details Formatted Diff Diff
Patch (12.48 KB, patch) 2019-10-22 17:30 PDT, Yury Semikhatsky	no flags	Details Formatted Diff Diff
Patch for landing (12.53 KB, patch) 2019-10-23 11:39 PDT, Yury Semikhatsky	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Yury Semikhatsky

Comment 1 2019-10-22 15:25:28 PDT

Created attachment 381619 [details] Patch

Yury Semikhatsky

Comment 2 2019-10-22 17:30:17 PDT

Created attachment 381637 [details] Patch

Devin Rousso

Comment 3 2019-10-23 11:25:31 PDT

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

Yury Semikhatsky

Comment 4 2019-10-23 11:38:42 PDT

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.

Yury Semikhatsky

Comment 5 2019-10-23 11:39:22 PDT

Created attachment 381707 [details] Patch for landing

WebKit Commit Bot

Comment 6 2019-10-23 12:09:11 PDT

Comment on attachment 381707 [details] Patch for landing Clearing flags on attachment: 381707 Committed r251485: <https://trac.webkit.org/changeset/251485>

WebKit Commit Bot

Comment 7 2019-10-23 12:09:13 PDT

All reviewed patches have been landed. Closing bug.

Radar WebKit Bug Importer

Comment 8 2019-10-23 12:10:16 PDT

<rdar://problem/56548679>

Note You need to log in before you can comment on or make changes to this bug.