Closed Bug 1147972 Opened 5 years ago Closed Last year

Write tests for the breakpoint code.

Categories

(DevTools :: Debugger, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ejpbruel, Assigned: ejpbruel)

References

(Blocks 1 open bug)

Details

Attachments

(21 files, 2 obsolete files)

9.20 KB, patch
jlong
: review+
Details | Diff | Splinter Review
10.84 KB, patch
jlong
: review+
Details | Diff | Splinter Review
3.67 KB, patch
jlong
: review+
Details | Diff | Splinter Review
5.97 KB, patch
jlong
: review+
Details | Diff | Splinter Review
5.12 KB, patch
jlong
: review+
Details | Diff | Splinter Review
4.42 KB, patch
jlong
: review+
Details | Diff | Splinter Review
4.08 KB, patch
jlong
: review+
Details | Diff | Splinter Review
4.30 KB, patch
jlong
: review+
Details | Diff | Splinter Review
7.07 KB, patch
jlong
: review+
Details | Diff | Splinter Review
6.75 KB, patch
jlong
: review+
Details | Diff | Splinter Review
4.48 KB, patch
jlong
: review+
Details | Diff | Splinter Review
10.81 KB, patch
jlong
: review+
Details | Diff | Splinter Review
4.04 KB, patch
jlong
: review+
Details | Diff | Splinter Review
59.58 KB, patch
jlong
: review+
Details | Diff | Splinter Review
33.22 KB, patch
jlong
: review+
Details | Diff | Splinter Review
19.76 KB, patch
jlong
: review+
Details | Diff | Splinter Review
1.97 KB, patch
jlong
: review+
Details | Diff | Splinter Review
2.18 KB, patch
jlong
: review+
Details | Diff | Splinter Review
36.16 KB, patch
jlong
: review+
Details | Diff | Splinter Review
4.86 KB, patch
jlong
: review+
Details | Diff | Splinter Review
3.46 KB, patch
jlong
: review+
Details | Diff | Splinter Review
Now that we've refactored the breakpoint sliding algorithm, we should write plenty of tests to make sure we're doing it right this time.

The plan is to start with tests for non-sourcemapped sources first, and sourcemapped sources second (since those are more complex).

I want to cover as many edge cases as possible, such as: setting a breakpoint on a line, setting a breakpoint on a column with no offsets, setting a breakpoint on a line with no offsets at the end of a function, setting a column breakpoint in a gcd script, etc.
Attached patch setBreakpoint-on-line (obsolete) — Splinter Review
Test setting a breakpoint on a line. This is the most basic case, so we probably already have a test for this. That said, it can't hurt to test for this again as part of a series of related tests (since all of these tests will be written in the same style, which should make them easier to understand).

My goal was to make the test as readable as possible. That means using Task.js, and using helper functions with descriptive names. I'm not a big fan of helper functions that combine two steps into one, such as resumeAndWaitForPause (I feel that they obscure control flow, and make tests less easier to change afterwards), so I've avoided them in favor of some new helper functions that I added.

Each of these helper functions is as simple as possible, and does one thing and one thing only. They are simple enough that I don't think they would benefit from adding a comment for each of them.

I've put this patch up first because it establishes the coding style I have in mind for these tests, and I'd like to make sure you agree before putting the other tests up.
Attachment #8583924 - Flags: review?(jlong)
Oh, one final remark. I've disabled packet logging by default because in my experience it obscures what each test is doing by logging too much. In practice, I'm only interested in packet logging when a test breaks down, and it's not obvious what's going on. With that in mind, I think it's preferable to have it off by default, and only turn it on when debugging a faulty test.
Comment on attachment 8583924 [details] [diff] [review]
setBreakpoint-on-line

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

::: toolkit/devtools/server/tests/unit/head_dbg.js
@@ +16,4 @@
>  const Services = devtools.require("Services");
>  // Always log packets when running tests. runxpcshelltests.py will throw
>  // the output away anyway, unless you give it the --verbose flag.
> +Services.prefs.setBoolPref("devtools.debugger.log", false);

I don't agree with this change, but not enough to argue a ton about it.

That said, at least update the comment above to be correct with regards to this change.

@@ +122,5 @@
> +    sourceClient.setBreakpoint(location, function (response, breakpointClient) {
> +      resolve([response, breakpointClient]);
> +    });
> +  });
> +}

These helpers should use `rdpRequest` so that if the server fails, catches its own error, and sends an error response of the form `{ error, message }` the promises get rejected and our failing tests are easier to debug. In addition, you won't be repeating yourself so much here.

::: toolkit/devtools/server/tests/unit/test_setBreakpoint-on-line.js
@@ +30,5 @@
> +    let [packet, breakpointClient] = yield setBreakpoint(sourceClient, location);
> +    do_check_false("actualLocation" in packet);
> +
> +    executeSoon(function () { Cu.evalInSandbox("f()", global); });
> +    packet = yield waitForPaused(threadClient);

We have to do this so often that we have a nice helper: executeOnNextTickAndWaitForPause. It also documents why the executeSoon is needed in a single place, which is valuable because executeSoon/setTimeout in a test often lead to intermittent failures and you should generally be wary whenever you see them.
(In reply to Nick Fitzgerald [:fitzgen] from comment #3)
> Comment on attachment 8583924 [details] [diff] [review]
> setBreakpoint-on-line
> 
> Review of attachment 8583924 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/server/tests/unit/head_dbg.js
> @@ +16,4 @@
> >  const Services = devtools.require("Services");
> >  // Always log packets when running tests. runxpcshelltests.py will throw
> >  // the output away anyway, unless you give it the --verbose flag.
> > +Services.prefs.setBoolPref("devtools.debugger.log", false);
> 
> I don't agree with this change, but not enough to argue a ton about it.
> 
> That said, at least update the comment above to be correct with regards to
> this change.
> 

We can let jlongster break the tie. That said, my personal experience is that these logs get in the way more than they help most of the time, and I always turn them off initially when hunting for a problem.

> @@ +122,5 @@
> > +    sourceClient.setBreakpoint(location, function (response, breakpointClient) {
> > +      resolve([response, breakpointClient]);
> > +    });
> > +  });
> > +}
> 
> These helpers should use `rdpRequest` so that if the server fails, catches
> its own error, and sends an error response of the form `{ error, message }`
> the promises get rejected and our failing tests are easier to debug. In
> addition, you won't be repeating yourself so much here.
> 

Ok, that's reasonable.

> ::: toolkit/devtools/server/tests/unit/test_setBreakpoint-on-line.js
> @@ +30,5 @@
> > +    let [packet, breakpointClient] = yield setBreakpoint(sourceClient, location);
> > +    do_check_false("actualLocation" in packet);
> > +
> > +    executeSoon(function () { Cu.evalInSandbox("f()", global); });
> > +    packet = yield waitForPaused(threadClient);
> 
> We have to do this so often that we have a nice helper:
> executeOnNextTickAndWaitForPause. It also documents why the executeSoon is
> needed in a single place, which is valuable because executeSoon/setTimeout
> in a test often lead to intermittent failures and you should generally be
> wary whenever you see them.

In this particular case, I agree, since it's a recurring pattern.
Comment on attachment 8583924 [details] [diff] [review]
setBreakpoint-on-line

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

Are you going to be replacing some of the test_breakpoint-*.js tests? I'd like to replace them instead of just piling on top.

::: toolkit/devtools/server/tests/unit/head_dbg.js
@@ +16,4 @@
>  const Services = devtools.require("Services");
>  // Always log packets when running tests. runxpcshelltests.py will throw
>  // the output away anyway, unless you give it the --verbose flag.
> +Services.prefs.setBoolPref("devtools.debugger.log", false);

I would love to have better logs. I want a few different log levels, and we could log various breakpoint activity without turning on the entire protocol log.

For now though, I do always turn the log off by default as well, so I'm find with this. Personally I've never been "saved" by this being on by default when trying to debug a rare intermittent; I almost always have to get the intermittent to happen again anyway in a controlled environment.

::: toolkit/devtools/server/tests/unit/test_setBreakpoint-on-line.js
@@ +6,5 @@
> +  return Task.spawn(function* () {
> +    do_test_pending();
> +
> +    DebuggerServer.registerModule("xpcshell-test/testactors");
> +    DebuggerServer.init(() => true);

How is this different than `initTestDebuggerServer`?

@@ +12,5 @@
> +    let global = createTestGlobal("test");
> +    DebuggerServer.addTestGlobal(global);
> +
> +    let client = new DebuggerClient(DebuggerServer.connectPipe());
> +    yield connect(client);

Might be nice to abstract out this connecting workflow as well, a lot of other tests do this. Fine for now though.
Attachment #8583924 - Flags: review?(jlong) → review+
New patch with comments by fitzgen and jlongster addressed.

Instead of changing the comment where we turn off the logging, I've removed turning the logging of completely. That way we'll just use whatever setting the pref is set to (which should be false by default), which seems the appropriate thing to do.

I've reimplemented some of my helper functions using rdpRequest, and used executeOnNextTickAndWaitForPause in the test where appropriate, as requested by fitzgen.

Jlongster, if you feel strongly about replacing some of the repeating code in these tests with more helper functions, we can talk about that (though I personally prefer a more explicit control flow in tests), but let's do so after we land these tests, ok?
Attachment #8583924 - Attachment is obsolete: true
Attachment #8584445 - Flags: review?(jlong)
Note that we have to load the source in this patch before starting the debugger server, otherwise the script will never be gc'd (because the script store holds on to it). I simulate a reload in this test by calling reload (which clears and readds all debuggees), and then reloading the same source.

Finally, this patch also adds a isPending field to response packet for setBreakpoint, which replaces the noCodeAtLocation error
Attachment #8584446 - Flags: review?(jlong)
This patch tests basic breakpoint sliding for a line breakpoint in a non-sourcemapped source.
Attachment #8584447 - Flags: review?(jlong)
This patch tests if basic breakpoint sliding still works if the corresponding script has been gc'd. When the script is reintroduced due to a reload, the breakpoint should slide to the next closest line.
Attachment #8584451 - Flags: review?(jlong)
This patch tests how breakpoint sliding behaves for line breakpoints when they are set at the end of a function. Right now, the correct behavior is that we slide to the next closest line that has at least one offset, even if that line is not within the same function: this behavior might change when bug 1148356 lands. I've added a comment to that affect in the test.
Attachment #8584452 - Flags: review?(jlong)
This patch tests whether line breakpoints behave correctly with respect to lines with multiple offsets. In this particular case, a breakpoint on a for statement should be hit twice, once for loop initialization, and once for loop iteration.
Attachment #8584454 - Flags: review?(jlong)
This patch tests that a line breakpoint set on a line with multiple statements is only hit once (even though each var statement has an offset, those offsets are not entry points, since they can only be reached from the same line. Therefore, they should not trigger the breakpoint again).
Attachment #8584455 - Flags: review?(jlong)
This patch tests if setting a basic column breakpoint works as expected (the breakpoint should hit in the middle of the line, not at the beginning).
Attachment #8584457 - Flags: review?(jlong)
This patch tests that setting a column breakpoint in a gcd script results in a pending breakpoint that becomes active after a reload.
Attachment #8584459 - Flags: review?(jlong)
This patch tests basic breakpoint sliding for column breakpoints (the breakpoint should slide to the next closest column, not the next closest line).
Attachment #8584461 - Flags: review?(jlong)
This patch tests whether breakpoint sliding for column breakpoints still behaves correctly after reload if the corresponding script was gcd when the breakpoint was set.
Attachment #8584462 - Flags: review?(jlong)
This patch tests the behavior of breakpoint sliding for column breakpoints if the breakpoint is set at the end of a function. The correct behavior right now is to slide to the next closest column, even if that column is not within the same function.

Note that this behavior won't be fixed by bug 1148356, because we don't have a column count for individual lines in each script.
Attachment #8584464 - Flags: review?(jlong)
Attachment #8584464 - Attachment description: patch → Test setting a breakpoint on a column with no offsets at the end of a script.
This patch tests the behavior of breakpoint sliding for column breakpoints when the breakpoint is set at the end of a line (but not the end of a script). In this case, the breakpoint should not slide to the next line, but remain pending (as we discussed on irc, conceptually the breakpoint changes to column Infinity, just like line breakpoint slide to line Infinity and stay pending when you set them at the end of your source).
Attachment #8584465 - Flags: review?(jlong)
What are you going to do about the other test_breakpoint* tests? Is the plan to eventually remove a bunch of them?
Comment on attachment 8584445 [details] [diff] [review]
Test setting a breakpoint on a line

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

::: toolkit/devtools/server/tests/unit/test_setBreakpoint-on-line.js
@@ +19,5 @@
> +    let tab = findTab(tabs, "test");
> +    let [, tabClient] = yield attachTab(client, tab);
> +
> +    let [, threadClient] = yield attachThread(tabClient);
> +    yield resume(threadClient);

I definitely think that lines 9-23 need to be wrapped up in a helper function soon. I like using raw APIs but I don't really care to see the detailed steps of initialization and every test needs to do, and if we ever want to tweak it we can change it in one place. You can land these patches but I'd love to see this refactored out.
Attachment #8584445 - Flags: review?(jlong) → review+
Comment on attachment 8584446 [details] [diff] [review]
Test setting a breakpoint on a line in a gcd script.

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

::: toolkit/devtools/server/tests/unit/test_setBreakpoint-on-line.js
@@ +28,5 @@
>      let sourceClient = threadClient.source(source);
>  
>      let location = { line: 5 };
>      let [packet, breakpointClient] = yield setBreakpoint(sourceClient, location);
> +    do_check_false(packet.isPending);

I like this!
Attachment #8584446 - Flags: review?(jlong) → review+
Attachment #8584447 - Flags: review?(jlong) → review+
Attachment #8584451 - Flags: review?(jlong) → review+
Attachment #8584452 - Flags: review?(jlong) → review+
Attachment #8584454 - Flags: review?(jlong) → review+
You attached the wrong patch for "Test setting a breakpoint on a line with multiple statements"
Attachment #8584457 - Flags: review?(jlong) → review+
Comment on attachment 8584459 [details] [diff] [review]
Test setting a breakpoint on a column in a gcd script

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

::: toolkit/devtools/server/tests/unit/test_setBreakpoint-on-column-in-gcd-script.js
@@ +27,5 @@
> +    let sourceClient = threadClient.source(source);
> +
> +    let location = { line: 6, column: 17 };
> +    let [packet, breakpointClient] = yield setBreakpoint(sourceClient, location);
> +    do_check_true(packet.isPending);

How is this deterministic? The script *could* still be around, right? How do we make sure the GC has been run? I know there's another GC test I could have asked this on, but just thought of it.
*NEW*! Now with 100% more correct patch!
Attachment #8584455 - Attachment is obsolete: true
Attachment #8584455 - Flags: review?(jlong)
Attachment #8585509 - Flags: review?(jlong)
(In reply to James Long (:jlongster) from comment #23)
> Comment on attachment 8584459 [details] [diff] [review]
> Test setting a breakpoint on a column in a gcd script
> 
> Review of attachment 8584459 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> toolkit/devtools/server/tests/unit/test_setBreakpoint-on-column-in-gcd-
> script.js
> @@ +27,5 @@
> > +    let sourceClient = threadClient.source(source);
> > +
> > +    let location = { line: 6, column: 17 };
> > +    let [packet, breakpointClient] = yield setBreakpoint(sourceClient, location);
> > +    do_check_true(packet.isPending);
> 
> How is this deterministic? The script *could* still be around, right? How do
> we make sure the GC has been run? I know there's another GC test I could
> have asked this on, but just thought of it.

Note the call to Cu.forceGC(). That's what makes this deterministic.
Try push for the line tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=481933bee578

James, do you think you could quickly review the last line test, so I can land them all in one go? I can't land the column tests yet because bug 1129784 is still blocked on bug 1099209.
Flags: needinfo?(jlong)
Comment on attachment 8585509 [details] [diff] [review]
Test setting a breakpoint on a line with multiple statements

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

looks good
Attachment #8585509 - Flags: review?(jlong) → review+
Whiteboard: [leave-open]
Comment on attachment 8584459 [details] [diff] [review]
Test setting a breakpoint on a column in a gcd script

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

::: toolkit/devtools/server/tests/unit/setBreakpoint-on-column-in-gcd-script.js
@@ +1,3 @@
> +"use strict";
> +
> +function f() {}

Is this function here for a reason?

::: toolkit/devtools/server/tests/unit/test_setBreakpoint-on-column-in-gcd-script.js
@@ +7,5 @@
> +    do_test_pending();
> +
> +    let global = testGlobal("test");
> +    loadSubScript(SOURCE_URL, global);
> +    Cu.forceGC();

Don't forget to call this 3 times like mentioned on IRC.
Attachment #8584459 - Flags: review?(jlong) → review+
Attachment #8584461 - Flags: review?(jlong) → review+
Comment on attachment 8584462 [details] [diff] [review]
Test setting a breakpoint on a column with no offsets in a gcd script

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

This patch still includes an old file "test_setBreakpoint-on-column-with-no-offests-in-gcd-script.js" which needs to be removed
Attachment #8584462 - Flags: review?(jlong) → review+
Attachment #8584464 - Flags: review?(jlong) → review+
Comment on attachment 8584465 [details] [diff] [review]
Test setting a breakpoint on a column with no offsets at the end of a line.

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

::: toolkit/devtools/server/tests/unit/test_setBreakpoint-on-column-with-no-offsets-at-end-of-line.js
@@ +30,5 @@
> +    let [packet, breakpointClient] = yield setBreakpoint(sourceClient, location);
> +    do_check_true(packet.isPending);
> +    do_check_false("actualLocation" in packet);
> +
> +    Cu.evalInSandbox("f()", global);

should we somehow test that this completes or does `close` do that?
Attachment #8584465 - Flags: review?(jlong) → review+
(In reply to James Long (:jlongster) from comment #30)
> Comment on attachment 8584459 [details] [diff] [review]
> Test setting a breakpoint on a column in a gcd script
> 
> Review of attachment 8584459 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> toolkit/devtools/server/tests/unit/setBreakpoint-on-column-in-gcd-script.js
> @@ +1,3 @@
> > +"use strict";
> > +
> > +function f() {}
> 
> Is this function here for a reason?
> 

Yes, if all functions are garbage collected, then the source is garbage collected as well, so we need to keep at least one function around.
Try push for the column breakpoint tests looks good, landing on fx-team (I've landed them all as a single commit, I hope that's ok):
https://hg.mozilla.org/integration/fx-team/rev/b8c3c063b3ab
This patch refactors the tests to use shorter names, as discussed on irc.
Attachment #8587935 - Flags: review?(jlong)
For the sourcemapped tests, we can't easily put the sourcemap in its own file, without making it hard to both read and change. It's much more convenient to just write sourcemapped sources inline, using SourceNode.

This patch refactors the non-sourcemapped tests to use inline sources as well, for the sake of consistency.
Attachment #8587937 - Flags: review?(jlong)
This patch implements a helper function for the initializing the server and clients for these tests, as you suggested.
Attachment #8587940 - Flags: review?(jlong)
This patch tests setting a breakpoint on a line with no entry points at the end of a source. The purpose of this test is mainly to check that breakpoint sliding fails gracefully, i.e. does not keep sliding down forever.
Attachment #8587941 - Flags: review?(jlong)
This patch adds some improvements to the existing breakpoint tests, such as checking what happens when an existing breakpoint is present, and whether we actually return from the call to evalInSandbox (if we don't, we've hit an unexpected pause).
Attachment #8587943 - Flags: review?(jlong)
Test setting a breakpoint on a sourcemapped line, using a simple source map that slides every line down by one.
Attachment #8587945 - Flags: review?(jlong)
Attachment #8587945 - Attachment description: patch → Test setting a breakpoint on a sourcemapped line
Test setting a breakpoint on a sourcemapped column, using a simple sourcemap that slides the columns on the given line to the right by 2 columns.
Attachment #8587946 - Flags: review?(jlong)
(In reply to Eddy Bruel [:ejpbruel] from comment #34)
> (In reply to James Long (:jlongster) from comment #30)
> > Comment on attachment 8584459 [details] [diff] [review]
> > Test setting a breakpoint on a column in a gcd script
> > 
> > Review of attachment 8584459 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > :::
> > toolkit/devtools/server/tests/unit/setBreakpoint-on-column-in-gcd-script.js
> > @@ +1,3 @@
> > > +"use strict";
> > > +
> > > +function f() {}
> > 
> > Is this function here for a reason?
> > 
> 
> Yes, if all functions are garbage collected, then the source is garbage
> collected as well, so we need to keep at least one function around.

Please document this unfortunate sorcery with a comment.
Comment on attachment 8587935 [details] [diff] [review]
Use shorter names for the tests.

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

Awesome, thanks
Attachment #8587935 - Flags: review?(jlong) → review+
Comment on attachment 8587937 [details] [diff] [review]
Use inline sources

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

I'm fine with this for now, but I like the technique of doing:

var code = function() {
  var x = 5;
  return x * 10;
}.toString();

Although I guess you can't do that because you need to set column breakpoints so it needs to formatted exactly as you wrote it, and I think our `toString` doesn't keep formatting in tact.
Attachment #8587937 - Flags: review?(jlong) → review+
Comment on attachment 8587940 [details] [diff] [review]
Implement a helper function for initializing the server and clients.

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

Yay! Looks good.
Attachment #8587940 - Flags: review?(jlong) → review+
Comment on attachment 8587941 [details] [diff] [review]
Test setting a breakpoint on a line with no entry points at the end of a source

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

::: toolkit/devtools/server/tests/unit/test_breakpoint-line-05.js
@@ +27,5 @@
> +    Cu.evalInSandbox(SOURCE, global, "1.8", SOURCE_URL);
> +    let { source } = yield promise;
> +    let sourceClient = threadClient.source(source);
> +
> +    let location = { line: global.line0 + 8 };

line 8 is one line past the end of the source, did you mean that? what happens if you set a bp on line 100 is the script is only 99 lines long anyway, is the bp just forever pending?
Attachment #8587941 - Flags: review?(jlong) → review+
Comment on attachment 8587942 [details] [diff] [review]
Test setting a breakpoint on a column with no offsets at the end of a source

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

I guess you did mean to set it on one line past the end of the script. So the bp is just forever pending?
Attachment #8587942 - Flags: review?(jlong) → review+
Comment on attachment 8587943 [details] [diff] [review]
Improve the breakpoint tests

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

::: toolkit/devtools/server/tests/unit/test_breakpoint-column-01.js
@@ +36,5 @@
> +    do_check_eq(breakpointClient.actor, existingBreakpointClient.actor);
> +
> +    resume(threadClient);
> +
> +    yield new Promise(function (resolve) {

The promise interaction is really confusing. Why are you creating a promise here? I like the way this looks better before.
Comment on attachment 8587946 [details] [diff] [review]
Test setting a breakpoint on a sourcemapped column

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

The last two tests look fine to me too, but I'll wait to r+ until I hear about why you yield a new promise and such. It'd be nice if there was a way to structure it so it reads like it did before, just a straight-forward linear code path.
(In reply to James Long (:jlongster) from comment #50)
> Comment on attachment 8587943 [details] [diff] [review]
> Improve the breakpoint tests
> 
> Review of attachment 8587943 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/server/tests/unit/test_breakpoint-column-01.js
> @@ +36,5 @@
> > +    do_check_eq(breakpointClient.actor, existingBreakpointClient.actor);
> > +
> > +    resume(threadClient);
> > +
> > +    yield new Promise(function (resolve) {
> 
> The promise interaction is really confusing. Why are you creating a promise
> here? I like the way this looks better before.

The reason I've added this promise is that we want to make sure that the breakpoint is hit exactly once. We can't do that by waiting for the debugger to pause twice, because if the breakpoint works correctly, waiting for a second pause would block the test forever.

Instead, I create a promise that does not get resolved until the call to Cu.evalInSandbox has returned. If there are any pauses, then this call will not return until we've resumed after each pause. By waiting for each expected pause and then resuming, we know that this promise will only resolve if the number of pauses was exactly what we expect (fewer pauses, and the test will block forever waiting for a pause. more pauses, and the test will block forever waiting for Cu.evalInSandbox to return).

I agree that the code is less readable than it used to be, but extending test coverage to check the exact number of pauses is worthwhile imo. We could remedy the lack of readability by adding a comment, or by abstracting this pattern into a helper function somehow. Would you agree to do this in a followup patch?
It looks like we won't be able to land the tests for sourcemapped sources before I leave on PTO, so here's a reminder to myself to what things I want to test:

1. Setting a breakpoint on a sourcemapped line
2. Setting a breakpoint on a sourcemapped column
3. Setting a breakpoint on a sourcemapped line with no mappings

4. Setting a breakpoint on a sourcemapped column with no mappings

5. Setting a breakpoint on a line with no mappings at the end of a sourcemapped source
6. Setting a breakpoint on a column with no mappings at the end of a sourcemapped source
7. Setting a breakpoint on a column with no mappings at the end of a line in a sourcemapped source

8. Setting a breakpoint on a line in a sourcemapped source mapped to a line in a gcd script.
9. Setting a breakpoint on a column in a sourcemapped source mapped to a column in a gcd script

10. Setting a breakpoint on a line in a sourcemapped source mapped to a line with no offsets
11. Setting a breakpoint on a column in a sourcemapped source mapped to a column with no offsets

12. Setting a breakpoint on a line in a sourcemapped source mapped to a line with no offsets in a gcd script
13. Setting a breakpoint on a column in a sourcemapped source mapped to a column with no offsets in a gcd script

14. Setting a breakpoint on a line at the end of  a sourcemapped source mapped to a line with no offsets 
15. Setting a breakpoint on a column at the end of a sourcemapped source mapped to a column with no offsets
16. Setting a breakpoint on a column at the end of a line in a sourcemapped source mapped to a column with no offsets

17. Setting a breakpoint on a line in a sourcemapped source mapped to multiple lines
18. Setting a breakpoint on a line in a sourcemapped source mapped to multiple columns
19. Setting a breakpoint on a column in a sourcemapped source mapped to multiple columns
20. Setting a breakpoint on multiple lines in a sourcemapped source mapped to the same line
Try run for everything up to (but not including) improving the breakpoint tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=290c24e05033
Comment on attachment 8587943 [details] [diff] [review]
Improve the breakpoint tests

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

Ok, yeah you can do it in a followup. It would be nice to see how we can improve it, because a new person reading this test would not be able to infer why its done this way.
Attachment #8587943 - Flags: review?(jlong) → review+
Attachment #8587945 - Flags: review?(jlong) → review+
Attachment #8587946 - Flags: review?(jlong) → review+
Can we please nominate and uplift whatever patches fixed bug 1035678 on trunk for Aurora ASAP? It's been a top-failure on that branch for two weeks now.
Flags: needinfo?(ejpbruel)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #57)
> Can we please nominate and uplift whatever patches fixed bug 1035678 on
> trunk for Aurora ASAP? It's been a top-failure on that branch for two weeks
> now.

Eddy is on PTO this week unfortunately and I'm not familiar enough with these patches to know exactly which one fixed it. I think we'll have to wait until next Monday.
Comment on attachment 8584445 [details] [diff] [review]
Test setting a breakpoint on a line

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

::: toolkit/devtools/server/tests/unit/head_dbg.js
@@ -15,5 @@
>  
>  const Services = devtools.require("Services");
> -// Always log packets when running tests. runxpcshelltests.py will throw
> -// the output away anyway, unless you give it the --verbose flag.
> -Services.prefs.setBoolPref("devtools.debugger.log", true);

Gar! It looks like we landed it like this?

I was pretty sure we agreed on IRC that if we were going to disable this logging by default, we would leave the pref here, but set it `false`. If we leave the pref setting, it is easy to enable again. The reasoning behind "use the default pref" is faulty because tests are run with their own profile, and we can't set our own defaults there.

I wasted tons of time yesterday trying to debug failing xpcshell tests, then tons more time trying to figure out why the RDP logs weren't showing up...

Really, I think this logging should be on by default. It is *very* hard to debug xpcshell tests without it...
I guess it did land; if there's a lot of value leaving it on, let's just do that. I don't really mind either way, I turn it off locally but I agree it's helpful when running on try. Go ahead and put it back in, I don't think we need to create a new bug for it. I would do it but I don't have direct commit access.
Comment on attachment 8584446 [details] [diff] [review]
Test setting a breakpoint on a line in a gcd script.

Approval Request Comment
[Feature/regressing bug #]:1035678
[User impact if declined]:This patch should fix the intermittent test failure in bug 1035678
[Describe test coverage new/current, TreeHerder]:Both the test included in the bug and the one in bug 1035678.
[Risks and why]:Virtually no risk. The actual fix is a one liner. The rest of the patch just adds test coverage.
[String/UUID change made/needed]:
Flags: needinfo?(ejpbruel)
Attachment #8584446 - Flags: approval-mozilla-aurora?
Comment on attachment 8584446 [details] [diff] [review]
Test setting a breakpoint on a line in a gcd script.

Approving this patch for uplift to aurora to help with intermittent test failure in bug 1035678.
Attachment #8584446 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This needed some rebasing in order to be landed on its own. We'll see how it goes :)
https://hg.mozilla.org/releases/mozilla-aurora/rev/923b536f3d2d
Assignee: ejpbruel → nobody
Product: Firefox → DevTools
Lot of things changes in the devtools, we can close it now.
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Whiteboard: [leave-open]
Assignee: nobody → ejpbruel
You need to log in before you can comment on or make changes to this bug.