1478945 - JS Debugger + xpcshell-tests should be covered by a tier-1 test suite

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Mark Banner (:standard8)]

xpcshell-tests + jsdebugger keep getting broken, e.g. bug 1198716, bug 1231759, bug 1408012, bug 1457896, bug 1478944

As this is a part of the Firefox developer process, we really should have a tier-1 test that xpcshell + jsdebugger are working correctly and prevent bustages landing.

Comment 1

[Alexandre Poirot [:ochameau]]

You may not believe it, but it actually is covered by a test:
  https://searchfox.org/mozilla-central/source/devtools/server/tests/unit/test_xpcshell_debugging.js
It is actually pretty decent test.

I looked into that as I regressed it last week...

The limitation of this test is that it mostly only covers the Debugger needs and not the toolbox one.
The last regression I introduced via bug 1474980 broke the toolbox setup, not the debugger one.

In order to better cover this feature, we would need to spawn a toolbox and connect to xpcshell,
but that is pretty challenging as we can't spawn a toolbox from xpcshell...
I don't see how to do that without significant work to be done in the test harness in order to spawn xpcshell and firefox at the same time.

Otherwise, I was wondering if we could move this code out from xpcshell to devtools:
  https://searchfox.org/mozilla-central/source/testing/xpcshell/head.js#364-476
and may be have it tested via a mochitest, opening a real toolbox?

Or, I can at least augment /test_xpcshell_debugging.js in order to catch the regression I introduced in bug 1474980 and look at previous regression to ensure that this unit test catches them...

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1478945 - Refactor test_xpcshell_debugging.js to use async/await instead of callbacks.

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

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1478945 - Test global actors against xpcshell debugger server.

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

Comment 4

[Julian Descottes [:jdescottes]]

Comment on attachment 8996378 [details]
Bug 1478945 - Refactor test_xpcshell_debugging.js to use async/await instead of callbacks.

https://reviewboard.mozilla.org/r/260494/#review267686

Thanks for the cleanup!

Comment 5

[Julian Descottes [:jdescottes]]

Comment on attachment 8996379 [details]
Bug 1478945 - Test global actors against xpcshell debugger server.

https://reviewboard.mozilla.org/r/260496/#review267688

Catches the recent regression, thanks!

Comment 6

[Alexandre Poirot [:ochameau]]

Mark, Julian, Do you have any feedback about comment 1?

With these patches, I'm only trying to prevent bug 1474980's regression.
That's already good step, but definitely less than what was originaly requested from comment 0.

Moving code from head.js to some place in devtools may help catch some more regressions,
but I looked into the bugs you linked and that would certainly not have catched bug 1198716.
Bug 1231759 wasn't really a regression, but more like an error logging improvement.
And the regression behind bug 1408012 isn't clear to me, so I don't know how to improve coverage here.

Comment 7

[Julian Descottes [:jdescottes]]

I don't know how we could do a full end-to-end test of this feature? Short of that, extending this test every time we spot a regression feels like the right approach to me.

> And the regression behind bug 1408012 isn't clear to me, so I don't know how 
> to improve coverage here.

If I remember correctly the initialization of the debugger UI could not complete, but we had the same issue in the browser content toolbox, so we added a mochitest for that in https://bugzilla.mozilla.org/show_bug.cgi?id=1407426

Comment 8

[Mark Banner (:standard8)]

Alexandre, thank you for taking this on.

The only other thought I had was maybe some sort of selenium/marionette based test, but I suspect that would be quite hard to set up given the different aspects that need to be controlled.

I think given the limitations you've expressed, extending tests is a reasonable way forward. I was aiming high to begin with, but I'm happy if we can get at least some better coverage - and I hadn't realised that we'd already added at least some mochitests.

Comment 9

[Alexandre Poirot [:ochameau]]

Ok, thanks for the feedback, I'll proceed with these patches.

Yes, marionette would be hard as well.
I'll take a minute to see if we can have a mochitest, to at least involve a toolbox.
One tricky part is to be able to use a module from testing/xpshell/head.js that comes from /devtools/.
But may be TESTING_JS_MODULES could help doing that...

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3768b6b8bf5c
Refactor test_xpcshell_debugging.js to use async/await instead of callbacks. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/beac4909c28e
Test global actors against xpcshell debugger server. r=jdescottes

Comment 11

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/3768b6b8bf5c
https://hg.mozilla.org/mozilla-central/rev/beac4909c28e