Closed Bug 1622043 Opened 5 years ago Closed 5 years ago

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

Categories

(DevTools :: Netmonitor, enhancement)

Product:
DevTools
Component:
Netmonitor
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox76 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 76
Tracking Flags:
Tracking Status
firefox76 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

Attachments

(5 files)

Bug 1622043 - Remove unecessary usages of waitForAllRequestsFinished method.
(deleted), text/x-phabricator-request
Details
Bug 1622043 - Remove waitForAllRequestsFinished in favor of always asserting the expected request count.
(deleted), text/x-phabricator-request
Details
Bug 1622043 - Always wait for Event Timings.
(deleted), text/x-phabricator-request
Details
Bug 1622043 - Remove unecessary wait.
(deleted), text/x-phabricator-request
Details
Bug 1622043 - Use waitForNetworkEvents where we should.
(deleted), text/x-phabricator-request
Details

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.

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

Assignee: nobody → poirot.alex
Status: NEW → ASSIGNED

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.

Attached file Bug 1622043 - Remove unecessary wait. (deleted) —

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.

Pushed by apoirot@mozilla.com: 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
Blocks: 1663877
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: