Closed Bug 870939 Opened 7 years ago Closed 7 years ago

add tests for various bits of nsITelemetry

Categories

(Toolkit :: Telemetry, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: froydnj, Unassigned)

Details

Attachments

(2 files, 1 obsolete file)

No description provided.
(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 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
Attachment #748140 - Flags: review?(vdjeric) → review+
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()?
Attachment #748139 - Flags: review?(vdjeric)
(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.
> 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
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.
Attachment #748139 - Attachment is obsolete: true
Attachment #748804 - Flags: review?(vdjeric)
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
Attachment #748804 - Flags: review?(vdjeric) → review+
https://hg.mozilla.org/mozilla-central/rev/b3ffcaef3463
https://hg.mozilla.org/mozilla-central/rev/e05b8147a276
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.