Fix 10 tests failures on devtools/client/netmonitor due the EventEmitter refactoring

RESOLVED FIXED in Firefox 61

Status

P2
normal
RESOLVED FIXED
a year ago
6 months ago

People

(Reporter: zer0, Assigned: yulia)

Tracking

(Blocks: 1 bug, {stale-bug})

unspecified
Firefox 56
stale-bug
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox57 wontfix, firefox61 fixed)

Details

Attachments

(1 attachment)

Failing tests:

devtools/client/netmonitor/src/har/test/browser_net_har_copy_all_as_har.js
devtools/client/netmonitor/src/har/test/browser_net_har_post_data.js
devtools/client/netmonitor/src/har/test/browser_net_har_throttle_upload.js
devtools/client/netmonitor/test/browser_net_copy_as_curl.js
devtools/client/netmonitor/test/browser_net_copy_headers.js
devtools/client/netmonitor/test/browser_net_copy_image_as_data_uri.js
devtools/client/netmonitor/test/browser_net_copy_params.js
devtools/client/netmonitor/test/browser_net_copy_response.js
devtools/client/netmonitor/test/browser_net_copy_svg_image_as_data_uri.js
devtools/client/netmonitor/test/browser_net_copy_url.js


The refactoring is currently only on:

https://github.com/zer0/gecko/tree/event-emitter-1381542

We need to address the test failures before land this patch in m-c.
Here the original try build with the failures:

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

Those failures are most likely due the breaking change in how the `EventEmitter` emits event.

Previously, the first argument was the type event:

  myEmitter.on("custom-event", (eventType, message) => { ... });

Now the first argument is the message:

  myEmitter.on("custom-event", (message) => { ... });

In the majority of the scenario the `eventType` is ignored by our code, so we should just remove it from the function's signature.

For more details, see: https://github.com/devtools-html/snippets-for-removing-the-sdk/#events
Flags: qe-verify-
Priority: -- → P2
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Target Milestone: --- → Firefox 56
(Reporter)

Updated

a year ago
No longer blocks: 1381542
(Reporter)

Updated

a year ago
Blocks: 1384546
(Reporter)

Updated

a year ago
Whiteboard: [nosdk]
This is an assigned P1 bug without activity in two weeks. 

If you intend to continue working on this bug for the current release/iteration/sprint, remove the 'stale-bug' keyword.

Otherwise we'll reset the priority of the bug back to '--' on Monday, August 28th.
Keywords: stale-bug
status-firefox57: --- → fix-optional
No longer blocks: 1402832
status-firefox57: fix-optional → wontfix
I am currently not working on this. Unassigned myself, so somebody else can pick this up.

@Sole: Is this still an issue actually? The tests from comment #0 are green on try.

Honza
Assignee: odvarko → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(spenades)
I'm not sure if the tests are green because they have been updated or because they failed with the initial refactoring (that was backtracked), and don't fail because of that reason (the refactoring didn't happen in the disruptive way).

Either way at some point we'll need to change the monitor to use the new event emitter, not the old one. And then I reckon maybe the tests will fail?
Flags: needinfo?(spenades)
That's it, we didn't continue the refactor. The netmonitor still uses the old-event-emitter, and therefore the tests pass.
This bug was filed as a consequence of Matteo migrating everything to the new event-emitter and listing all of the bugs that failed as a result of this.
(Assignee)

Updated

9 months ago
Assignee: nobody → ystartsev
Comment hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Attachment #8958818 - Flags: superreview?(nchevobbe)
(Assignee)

Updated

9 months ago
Attachment #8958818 - Flags: superreview?(nchevobbe) → review?(nchevobbe)
Comment on attachment 8958818 [details]
Bug 1382602 - update devtools/client/netmonitor to use new event emitter.

https://reviewboard.mozilla.org/r/227718/#review233508

Thanks for the patch Yulia, looks great to me!

R+

Honza
Attachment #8958818 - Flags: review?(odvarko) → review+

Comment 8

9 months ago
mozreview-review
Comment on attachment 8958818 [details]
Bug 1382602 - update devtools/client/netmonitor to use new event emitter.

https://reviewboard.mozilla.org/r/227718/#review233812

One comment slightly unrelated to the patch, but this looks good to me :)
Thanks yulia !

::: devtools/client/netmonitor/test/browser_net_pause.js:92
(Diff revision 1)
>    return new Promise(resolve => {
> -    connector.connector.webConsoleClient.once("networkEvent", (type, networkInfo) => {
> +    connector.connector.webConsoleClient.once("networkEvent", networkInfo => {
>        resolve();
>      });

I think we could do: 

return connector.connector.webConsoleClient.once("networkEvent");
Attachment #8958818 - Flags: review?(nchevobbe) → review+
Comment hidden (mozreview-request)

Comment 10

9 months ago
Pushed by ystartsev@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e059b4fc0521
update devtools/client/netmonitor to use new event emitter. r=Honza,nchevobbe

Comment 11

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e059b4fc0521
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox61: --- → fixed
Resolution: --- → FIXED

Updated

6 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.