Web Inspector: inspector/layers/layers-for-node.html and inspector/timeline/line-column.html are flaky

• Component: Web Inspector
• Status: RESOLVED FIXED

[Yury Semikhatsky]

The two tests became very flaky after replacing Timer based queue with RunLoop one in https://trac.webkit.org/changeset/250655/webkit. The change seems to have revealed existing flaw in the tests as it essentially modified the timing of the event dispatches.

inspector/layers/layers-for-node.html 
inspector/timeline/line-column.html

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Flayers%2Flayers-for-node.html%20inspector%2Ftimeline%2Fline-column.html

Diff:
https://build.webkit.org/results/Apple%20Mojave%20Release%20WK1%20(Tests)/r250672%20(8069)/inspector/layers/layers-for-node-diff.txt
https://build.webkit.org/results/Apple%20Mojave%20Release%20WK1%20(Tests)/r250672%20(8069)/inspector/timeline/line-column-diff.txt

[Yury Semikhatsky]

Created attachment 380350 [details]
Patch

[Devin Rousso]

Comment on attachment 380350 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=380350&action=review

rs=me

> LayoutTests/inspector/layers/layers-for-node.html:25
> +        layersChanged = InspectorProtocol.awaitEvent({event: "LayerTree.layerTreeDidChange"})

Frankly, given that `enableLayerTreeAgent` is called synchronously at the start of the test, perhaps we should just inline this at the top of `test` as a sort of "make sure we get at least one of these before continuing with the rest of the test".

> LayoutTests/inspector/layers/layers-for-node.html:36
> +        layersChanged.then(() => step({

Style: if the body of the arrow function is more than one line (or the value isn't intended as the return value), we add the `{ ... }` to the arrow function.
```js
    layersChanged.then(() => {
        step({
            ...
        });
    });
```

[Yury Semikhatsky]

Created attachment 380361 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 380350 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=380350&action=review

>> LayoutTests/inspector/layers/layers-for-node.html:25
>> +        layersChanged = InspectorProtocol.awaitEvent({event: "LayerTree.layerTreeDidChange"})
> 
> Frankly, given that `enableLayerTreeAgent` is called synchronously at the start of the test, perhaps we should just inline this at the top of `test` as a sort of "make sure we get at least one of these before continuing with the rest of the test".

Done.

>> LayoutTests/inspector/layers/layers-for-node.html:36
>> +        layersChanged.then(() => step({
> 
> Style: if the body of the arrow function is more than one line (or the value isn't intended as the return value), we add the `{ ... }` to the arrow function.
> ```js
>     layersChanged.then(() => {
>         step({
>             ...
>         });
>     });
> ```

Moved this logic out of the method.

[Devin Rousso]

Comment on attachment 380361 [details]
Patch

rs=me

[WebKit Commit Bot]

Comment on attachment 380361 [details]
Patch

Clearing flags on attachment: 380361

Committed r250872: <https://trac.webkit.org/changeset/250872>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/56096739>