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

RESOLVED FIXED in Firefox 60

Status

enhancement
P2
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: zer0, Assigned: nchevobbe)

Tracking

unspecified
Firefox 60
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
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
No longer blocks: 1381542
Blocks: 1384546
Whiteboard: [nosdk]
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).
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
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
Attachment #8954740 - Flags: review?(pbrosset) → review+
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.
Attachment #8913658 - Attachment is obsolete: true
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/784928adcf87
Remove old-event-emitter usage from devtools/server; r=pbro.
https://hg.mozilla.org/mozilla-central/rev/784928adcf87
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.