1382609 - Fix 2 tests failures on devtools/server/tests due the EventEmitter refactoring

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P2)

Description

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

Failing tests:

devtools/server/tests/mochitest/test_connection-manager.html
devtools/server/tests/mochitest/test_inspector-pick-color.html


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

[Patrick Brosset <:pbro>]

I have been looking at this yesterday. I've got something that seems to work when you run DevTools, but fails a lot of tests.
Changing all event-emitters in devtools/server to be the new one is fairly easy, but then it gets more involved, because we require some scripts in devtools/shared from both client and server. In particular, the DevTools protocol transport library is there. I haven't solved all the issues related to that yet.
I'm not assigning this bug to myself just yet, because I don't know if I'll be able to work on it enough to actually solve the remaining pieces. But make sure to start from my patch if you want to attempt this (I'll attach it next).

Comment 3

[Patrick Brosset <:pbro>]

Attachment: Bug1382609-new-event-emitter-in-devtools-server.diff

Try build with this patch applied: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9d5771876c23d4c1a3814e4a230dd206dd5268b&group_state=expanded

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1382609 - Remove old-event-emitter usage from devtools/server; .

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

Comment 5

[Patrick Brosset <:pbro>]

Comment on attachment 8954740 [details]
Bug 1382609 - Remove old-event-emitter usage from devtools/server; .

https://reviewboard.mozilla.org/r/223874/#review230212

This looks good. Thank you Nicolas.
I only have a few minor comment that you can feel free to ignore.
I'm just concerned about the impact of the change. I mean, this is the right change, but I was expecting this to basically require the whole rest of the code to be refactored to the new event-emitter too.
But if try is happy and devtools still works, then I'm happy too.

::: devtools/shared/transport/tests/unit/test_queue.js:115
(Diff revision 1)
>        info(Object.keys(DebuggerServer._connections));
>        for (let connId in DebuggerServer._connections) {
>          DebuggerServer._connections[connId].onBulkPacket = on_bulk_packet;
>        }
>  
> -      DebuggerServer.on("connectionchange", (event, type) => {
> +      DebuggerServer.on("connectionchange", (type) => {

nit: no need for parens around a single parameter in arrow functions.

::: devtools/shared/transport/tests/unit/test_transport_bulk.js:88
(Diff revision 1)
>        info(Object.keys(DebuggerServer._connections));
>        for (let connId in DebuggerServer._connections) {
>          DebuggerServer._connections[connId].onBulkPacket = on_bulk_packet;
>        }
>  
> -      DebuggerServer.on("connectionchange", (event, type) => {
> +      DebuggerServer.on("connectionchange", (type) => {

same here

::: devtools/shared/transport/tests/unit/test_transport_events.js:31
(Diff revision 1)
> -  let rootReceived = transport.once("packet", (event, packet) => {
> -    info(`Packet event: ${event} ${JSON.stringify(packet)}`);
> +  let rootReceived = transport.once("packet", (packet) => {
> +    info(`Packet event: ${JSON.stringify(packet)}`);
> -    Assert.equal(event, "packet");
>      Assert.equal(packet.from, "root");
>    });
>  
>    transport.ready();
>    await rootReceived;
>  
> -  let echoSent = transport.once("send", (event, packet) => {
> -    info(`Send event: ${event} ${JSON.stringify(packet)}`);
> +  let echoSent = transport.once("send", (packet) => {
> +    info(`Send event: ${JSON.stringify(packet)}`);
> -    Assert.equal(event, "send");
>      Assert.equal(packet.to, "root");
>      Assert.equal(packet.type, "echo");
>    });
>  
> -  let echoReceived = transport.once("packet", (event, packet) => {
> -    info(`Packet event: ${event} ${JSON.stringify(packet)}`);
> +  let echoReceived = transport.once("packet", (packet) => {
> +    info(`Packet event: ${JSON.stringify(packet)}`);

and here.

::: devtools/shared/transport/tests/unit/test_transport_events.js:59
(Diff revision 1)
> -  let clientClosed = transport.once("close", (event) => {
> -    info(`Close event: ${event}`);
> +  let clientClosed = transport.once("close", () => {
> +    info(`Close event`);
> -    Assert.equal(event, "close");
>    });
>  
> -  let serverClosed = DebuggerServer.once("connectionchange", (event, type) => {
> +  let serverClosed = DebuggerServer.once("connectionchange", (type) => {

and here

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8954740 [details]
Bug 1382609 - Remove old-event-emitter usage from devtools/server; .

https://reviewboard.mozilla.org/r/223874/#review230212

Thanks for the review Patrick. I fixed the nits, and I'm going to push to TRY on different platform (previous one was linux only) so we are extra careful about this.

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8954740 [details]
Bug 1382609 - Remove old-event-emitter usage from devtools/server; .

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/784928adcf87
Remove old-event-emitter usage from devtools/server; r=pbro.

Comment 9

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/784928adcf87