Closed Bug 1427766 Opened 4 years ago Closed 4 years ago

Add a C++ Test for GetHistogramName

Categories

(Toolkit :: Telemetry, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: chutten, Assigned: diorahman, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good first bug][lang=c++])

Attachments

(1 file, 3 obsolete files)

We have gtest C++ tests for Histograms in toolkit/components/telemetry/tests/gtest/TestHistograms.cpp

But we don't have complete test coverage.

For instance, we don't test Telemetry::GetHistogramName(aId). It should return the const char* name for a given enum id, but if we don't test it we can't be sure.

You can see that the entire function is uncovered according to codecov.io: https://codecov.io/gh/marco-c/gecko-dev/src/master/toolkit/components/telemetry/Telemetry.cpp#L1965

This bug is to add a test to TestHistograms.cpp that ensures this function works as expected.
Could I be assigned this bug, please? Thanks.

Is it okay if I look at this on the weekend?  I am out of town, right now and only access is my tiny phone.
Certainly! Take your time. It's assigned to you.
Assignee: nobody → qimeara
Status: NEW → ASSIGNED
Assignee: qimeara → diorahman
Attachment #8951950 - Flags: review?(chutten)
Comment on attachment 8951950 [details] [diff] [review]
Add a C++ Test for GetHistogramName

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

Thank you for your contribution!

Be sure to include r?chutten in your commit message's first line.

When writing a test we don't need to attack the code exhaustively. This test will be fragile because some of the histograms you are using will expire, or may be removed in the future. I recommend testing one of the TELEMETRY_TEST_* histograms instead to test the part of TelemetryHistogram::GetHistogramName that successfully returns the name, and then maybe using HistogramCount to test the part of the function that fails to return the name.

Does this make sense? Please let me know if I can help clarify anything.
Attachment #8951950 - Flags: review?(chutten)
Hi Chris, sorry for that. But I'm not sure about this: maybe using HistogramCount to test the part of the function that fails to return the name.
Flags: needinfo?(chutten)
Attachment #8952873 - Flags: review?(chutten)
Comment on attachment 8952873 [details] [diff] [review]
Add a C++ Test for GetHistogramName

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

This is a good patch, thank you for your contribution!

If we look at TelemetryHistogram::GetHistogramName()[1], there are two branches the code can follow. In the first, if the HistogramID we pass isn't actually a Histogram ID, we warn, assert, and return nullptr. In the second, we return the name.

Your test covers the second branch completely.

Now we need to find a way to test the first branch, too. To do that, we must pass something that is a HistogramID type, but will fail at being a histogram id. "mozilla::Telemetry::HistogramCount" is of the HistogramID type, but isn't a Histogram (it is what's known as a sentinel value, and marks the end of the acceptable HistogramID values[2]). That would make it a good test to ensure that calling GetHistogramName with an invalid value will return nullptr.

[1]: https://searchfox.org/mozilla-central/rev/9a8d3f8191b15cdca89a7ce044c7bea2dd0462dc/toolkit/components/telemetry/TelemetryHistogram.cpp#2168
[2]: https://searchfox.org/mozilla-central/search?q=HistogramCount&path=
Attachment #8952873 - Flags: review?(chutten)
Flags: needinfo?(chutten)
Attachment #8953317 - Flags: review?(chutten)
Attachment #8951950 - Attachment is obsolete: true
Attachment #8952873 - Attachment is obsolete: true
Comment on attachment 8953317 [details] [diff] [review]
Add a c++ test for GetHistogramName

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

(( Tip: When you upload a new version of a patch, mark the old version as "obsolete" so that it's hidden from view. ))

This looks good! I will go ahead and mark this for checkin so it can be included in our next builds. Thank you!

Have you given any thought to what you might want to do for your next contribution?
Attachment #8953317 - Flags: review?(chutten) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/37f8ff509e9d
Add a c++ test for GetHistogramName. r=chutten
Keywords: checkin-needed
> (( Tip: When you upload a new version of a patch, mark the old version as
> "obsolete" so that it's hidden from view. ))

Thanks for the tip Chris! Sorry I didn't do that. 

> 
> This looks good! I will go ahead and mark this for checkin so it can be
> included in our next builds. Thank you!

Awesome. Thanks for your guidance!

> 
> Have you given any thought to what you might want to do for your next
> contribution?

Not, yet. Since I have considerably plenty of time I'm thinking to find any mentored bugs to work with. Do you have any for me, Chris? 

One question: how do we manage multiple patches in our local? With different branches?
(In reply to Narcis Beleuzu [:NarcisB] from comment #12)
> Backed out for GTest crashes on TelemetryHistogram::GetHistogramName
> 
> Log:
> https://treeherder.mozilla.org/logviewer.html#?job_id=164031653&repo=mozilla-
> inbound&lineNumber=1712
> Push:
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=37f8ff509e9d97ff58a9c98bea9719ee473ca7c8
> Backout push:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 03eed16ea12ef8e7cd29040f86120b55ed82711f

Chris, is there a way to get around the `MOZ_ASSERT_UNREACHABLE` while in a test mode? If not, should we just remove the relevant test?
Flags: needinfo?(diorahman) → needinfo?(chutten)
I have half a feeling that we ought to be able to test that it _does_ assert (thus "catching" the assert, and testing proper behaviour), but I have no experience with it. Let me search around...

Blast, I find no evidence of a "will assert" directive being used in gecko. Let me check the documentation...

Darn. Google Test does have "Death Tests" but they appear to not be what we want. So we'll have to remove the test.
Flags: needinfo?(chutten)
Attachment #8953317 - Attachment is obsolete: true
Attachment #8954199 - Flags: review?(chutten)
Hi Chris, I have updated the patch. Please help me to double check it. Thanks!
Comment on attachment 8954199 [details] [diff] [review]
Add a c++ test for GetHistogramName

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

Sorry for having this bounce on you. I should have caught that a debug-only assert would probably be missed in your testing, and that our gtest setup might not catch moz asserts.
Attachment #8954199 - Flags: review?(chutten) → review+
Keywords: checkin-needed
(In reply to Dhi Aurrahman from comment #11)
> > 
> > Have you given any thought to what you might want to do for your next
> > contribution?
> 
> Not, yet. Since I have considerably plenty of time I'm thinking to find any
> mentored bugs to work with. Do you have any for me, Chris? 
> 
> One question: how do we manage multiple patches in our local? With different
> branches?

I tend to use multiple branches, but I'm using git[1]. If you are using mercurial, then I'm not sure what the recommended workflow is for multiple "branches". You could ask on IRC on #developers about that, and I'm sure you'll hear a few helpful opinions :)

What are your interests? If you want to try some light Python work in our parsing scripts, there's bug 1426461 for making our build error messages better. If you'd like some JS work, there's bug 918557 for removing an unnecessary cache. In C++ there's bug 1432362 for ensuring we don't trip up in some undefined behaviour when callers try to record negative intervals of time.

[1]: https://github.com/glandium/git-cinnabar/wiki/Mozilla:-A-git-workflow-for-Gecko-development
Pushed by aciure@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/54ad50a25826
Add a c++ test for GetHistogramName r=chutten
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/54ad50a25826
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.