1622043 - Converge all waiting helper of the netmonitor tests into one or two

Status: RESOLVED FIXED

(DevTools :: Netmonitor, enhancement)

Description

[Alexandre Poirot [:ochameau]]

We have way too many different wait mechanisms in the netmonitor tests:

• waitForAllNetworkUpdateEvents
• waitForNetworkEvents
• waitForAllRequestsFinished
• waitForRequestsFinished
• and probably some others...

Bug 1439509 highlights that the existing helpers are not enough and a combination of either waitForNetworkEvents + waitForAllNetworkUpdateEvents or initNetmonitor + waitForAllNetworkUpdateEvents are necessary in order to prevent having pending requests.

In his WIP patch, Sorin started adding calls to waitForAllNetworkUpdateEvents here and there, but that doesn't scale:

• it piles up wait methods. Everywhere a call to waitForAllNetworkUpdateEvents was added, there was already a wait method in the test. Either initNetMonitor (which calls some wait methods internally) or waitForNetworkEvents.
• it most likely require to go modify all netmonitor tests!

We should probably first do a swap on all these various wait methods, so that we can more easily integrate waitForAllNetworkUpdateEvents into all the existing methods.

I started looking and the lack of consistancy in the helper methods being used highlights that we are not waiting for many async calls. This is probably the source of various intermittents.

Comment 1

[Alexandre Poirot [:ochameau]]

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

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1622043 - Remove unecessary usages of waitForAllRequestsFinished method.

Other tests are typically using waitForNetworkEvents when doing a reload,
with a clear assertion on how many requests should be displayed and awaited for.

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1622043 - Remove waitForAllRequestsFinished in favor of always asserting the expected request count.

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1622043 - Always wait for Event Timings.

We were explicitely waiting for event timings in two tests only,
but these requests are also done in most other tests using waitForNetworkEvents.
Unfortunately, EVENT_TIMINGS aren't always fetched, so it complexify
waitForNetworkEvents a bit. But I think it would prevent pending requests
and helps using a unique wait helper.

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1622043 - Remove unecessary wait.

This test was wrong. The event timings were fire before, when performing the request.
Not when opening the context menu. So, with the previous changeset, this test started to fail.

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1622043 - Use waitForNetworkEvents where we should.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a8244239b26b
Remove unecessary usages of waitForAllRequestsFinished method. r=Honza,bomsy
https://hg.mozilla.org/integration/autoland/rev/8e3c88718a88
Remove waitForAllRequestsFinished in favor of always asserting the expected request count. r=Honza,bomsy
https://hg.mozilla.org/integration/autoland/rev/1d8ef16a713b
Always wait for Event Timings. r=Honza,bomsy
https://hg.mozilla.org/integration/autoland/rev/cb0ca583ad73
Remove unecessary wait. r=Honza,bomsy
https://hg.mozilla.org/integration/autoland/rev/527cb874447a
Use waitForNetworkEvents where we should. r=Honza,bomsy

Comment 8

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/a8244239b26b
https://hg.mozilla.org/mozilla-central/rev/8e3c88718a88
https://hg.mozilla.org/mozilla-central/rev/1d8ef16a713b
https://hg.mozilla.org/mozilla-central/rev/cb0ca583ad73
https://hg.mozilla.org/mozilla-central/rev/527cb874447a