Bug 203262 – Web Inspector: frontend tests should clear output before resending results

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Yury Semikhatsky]

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.

[Yury Semikhatsky]

Created attachment 381619 [details]
Patch

[Yury Semikhatsky]

Created attachment 381637 [details]
Patch

[Devin Rousso]

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 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]

Created attachment 381707 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 381707 [details]
Patch for landing

Clearing flags on attachment: 381707

Committed r251485: <https://trac.webkit.org/changeset/251485>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/56548679>