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

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P2)

Description

[Matteo Ferretti [:zer0] [:matteo]]

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.

Comment 1

[Matteo Ferretti [:zer0] [:matteo]]

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

Comment 2

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

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.

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 4

[Soledad Penades [:sole] [: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?

Comment 5

[Patrick Brosset <:pbro>]

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.

Comment 6

[Yulia Startsev [:yulia]]

Attachment: Bug 1382602 - update devtools/client/netmonitor to use new event emitter.

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

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

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");

Comment 9

[Yulia Startsev [:yulia]]

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

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e059b4fc0521
update devtools/client/netmonitor to use new event emitter. r=Honza,nchevobbe

Comment 11

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/e059b4fc0521