870939 - add tests for various bits of nsITelemetry

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: part 1 - add tests for nsITelemetry.lateWrites

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 2 - add tests for nsITelemetry.failedProfileLockCount

Comment 3

[Vladan Djeric (:vladan)]

Aren't there already tests for failed profile lock count?

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/tests/unit/test_TelemetryPing.js#194

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Vladan Djeric (:vladan) from comment #3)
> Aren't there already tests for failed profile lock count?
> 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/
> tests/unit/test_TelemetryPing.js#194

Hm, so there are.

I think this test is still valuable, because it's more comprehensive than the TelemetryPing test.  test_TelemetryPing is more like an integration test; the individual unit tests are still valuable.  And because test_TelemetryPing.js shouldn't be the sole source of truth for all Telemetry bits.  As much as possible should be separated out into finer-grained tests so that our main source of tests isn't some 1000-line test file that requires async HTTP requests for testing.

Comment 5

[Vladan Djeric (:vladan)]

Comment on attachment 748140 [details] [diff] [review]
part 2 - add tests for nsITelemetry.failedProfileLockCount

Remove Rafael's profileLockedCount test so we're not wasting test resources

Comment 6

[Vladan Djeric (:vladan)]

Comment on attachment 748139 [details] [diff] [review]
part 1 - add tests for nsITelemetry.lateWrites

>+  for (let library of lateWrites.memoryMap) {
>+    let name = library[0]
>+    let breakpadID = library[1];
>+    do_check_true(breakpadID in LOADED_MODULES);
>+    do_check_eq(LOADED_MODULES[breakpadID], name);
>+  }

Why not iterate over LOADED_MODULES instead? Wouldn't the code above pass if the same module was repeated in memoryMap 3 times?

>+  let uneval_STACKS = [uneval(STACK1), uneval(STACK2)];
>+  let first_stack = lateWrites.stacks[0];
>+  let second_stack = lateWrites.stacks[1];
>+  function stackChecker(canonicalStack) {
>+    let unevalCanonicalStack = uneval(canonicalStack);
>+    return function(obj, idx, array) {
>+      return unevalCanonicalStack == obj;
>+    }
>+  }
>+  do_check_eq(uneval_STACKS.filter(stackChecker(first_stack)).length, 1);
>+  do_check_eq(uneval_STACKS.filter(stackChecker(second_stack)).length, 1);

Couldn't you just test lateWrites.stacks[0].toString() == STACK1.toString()?

Comment 7

[Nathan Froyd [:froydnj]]

(In reply to Vladan Djeric (:vladan) from comment #6)
> Comment on attachment 748139 [details] [diff] [review]
> part 1 - add tests for nsITelemetry.lateWrites
> 
> >+  for (let library of lateWrites.memoryMap) {
> >+    let name = library[0]
> >+    let breakpadID = library[1];
> >+    do_check_true(breakpadID in LOADED_MODULES);
> >+    do_check_eq(LOADED_MODULES[breakpadID], name);
> >+  }
> 
> Why not iterate over LOADED_MODULES instead? Wouldn't the code above pass if
> the same module was repeated in memoryMap 3 times?

You could do that, but you want to make sure some other module didn't sneak into lateWrites.  I suppose I should add some check in the opposite direction to guarantee uniqueness or something.

> >+  let uneval_STACKS = [uneval(STACK1), uneval(STACK2)];
> >+  let first_stack = lateWrites.stacks[0];
> >+  let second_stack = lateWrites.stacks[1];
> >+  function stackChecker(canonicalStack) {
> >+    let unevalCanonicalStack = uneval(canonicalStack);
> >+    return function(obj, idx, array) {
> >+      return unevalCanonicalStack == obj;
> >+    }
> >+  }
> >+  do_check_eq(uneval_STACKS.filter(stackChecker(first_stack)).length, 1);
> >+  do_check_eq(uneval_STACKS.filter(stackChecker(second_stack)).length, 1);
> 
> Couldn't you just test lateWrites.stacks[0].toString() == STACK1.toString()?

No.  You don't know what order the stacks are in.

Comment 8

[Vladan Djeric (:vladan)]

> You could do that, but you want to make sure some other module didn't sneak
> into lateWrites.  I suppose I should add some check in the opposite
> direction to guarantee uniqueness or something.

Ah but you have your "do_check_eq(lateWrites.memoryMap.length, N_MODULES)" check

> > Couldn't you just test lateWrites.stacks[0].toString() == STACK1.toString()?
> 
> No.  You don't know what order the stacks are in.

Oic. You could sort the stringified stacks before comparing individual stacks, but that's getting pretty complex too

Comment 9

[Nathan Froyd [:froydnj]]

Attachment: part 1 - add tests for nsITelemetry.lateWrites

With iterating over LOADED_MODULES.  Not quite so elegant as you might
assume, since we can't assume anything about the ordering of the memory maps.

Comment 10

[Vladan Djeric (:vladan)]

Comment on attachment 748804 [details] [diff] [review]
part 1 - add tests for nsITelemetry.lateWrites

Don't forget to remove Rafael's version of profileLockedCount test

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b3ffcaef3463
https://hg.mozilla.org/mozilla-central/rev/e05b8147a276