899215 - test_breakpointstore.js isn't testing anything

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

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

We moved ThreadActor._breakpointStore to ThreadActor.breakpointStore and the test hasn't been updated, but still passes. We should fix it.

Comment 1

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

Attachment: v1

* Fixes the existing test in the file to make sure that ThreadActor.breakpointStore is shared across all thread actors

* Add a bunch of tests for the BreakpointStore

https://tbpl.mozilla.org/?tree=Try&rev=e67c05929ae1

Comment 2

[Jim Blandy :jimb]

Comment on attachment 782755 [details] [diff] [review]
v1

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

Looks good; some suggestions for tightening up the tests; see what you think. r=me either way.

::: toolkit/devtools/server/tests/unit/test_breakpointstore.js
@@ +25,5 @@
>    let instance2 = new ThreadActor();
> +  do_check_true(instance1.breakpointStore instanceof BreakpointStore);
> +  do_check_eq(instance1.breakpointStore, ThreadActor.breakpointStore);
> +  do_check_eq(instance2.breakpointStore, ThreadActor.breakpointStore);
> +  do_check_eq(instance1.breakpointStore, instance2.breakpointStore);

You don't trust == to be transitive? :)

@@ +71,5 @@
> +  };
> +  bpStore.addBreakpoint(location);
> +  bpStore.removeBreakpoint(location);
> +  do_check_eq(bpStore.getBreakpoint(location, false), null,
> +              "We should npt have the whole line breakpoint anymore");

"not"

@@ +99,5 @@
> +  for (let bp of bpStore.findBreakpoints()) {
> +    numBpsFound++;
> +  }
> +  do_check_eq(numBpsFound, bps.length,
> +              "Should be able to iterate over all breakpoints");

Or, since it's part of the BreakpointStore API that we're actually handing back the same *object*, not just an equivalent one, you could start with Set(bps), and then assert that each bp location yielded is present in the set and then delete it. That would be a tighter check.

@@ +108,5 @@
> +  for (let bp of bpStore.findBreakpoints({ url: "foo.js" })) {
> +    numBpsFound++
> +  }
> +  do_check_eq(numBpsFound, bps.filter(bp => { return bp.url === "foo.js" }).length,
> +              "Should be able to filter the iteration by url");

And you could do the same thing here with Set(bps.filter(...)).

Comment 3

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

Attachment: v1.1

The patch that landed in fx-team.

https://hg.mozilla.org/integration/fx-team/rev/8911e64ad234

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8911e64ad234