1411090 - Improve RDM test reliability using --verify mode

Status: RESOLVED FIXED

(DevTools :: Responsive Design Mode, enhancement, P3)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

The recently added --verify highlights some issues in RDM tests.  Resolving these should help with intermittent failures.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Well, getting through all the issues is taken longer than I'd like.  So, I'll post some parts for now and revisit the rest later.

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1411090 - Convert devtools/client/shared/devices.js to async / await.

Review commit: https://reviewboard.mozilla.org/r/194174/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194174/

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1411090 - Clear local device cache when testing.

Review commit: https://reviewboard.mozilla.org/r/194176/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194176/

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1411090 - Stop loading frame scripts in outer browser.

The tunnel was being too loose with loading frame scripts, and would load them
in the outer browser, when really there's nothing to gain when doing so.

Additionally, it causes intermittent issues: `remote-browser.xml` will load
`browser-child.js` during construction, which may cause the outer browser to
send progress updates, confusing the UI and causing intermittent failures in
`browser_navigation.js`.

We want to ignore the outer browser's state as much as we can, so blocking it
from loading frame scripts seems reasonable here.

Review commit: https://reviewboard.mozilla.org/r/194178/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194178/

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1411090 - Update browser_navigation to avoid intermittents.

Review commit: https://reviewboard.mozilla.org/r/194180/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194180/

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c22598b0f72c54c0f629b223bf99402847620112

Comment 7

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8922986 [details]
Bug 1411090 - Convert devtools/client/shared/devices.js to async / await.

https://reviewboard.mozilla.org/r/194174/#review199240

::: devtools/client/shared/devices.js:48
(Diff revision 1)
>  let localDevicesLoaded = false;
>  
> -// Load local devices from storage.
> -let loadLocalDevices = Task.async(function* () {
> +/**
> + * Load local devices from storage.
> + */
> +let loadLocalDevices = async () => {

Can we simplify this by doing:
async function loadLocalDevices() {
 ...
}
exports.loadLocalDevices

::: devtools/client/shared/devices.js:96
(Diff revision 1)
>    list.splice(index, 1);
> -  yield asyncStorage.setItem(LOCAL_DEVICES, JSON.stringify(localDevices));
> +  await asyncStorage.setItem(LOCAL_DEVICES, JSON.stringify(localDevices));
>  
>    return true;
> -});
> +};
>  exports.removeDevice = removeDevice;

I personally prefer to see all exports at the end of the file.

Comment 8

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8922987 [details]
Bug 1411090 - Clear local device cache when testing.

https://reviewboard.mozilla.org/r/194176/#review199242

::: devtools/client/shared/devices.js:101
(Diff revision 1)
>  exports.removeDevice = removeDevice;
>  
>  /**
> + * Remove all local devices.  Useful to clear everything when testing.
> + */
> +let removeLocalDevices = async () => {

Same comments as part 1.

Comment 9

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8922988 [details]
Bug 1411090 - Stop loading frame scripts in outer browser.

https://reviewboard.mozilla.org/r/194178/#review199244

Comment 10

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8922989 [details]
Bug 1411090 - Update browser_navigation to avoid intermittents.

https://reviewboard.mozilla.org/r/194180/#review199246

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8922986 [details]
Bug 1411090 - Convert devtools/client/shared/devices.js to async / await.

https://reviewboard.mozilla.org/r/194174/#review199240

> Can we simplify this by doing:
> async function loadLocalDevices() {
>  ...
> }
> exports.loadLocalDevices

Sure!  I've updated them to match.

> I personally prefer to see all exports at the end of the file.

Okay, sounds good to me to.  I've changed to this format.

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cf5806ddec3f70aa3b104bb2bea63e78dd9c625

Comment 13

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8922986 [details]
Bug 1411090 - Convert devtools/client/shared/devices.js to async / await.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194174/diff/1-2/

Comment 14

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8922987 [details]
Bug 1411090 - Clear local device cache when testing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194176/diff/1-2/

Comment 15

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

The last 2 patches need more work (they cause an intermittent to become more common), so I'll refine them in a separate bug.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c662b7320a43
Convert devtools/client/shared/devices.js to async / await. r=gl
https://hg.mozilla.org/integration/autoland/rev/b93a7bc80b11
Clear local device cache when testing. r=gl

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/c662b7320a43
https://hg.mozilla.org/mozilla-central/rev/b93a7bc80b11