1109563 - xpcshell-test debugging broken - needs fixing and tests!

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-XXXXXXX-fix-xpshell-test-debugging.-r-past.patch

xpcshell-tests broke days after landing :)  A subsequent change removed createAndStoreBreakpoint.

This patch fixes things and adds tests in the hope of preventing future breakage.  Of note:

* _setBreakpoint is now setBreakpoint(), but I didn't bother with a comment that the xpcshell tests are using it - that comment didn't prevent createAndStoreBreakpoint being removed so I'm not sure it has value ;)

* The test is in the devtools/server directory as xpcshell doesn't have xpcshell tests of its own - ideally we should work out how to move it into testing/xpcshell later.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8534312 [details] [diff] [review]
0001-Bug-XXXXXXX-fix-xpshell-test-debugging.-r-past.patch

Review of attachment 8534312 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/xpcshell/head.js
@@ +379,2 @@
>              let sourceActor = threadActor.sources.source({originalUrl: file});
> +            sourceActor._setBreakpoint(location);

Wait, isn't this setBreakpoint() now (without the underscore)?

::: toolkit/devtools/server/tests/unit/test_xpcshell_debugging.js
@@ +8,5 @@
> +  let testFile = do_get_file("xpcshell_debugging_script.js");
> +
> +  // _setupDebuggerServer is from xpcshell-test's head.js
> +  let testResumed = false;
> +  let DebuggerServer = _setupDebuggerServer([testFile.path], () => {testResumed = true});

You could also omit the curly brackets if you wanted, since you don't really care about the return value.

@@ +21,5 @@
> +          print("yay - hit the first line of our script");
> +          // XXX - check it is our test and the correct location.
> +          // Resume again - next stop should be our "debugger" statement.
> +          threadClient.addOneTimeListener("paused", (aEvent, aPacket) => {
> +            print("yay - hit the 'debugger' statement in our script");

You could verify it, like this:

equal(aPacket.why.type, "debuggerStatement", "yay - hit the 'debugger' statement in our script");

@@ +23,5 @@
> +          // Resume again - next stop should be our "debugger" statement.
> +          threadClient.addOneTimeListener("paused", (aEvent, aPacket) => {
> +            print("yay - hit the 'debugger' statement in our script");
> +            threadClient.resume();
> +            do_test_finished();

Finishing the test should happen after the thread is resumed and preferably after the client disconnects. The common pattern in these tests is:

threadClient.resume(() => finishClient(client);

@@ +31,5 @@
> +        // tell the thread to do the initial resume.  This would cause the
> +        // xpcshell test harness to resume and load the file under test.
> +        threadClient.resume(response => {
> +          // should have been told to resume the test itself.
> +          Assert.ok(testResumed);

Just curious, but is prefixing with Assert required here?

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-1109563-fix-xpshell-test-debugging.-r-past.patch

(In reply to Panos Astithas [:past] from comment #1)
> Wait, isn't this setBreakpoint() now (without the underscore)?

Yep :)  That's what I get when I rush before trying to hit bed :)

> You could also omit the curly brackets if you wanted, since you don't really
> care about the return value.

Done.

> You could verify it, like this:

Done.

> Finishing the test should happen after the thread is resumed and preferably
> after the client disconnects. The common pattern in these tests is:

Done.

> Just curious, but is prefixing with Assert required here?

Nope, and done :)

Thanks!

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8534606 [details] [diff] [review]
0001-Bug-1109563-fix-xpshell-test-debugging.-r-past.patch

Review of attachment 8534606 [details] [diff] [review]:
-----------------------------------------------------------------

Ship it!

Comment 4

[Mark Hammond [:markh] [:mhammond]]

Thanks!

https://hg.mozilla.org/integration/fx-team/rev/0f7e5b1c4b62

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0f7e5b1c4b62