Closed Bug 1109563 Opened 8 years ago Closed 8 years ago

xpcshell-test debugging broken - needs fixing and tests!

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 37

People

(Reporter: markh, Assigned: markh)

Details

Attachments

(1 file, 1 obsolete file)

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.
Attachment #8534312 - Flags: feedback?(past)
Summary: xpcshell test broken - needs fixing and tests! → xpcshell-test debugging broken - needs fixing and tests!
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?
Attachment #8534312 - Flags: feedback?(past) → feedback+
(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!
Attachment #8534312 - Attachment is obsolete: true
Attachment #8534606 - Flags: review?(past)
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!
Attachment #8534606 - Flags: review?(past) → review+
Thanks!

https://hg.mozilla.org/integration/fx-team/rev/0f7e5b1c4b62
Assignee: nobody → mhammond
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/0f7e5b1c4b62
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.