1147972 - Write tests for the breakpoint code.

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Eddy Bruel [:ejpbruel]]

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.

Comment 1

[Eddy Bruel [:ejpbruel]]

Attachment: setBreakpoint-on-line

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.

Comment 2

[Eddy Bruel [:ejpbruel]]

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 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 4

[Eddy Bruel [:ejpbruel]]

(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 5

[James Long (:jlongster)]

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.

Comment 6

[Eddy Bruel [:ejpbruel]]

Attachment: Test setting a breakpoint on a line

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?

Comment 7

[Eddy Bruel [:ejpbruel]]

Attachment: Test setting a breakpoint on a line in a gcd script.

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

Comment 8

[Eddy Bruel [:ejpbruel]]

Attachment: Test setting a breakpoint on a line with no offsets

This patch tests basic breakpoint sliding for a line breakpoint in a non-sourcemapped source.

Comment 9

[Eddy Bruel [:ejpbruel]]

Attachment: Test setting a breakpoint on a line with no offsets in a gcd script.

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.

Comment 10

[Eddy Bruel [:ejpbruel]]

Attachment: Test setting a breakpoint on a line with no offsets at the end of a script

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.

Comment 11

[Eddy Bruel [:ejpbruel]]

Attachment: Test setting a breakpoint on a line with multiple offsets

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.

Comment 12

[Eddy Bruel [:ejpbruel]]

Attachment: Test setting a breakpoint on a line with multiple statements

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).

Comment 13

[Eddy Bruel [:ejpbruel]]

Attachment: Test setting a breakpoint on a column

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).

Comment 14

[Eddy Bruel [:ejpbruel]]

Attachment: Test setting a breakpoint on a column in a gcd script

This patch tests that setting a column breakpoint in a gcd script results in a pending breakpoint that becomes active after a reload.

Comment 15

[Eddy Bruel [:ejpbruel]]

Attachment: Test setting a breakpoint on a column with no offsets

This patch tests basic breakpoint sliding for column breakpoints (the breakpoint should slide to the next closest column, not the next closest line).

Comment 16

[Eddy Bruel [:ejpbruel]]

Attachment: Test setting a breakpoint on a column with no offsets in a gcd script

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.

Comment 17

[Eddy Bruel [:ejpbruel]]

Attachment: 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 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.

Comment 18

[Eddy Bruel [:ejpbruel]]

Attachment: Test setting a breakpoint on a column with no offsets at the end of a line.

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).

Comment 19

[James Long (:jlongster)]

What are you going to do about the other test_breakpoint* tests? Is the plan to eventually remove a bunch of them?

Comment 20

[James Long (:jlongster)]

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.

Comment 21

[James Long (:jlongster)]

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!

Comment 22

[James Long (:jlongster)]

You attached the wrong patch for "Test setting a breakpoint on a line with multiple statements"

Comment 23

[James Long (:jlongster)]

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.

Comment 24

[Eddy Bruel [:ejpbruel]]

Attachment: Test setting a breakpoint on a line with multiple statements

*NEW*! Now with 100% more correct patch!

Comment 25

[Eddy Bruel [:ejpbruel]]

(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.

Comment 26

[Eddy Bruel [:ejpbruel]]

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.

Comment 27

[James Long (:jlongster)]

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

Comment 28

[Eddy Bruel [:ejpbruel]]

Try push for the line tests looks good, landing on fx-team:
https://hg.mozilla.org/integration/fx-team/rev/a9d3e66e2e77
https://hg.mozilla.org/integration/fx-team/rev/92c3c1e2717c
https://hg.mozilla.org/integration/fx-team/rev/fd70ecaacf85
https://hg.mozilla.org/integration/fx-team/rev/3e0964957e23
https://hg.mozilla.org/integration/fx-team/rev/6742ddf66832
https://hg.mozilla.org/integration/fx-team/rev/983deab6cb0f
https://hg.mozilla.org/integration/fx-team/rev/d9b8b80e9039

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a9d3e66e2e77
https://hg.mozilla.org/mozilla-central/rev/92c3c1e2717c
https://hg.mozilla.org/mozilla-central/rev/fd70ecaacf85
https://hg.mozilla.org/mozilla-central/rev/3e0964957e23
https://hg.mozilla.org/mozilla-central/rev/6742ddf66832
https://hg.mozilla.org/mozilla-central/rev/983deab6cb0f
https://hg.mozilla.org/mozilla-central/rev/d9b8b80e9039

Comment 30

[James Long (:jlongster)]

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.

Comment 31

[James Long (:jlongster)]

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

Comment 32

[James Long (:jlongster)]

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?

Comment 33

[Eddy Bruel [:ejpbruel]]

Try run for the column tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca0d7ed664ae

Comment 34

[Eddy Bruel [:ejpbruel]]

(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.

Comment 35

[Eddy Bruel [:ejpbruel]]

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

Comment 36

[Eddy Bruel [:ejpbruel]]

Attachment: Use shorter names for the tests.

This patch refactors the tests to use shorter names, as discussed on irc.

Comment 37

[Eddy Bruel [:ejpbruel]]

Attachment: Use inline sources

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.

Comment 38

[Eddy Bruel [:ejpbruel]]

Attachment: Implement a helper function for initializing the server and clients.

This patch implements a helper function for the initializing the server and clients for these tests, as you suggested.

Comment 39

[Eddy Bruel [:ejpbruel]]

Attachment: Test setting a breakpoint on a line with no entry points at the end of a source

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.

Comment 40

[Eddy Bruel [:ejpbruel]]

Attachment: Test setting a breakpoint on a column with no offsets at the end of a source

Comment 41

[Eddy Bruel [:ejpbruel]]

Attachment: Improve the breakpoint tests

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).

Comment 42

[Eddy Bruel [:ejpbruel]]

Attachment: Test setting a breakpoint on a sourcemapped line

Test setting a breakpoint on a sourcemapped line, using a simple source map that slides every line down by one.

Comment 43

[Eddy Bruel [:ejpbruel]]

Attachment: Test setting a breakpoint on a sourcemapped column

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.

Comment 44

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(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 45

[James Long (:jlongster)]

Comment on attachment 8587935 [details] [diff] [review]
Use shorter names for the tests.

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

Awesome, thanks

Comment 46

[James Long (:jlongster)]

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.

Comment 47

[James Long (:jlongster)]

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.

Comment 48

[James Long (:jlongster)]

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?

Comment 49

[James Long (:jlongster)]

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?

Comment 50

[James Long (:jlongster)]

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 51

[James Long (:jlongster)]

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.

Comment 52

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b8c3c063b3ab

Comment 53

[Eddy Bruel [:ejpbruel]]

(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?

Comment 54

[Eddy Bruel [:ejpbruel]]

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

Comment 55

[Eddy Bruel [:ejpbruel]]

Try run for everything up to (but not including) improving the breakpoint tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=290c24e05033

Comment 56

[James Long (:jlongster)]

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.

Comment 57

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 58

[James Long (:jlongster)]

(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 59

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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...

Comment 60

[James Long (:jlongster)]

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 61

[Eddy Bruel [:ejpbruel]]

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]:

Comment 62

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 63

[Ryan VanderMeulen [:RyanVM]]

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

Comment 64

[Sylvestre Ledru [:Sylvestre]]

Lot of things changes in the devtools, we can close it now.