Closed Bug 1382577 Opened 3 years ago Closed 2 years ago

Fix 8 tests failures on devtools/client/canvasdebugger due the EventEmitter refactoring

Categories

(DevTools Graveyard :: Canvas Debugger, enhancement, P2)

enhancement

Tracking

(firefox60 fixed)

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: zer0, Assigned: nchevobbe)

References

Details

Attachments

(1 file)

Failing tests:

devtools/client/canvasdebugger/test/browser_canvas-frontend-call-stack-01.js
devtools/client/canvasdebugger/test/browser_canvas-frontend-call-stack-02.js
devtools/client/canvasdebugger/test/browser_canvas-frontend-img-thumbnails-02.js
devtools/client/canvasdebugger/test/browser_canvas-frontend-record-01.js
devtools/client/canvasdebugger/test/browser_canvas-frontend-record-02.js
devtools/client/canvasdebugger/test/browser_canvas-frontend-reload-01.js
devtools/client/canvasdebugger/test/browser_canvas-frontend-reload-02.js
devtools/client/canvasdebugger/test/browser_canvas-frontend-snapshot-select-01.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 event type:

  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 and edge cases, 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]
Component: Developer Tools → Developer Tools: Canvas Debugger
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
No tests failing after switching to the new event emitter. I guess the test failure were seen when using the new EventEmitter on a global toolbox object (target ?)
Comment on attachment 8955418 [details]
Bug 1382577 - Remove old-event-emitter usage from canvasdebugger; .

https://reviewboard.mozilla.org/r/224602/#review230642

There are some failures on the try run, but I don't think they're related to your patch (I wonder if maybe you should run a more limited set of tests since you're not changing code in Firefox?).

I applied the patch locally and run the canvas debugger tests, which passed. It works nicely as well, as I tried capturing a stack and using it normally and I couldn't see any weirdness.
Attachment #8955418 - Flags: review?(spenades) → review+
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8d70045ab416
Remove old-event-emitter usage from canvasdebugger; r=sole.
https://hg.mozilla.org/mozilla-central/rev/8d70045ab416
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.