Closed
Bug 1351274
Opened 7 years ago
Closed 7 years ago
Add C++ test coverage for the Categorical Histogram
Categories
(Toolkit :: Telemetry, enhancement, P3)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla58
People
(Reporter: Dexter, Assigned: david, Mentored)
References
Details
(Whiteboard: [measurement:client][lang=c++])
Attachments
(1 file, 4 obsolete files)
6.83 KB,
patch
|
Dexter
:
review+
|
Details | Diff | Splinter Review |
We should add test coverage for the keyed version of the AccumulateCategorical/Accumulate functions [1]. We can use the "TELEMETRY_TEST_CATEGORICAL" for testing. The new test should be added to the TestHistograms.cpp file in toolkit/components/telemetry/tests/gtest and should: - Get a reference to the histogram. - Clear its content. - Store a categorical value - Get a snapshot of the histogram. - Check that the histogram contains the stored values. The code can be tested by using the following command: > ./mach gtest Telemetry* [1] - http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/toolkit/components/telemetry/Telemetry.h#108,124
Reporter | ||
Comment 1•7 years ago
|
||
Federico, would you be interested in taking on this one?
Points: --- → 2
Depends on: 1312698
Flags: needinfo?(federico_padua)
Priority: -- → P3
Whiteboard: [measurement:client][lang=c++]
Comment 2•7 years ago
|
||
(In reply to Alessio Placitelli [:Dexter] from comment #1) > Federico, would you be interested in taking on this one? I can take it and try, but I will only have time during the weekend for that this week...but I'll be happy to do it!
Flags: needinfo?(federico_padua)
Reporter | ||
Comment 3•7 years ago
|
||
(In reply to Federico Padua [:fedepad] from comment #2) > (In reply to Alessio Placitelli [:Dexter] from comment #1) > > Federico, would you be interested in taking on this one? > > I can take it and try, but I will only have time during the weekend for that > this week...but I'll be happy to do it! No rush, take your time! Feel free to refactor the TestHistograms.cpp file as you see fit! http://searchfox.org/mozilla-central/source/toolkit/components/telemetry/tests/gtest/TestHistograms.cpp
Assignee: nobody → federico_padua
Reporter | ||
Comment 4•7 years ago
|
||
Federico, are you still interested in working on this bug?
Flags: needinfo?(federico_padua)
Reporter | ||
Updated•7 years ago
|
Assignee: federico_padua → nobody
Flags: needinfo?(federico_padua)
Comment 5•7 years ago
|
||
Bug 1357682 also adds an overload for AccumulateCategorical() for keyed histograms. We should also cover that.
Assignee | ||
Comment 6•7 years ago
|
||
Alessio: mind if I work on this one?
Reporter | ||
Comment 7•7 years ago
|
||
(In reply to David Krauser from comment #6) > Alessio: mind if I work on this one? Not at all! I've assigned the bug to you, thanks for your interest (sorry for the delay, I was out last week). You can find some information about how to work on this bug in comment 0, let me know if there's any doubt or anything else we should discuss!
Assignee: nobody → david
Flags: needinfo?(david)
Assignee | ||
Comment 8•7 years ago
|
||
Awesome. I'll let you know if I hit any issues :-)
Flags: needinfo?(david)
Reporter | ||
Comment 9•7 years ago
|
||
Hi David, how is this progressing? Are you still working on it? Do you need any help?
Flags: needinfo?(david)
Assignee | ||
Comment 10•7 years ago
|
||
Alessio: Yes, sorry! Don't give up hope, yet - I should have a first pass this week.
Flags: needinfo?(david)
Assignee | ||
Comment 11•7 years ago
|
||
Sorry this took so long. Is this the right approach here?
Flags: needinfo?(alessio.placitelli)
Reporter | ||
Comment 12•7 years ago
|
||
Comment on attachment 8909065 [details] [diff] [review] 1351274.patch Review of attachment 8909065 [details] [diff] [review]: ----------------------------------------------------------------- That's a very good start, thanks David! I now see how the C++ categorical API could use some more documentation :-) The tests look clear and clean, however they are not testing the API properly. With these changes, we should get on the right track ;) Thanks for the work so far! Please feel free to reach out if you have any other question! ::: toolkit/components/telemetry/tests/gtest/TestHistograms.cpp @@ +173,5 @@ > } > + > +TEST_F(TelemetryTestFixture, AccumulateCategoricalHistogram) > +{ > + const Telemetry::LABELS_TELEMETRY_TEST_CATEGORICAL kExpectedValue = This is not an expected value but an identifier for the categorical histogram we want to modify. I can see how this is confusing though, the header file don't provide much info! See http://searchfox.org/mozilla-central/rev/05c4c3bc0cfb9b0fc66bdfc8c47cac674e45f151/toolkit/components/telemetry/Telemetry.h#101,117,134 @@ +182,5 @@ > + GetAndClearHistogram(cx.GetJSContext(), mTelemetry, > + NS_LITERAL_CSTRING("TELEMETRY_TEST_CATEGORICAL"), false); > + > + // Accumulate in the histogram > + Telemetry::AccumulateCategorical(kExpectedValue); This is basically saying "Accumulate one unit into the categorical histogram with label Telemetry::LABELS_TELEMETRY_TEST_CATEGORICAL::CommonLabel". The value to accumulate is always 1 as far as I can tell for the c++ categorical histograms. I would add a comment above this line to explain that and then explicitly call the accumulation function like that: Telemetry::AccumulateCategorical(Telemetry::LABELS_TELEMETRY_TEST_CATEGORICAL::CommonLabel); Since the categorical histogram c++ API allows for two different ways of accumulating, I would additionally try to accumulate in the same label using the string version of the API: Telemetry::AccumulateCategorical(Telemetry::TELEMETRY_TEST_CATEGORICAL, NS_LITERAL_CSTRING("CommonLabel")); At the end of this test, we would check if that label for the categorical histogram contains the value of 2 (you can define kExpectedValue as 2). @@ +204,5 @@ > +} > + > +TEST_F(TelemetryTestFixture, AccumulateKeyedCategoricalHistogram) > +{ > + const Telemetry::LABELS_TELEMETRY_TEST_KEYED_CATEGORICAL kExpectedValue = The same comment for the other test applies here: this is the cateogrical label, not the expected value. Just pass this to the accumulate function below. @@ +234,5 @@ > + > + // Check that the "sum" stored in the histogram matches with |kExpectedValue| > + uint32_t uSum = 0; > + JS::ToUint32(cx.GetJSContext(), sum, &uSum); > + ASSERT_EQ(uSum, static_cast<uint32_t>(kExpectedValue)) << "The histogram is not returning expected value"; The expected value in this case should be 1, since you're only performing a single accumulation on that key/label pair.
Attachment #8909065 -
Flags: feedback+
Reporter | ||
Updated•7 years ago
|
Flags: needinfo?(alessio.placitelli)
Assignee | ||
Comment 13•7 years ago
|
||
That makes sense. I got lucky that my tests passed - I just happened to choose an enum with the value of 1. I'll make some changes.
Assignee | ||
Comment 14•7 years ago
|
||
How's this look?
Attachment #8909065 -
Attachment is obsolete: true
Flags: needinfo?(alessio.placitelli)
Reporter | ||
Comment 15•7 years ago
|
||
Comment on attachment 8912930 [details] [diff] [review] 1351274-2.patch Review of attachment 8912930 [details] [diff] [review]: ----------------------------------------------------------------- Great job, this looks on the right track now! There are a few smaller issues left, let's iron them out. One note: when requesting a review of a patch, instead of adding a need-info for me, you can also set me as a reviewer in the form you use to attach it. ::: toolkit/components/telemetry/tests/gtest/TestHistograms.cpp @@ +42,5 @@ > } > > void > +GetElement(JSContext* cx, uint32_t index, JS::HandleValue valueIn, > + JS::MutableHandleValue valueOut) nit: please align this with |JSContext| - I know this is identical to |GetProperty|, but the other function is wrong :P Since you're here, would you mind fixing indentation there as well? @@ +196,5 @@ > + // Telemetry::LABELS_TELEMETRY_TEST_CATEGORICAL::CommonLabel > + Telemetry::AccumulateCategorical(Telemetry::LABELS_TELEMETRY_TEST_CATEGORICAL::CommonLabel); > + > + // Accumulate another unit into the categorical histogram with label > + // Telemetry::LABELS_TELEMETRY_TEST_CATEGORICAL::CommonLabel using the string API nit: rephrase this as "Accumulate another unit into the same categorical histogram using a string label". Let's also remove the trailing whitespace @@ +208,5 @@ > + // Get our histogram from the snapshot > + JS::RootedValue histogram(cx.GetJSContext()); > + GetProperty(cx.GetJSContext(), "TELEMETRY_TEST_CATEGORICAL", snapshot, &histogram); > + > + // Get counts array from histogram. Each entry in the array maps to a label in the histogram. nit: remove the trailing whitespace @@ +234,5 @@ > + GetAndClearHistogram(cx.GetJSContext(), mTelemetry, > + NS_LITERAL_CSTRING("TELEMETRY_TEST_KEYED_CATEGORICAL"), true); > + > + // Accumulate one unit into the categorical histogram with label > + // Telemetry::LABELS_TELEMETRY_TEST_CATEGORICAL::CommonLabel nit: that's LABELS_TELEMETRY_TEST_KEYED_CATEGORICAL (it's missing KEYED) @@ +236,5 @@ > + > + // Accumulate one unit into the categorical histogram with label > + // Telemetry::LABELS_TELEMETRY_TEST_CATEGORICAL::CommonLabel > + Telemetry::AccumulateCategoricalKeyed(NS_LITERAL_CSTRING("sample"), > + Telemetry::LABELS_TELEMETRY_TEST_KEYED_CATEGORICAL::CommonLabel); Let's accumulate at least in 2 keys (with different values) and make sure to get the correct values out. @@ +250,5 @@ > + // Get "sample" property from histogram > + JS::RootedValue sample(cx.GetJSContext()); > + GetProperty(cx.GetJSContext(), "sample", histogram, &sample); > + > + // Get counts array from the sample. Each entry in the array maps to a label in the histogram. nit: remove the trailing whitespace @@ +257,5 @@ > + > + // Get the value for the label we care about > + JS::RootedValue value(cx.GetJSContext()); > + GetElement(cx.GetJSContext(), > + static_cast<uint32_t>(Telemetry::LABELS_TELEMETRY_TEST_CATEGORICAL::CommonLabel), The label here should be Telemetry::LABELS_TELEMETRY_TEST_KEYED_CATEGORICAL::CommonLabel - the labels happen to have the same index.
Attachment #8912930 -
Flags: feedback+
Reporter | ||
Updated•7 years ago
|
Flags: needinfo?(alessio.placitelli)
Assignee | ||
Comment 16•7 years ago
|
||
Attachment #8912930 -
Attachment is obsolete: true
Attachment #8914111 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Attachment #8914111 -
Flags: review+
Assignee | ||
Comment 17•7 years ago
|
||
Attachment #8914111 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8914112 -
Flags: review?(alessio.placitelli)
Reporter | ||
Comment 18•7 years ago
|
||
Comment on attachment 8914112 [details] [diff] [review] 1351274-4.patch Review of attachment 8914112 [details] [diff] [review]: ----------------------------------------------------------------- This looks good, we only need to make one little change to the second test and we're done! ::: toolkit/components/telemetry/tests/gtest/TestHistograms.cpp @@ +238,5 @@ > + Telemetry::AccumulateCategoricalKeyed(NS_LITERAL_CSTRING("sample"), > + Telemetry::LABELS_TELEMETRY_TEST_KEYED_CATEGORICAL::CommonLabel); > + > + // Accumulate another unit into the same categorical histogram > + Telemetry::AccumulateCategoricalKeyed(NS_LITERAL_CSTRING("sample"), We should use a different key here, as we want to make sure that the keying system works with categoricals as well. Let's just swap "sample" with "other-key" and change the rest of the test to check that both keys have the right value.
Attachment #8914112 -
Flags: review?(alessio.placitelli) → feedback+
Assignee | ||
Comment 19•7 years ago
|
||
Attachment #8914112 -
Attachment is obsolete: true
Attachment #8914578 -
Flags: review?(alessio.placitelli)
Reporter | ||
Comment 20•7 years ago
|
||
Comment on attachment 8914578 [details] [diff] [review] 1351274-5.patch Review of attachment 8914578 [details] [diff] [review]: ----------------------------------------------------------------- This looks good now, thanks for your efforts David!
Attachment #8914578 -
Flags: review?(alessio.placitelli) → review+
Reporter | ||
Comment 21•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b59777a55bc2d5a6e35260a48834255392d9bfb3
Reporter | ||
Comment 22•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba149aead55314a6b9094aef39dfd0c8b0439c5a Bug 1351274 - Add C++ test coverage for the Categorical Histogram. r=dexter
Comment 23•7 years ago
|
||
Pushed by alessio.placitelli@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ba149aead553 Add C++ test coverage for the Categorical Histogram. r=dexter
Reporter | ||
Comment 24•7 years ago
|
||
David, would you be interested in taking on another bug? There are a few options depending on the language of choice. For example, for C++ one valuable bug for us would be bug 1393041.
Flags: needinfo?(david)
Comment 25•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ba149aead553
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Reporter | ||
Updated•6 years ago
|
Flags: needinfo?(david)
You need to log in
before you can comment on or make changes to this bug.
Description
•