Closed Bug 1447455 Opened 6 years ago Closed 6 years ago

Add tests to the AutoCounter and RuntimeAutoCounter

Categories

(Toolkit :: Telemetry, enhancement, P1)

Product:
Toolkit
Component:
Telemetry
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: o0ignition0o, Assigned: o0ignition0o)

References

Details

Attachments

(1 file)

Bug 1447455 - Add tests to AutoCounter and RuntimeAutoCounter.
59 bytes, text/x-review-board-request
chutten
: review+
Details 
Followup to Bug 1444735, We could add tests to the AutoCounter and RuntimeAutoCounter classes in Telemetry.h
Comment on attachment 8960885 [details]
Bug 1447455 - Add tests to AutoCounter and RuntimeAutoCounter.

https://reviewboard.mozilla.org/r/229644/#review235522

These tests look good! Couple of minor things.

::: toolkit/components/telemetry/tests/gtest/TestCounters.cpp:29
(Diff revision 1)
> +                       false);
> +
> +  // Accumulate in the histogram
> +  {
> +    Telemetry::AutoCounter<Telemetry::TELEMETRY_TEST_COUNT> autoCounter;
> +    autoCounter += kExpectedValue;

could you snapshot inside the scope and assert that the count doesn't get accumulated until the scope exits?

::: toolkit/components/telemetry/tests/gtest/TestCounters.cpp:57
(Diff revision 1)
> +  const uint32_t kExpectedValue = 0;
> +  AutoJSContextWithGlobal cx(mCleanGlobal);
> +
> +  const char* telemetryTestCountName = Telemetry::GetHistogramName(Telemetry::TELEMETRY_TEST_COUNT);
> +
> +  GetAndClearHistogram(cx.GetJSContext(), mTelemetry, NS_LITERAL_CSTRING("TELEMETRY_TEST_COUNT"),

Presumably you could use telemetryTestCountName in GetAndClearHistogram?
Attachment #8960885 - Flags: review?(chutten)
const char* telemetryTestCountName = "TELEMETRY_TEST_COUNT";
NS_NAMED_LITERAL_CSTRING(cTelemetryTestCountName, "TELEMETRY_TEST_COUNT");

I'm having troubles to cast from char* to NS_LITERAL_CSTRING or vice-versa. (I think the latter might be allowed?)

Any thoughts ?
Comment on attachment 8960885 [details]
Bug 1447455 - Add tests to AutoCounter and RuntimeAutoCounter.

https://reviewboard.mozilla.org/r/229644/#review235594

Oh, I'm sorry, I misinterpreted the macro. It is a macro to create an nsCString, not an NS macro to create a "c string".

You'd think I'd have this figured out by now :S

This looks good, thank you!
Attachment #8960885 - Flags: review?(chutten) → review+
Aw ok, I think I might found a way to do it though :p 

I'm going to rollback my CSTRING wizardry for readability's sake, and I'll submit again :)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/566262c83ec7
Add tests to AutoCounter and RuntimeAutoCounter. r=chutten
Keywords: checkin-needed
Backout by dluca@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7502aa54dce3
Backed out changeset 566262c83ec7 for build bustages. CLOSED TREE
Any idea what happened here?
Flags: needinfo?(jeremy.lempereur)
Hi,

Yeah, I just rebased my work and noticed I was trying to do something I'm was not supposed to (two snapshots).

I'm going to create 2 counters and do only one snapshot eg : 

``` 
// Accumulate in the histogram
  {
    auto autoCounter = Telemetry::RuntimeAutoCounter(Telemetry::TELEMETRY_TEST_COUNT);
    autoCounter += kExpectedValue;
  }
  // Create an other Counter which will not go out of scope
  auto autoCounter = Telemetry::RuntimeAutoCounter(Telemetry::TELEMETRY_TEST_COUNT);
  autoCounter += kExpectedValue;


  // Get a snapshot for all the histograms
  JS::RootedValue snapshot(cx.GetJSContext());

[...]
 ASSERT_EQ(uSum, kExpectedValue);
```

I'll try to push it for review tonight :)
Flags: needinfo?(jeremy.lempereur)
Keywords: checkin-needed
Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/844938a9fc27
Add tests to AutoCounter and RuntimeAutoCounter. r=chutten
Keywords: checkin-needed
Backed out for bustages on TestCounters.cpp

URL of the backout:
https://hg.mozilla.org/integration/autoland/rev/38a6f16e53b0e6c38ba5deb87054fd7341ed0ebf

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=844938a9fc278e5fc753bec9301381517715881d&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&filter-resultStatus=pending&filter-resultStatus=running&selectedJob=169835333

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=169835333&repo=autoland&lineNumber=17565

/builds/worker/workspace/build/src/toolkit/components/telemetry/tests/gtest/TestCounters.cpp:104:24: note: value incorrectly allocated in a temporary
[task 2018-03-23T10:40:53.070Z] 10:40:53     INFO -  /builds/worker/workspace/build/src/toolkit/components/telemetry/tests/gtest/TestCounters.cpp:108:22: error: variable of type 'Telemetry::RuntimeAutoCounter' is not valid in a temporary
[task 2018-03-23T10:40:53.070Z] 10:40:53     INFO -    auto autoCounter = Telemetry::RuntimeAutoCounter(Telemetry::TELEMETRY_TEST_COUNT);
[task 2018-03-23T10:40:53.071Z] 10:40:53     INFO -                       ^
[task 2018-03-23T10:40:53.071Z] 10:40:53     INFO -  /builds/worker/workspace/build/src/toolkit/components/telemetry/tests/gtest/TestCounters.cpp:108:22: note: value incorrectly allocated in a temporary
[task 2018-03-23T10:40:53.072Z] 10:40:53     INFO -  /builds/worker/workspace/build/src/toolkit/components/telemetry/tests/gtest/TestCounters.cpp:112:24: error: variable of type 'Telemetry::RuntimeAutoCounter' is not valid in a temporary
[task 2018-03-23T10:40:53.072Z] 10:40:53     INFO -      auto autoCounter = Telemetry::RuntimeAutoCounter(Telemetry::TELEMETRY_TEST_COUNT);
[task 2018-03-23T10:40:53.072Z] 10:40:53     INFO -                         ^
[task 2018-03-23T10:40:53.072Z] 10:40:53     INFO -  /builds/worker/workspace/build/src/toolkit/components/telemetry/tests/gtest/TestCounters.cpp:112:24: note: value incorrectly allocated in a temporary
[task 2018-03-23T10:40:53.072Z] 10:40:53     INFO -  /builds/worker/workspace/build/src/toolkit/components/telemetry/tests/gtest/TestCounters.cpp:145:24: error: variable of type 'Telemetry::RuntimeAutoCounter' is not valid in a temporary
[task 2018-03-23T10:40:53.073Z] 10:40:53     INFO -      auto autoCounter = Telemetry::RuntimeAutoCounter(Telemetry::TELEMETRY_TEST_COUNT, kExpectedValue);
[task 2018-03-23T10:40:53.073Z] 10:40:53     INFO -                         ^
[task 2018-03-23T10:40:53.073Z] 10:40:53     INFO -  /builds/worker/workspace/build/src/toolkit/components/telemetry/tests/gtest/TestCounters.cpp:145:24: note: value incorrectly allocated in a temporary
[task 2018-03-23T10:40:53.073Z] 10:40:53     INFO -  4 errors generated.
[task 2018-03-23T10:40:53.073Z] 10:40:53     INFO -  /builds/worker/workspace/build/src/config/rules.mk:1025: recipe for target 'Unified_cpp_tests_gtest0.o' failed
[task 2018-03-23T10:40:53.073Z] 10:40:53     INFO -  make[4]: *** [Unified_cpp_tests_gtest0.o] Error 1
[task 2018-03-23T10:40:53.073Z] 10:40:53     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/toolkit/components/telemetry/tests/gtest'
[task 2018-03-23T10:40:53.074Z] 10:40:53     INFO -  /builds/worker/workspace/build/src/config/recurse.mk:73: recipe for target 'toolkit/components/telemetry/tests/gtest/target' failed
[task 2018-03-23T10:40:53.074Z] 10:40:53     INFO -  make[3]: *** [toolkit/components/telemetry/tests/gtest/target] Error 2
[task 2018-03-23T10:40:53.074Z] 10:40:53     INFO -  make[3]: *** Waiting for unfinished jobs....
[task 2018-03-23T10:40:53.075Z] 10:40:53     INFO -  make[4]: Entering directory '/builds/worker/work
Flags: needinfo?(jeremy.lempereur)
Ok there must be something else wrong, but I cannot reproduce it :/ 

Is there any flag I'm supposed to switch on to get the same error ? 
I think it might have to do with the MOZ_RAAI setting ?
I'm kind of confused :/
Flags: needinfo?(jeremy.lempereur) → needinfo?(chutten)
I think it's because I'm not supposed to use the keyword auto as in 
`auto autoCounter = Telemetry::RuntimeAutoCounter(Telemetry::TELEMETRY_TEST_COUNT, kExpectedValue); `
I need to first make sure I can build with the right flags enabled before I submit again.
Ok your blog post just saved me https://chuttenblog.wordpress.com/tag/raii/

Thanks for helping me out even when you're not there :D
Keywords: checkin-needed
As we chatted about on IRC, what you ran across was a static analysis failure. Static analysis is a build mode on Linux and MacOS to catch things like this. We have some documentation on MDN about how to run it locally: https://developer.mozilla.org/en-US/docs/Mozilla/Testing/Clang_static_analysis

I'm glad my blog post was helpful. That's one of the reasons I write them :)
Flags: needinfo?(chutten)
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/772245fb2926
Add tests to AutoCounter and RuntimeAutoCounter. r=chutten
Keywords: checkin-needed
Status: NEW → ASSIGNED
Priority: -- → P1
https://hg.mozilla.org/mozilla-central/rev/772245fb2926
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: