1427766 - Add a C++ Test for GetHistogramName

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P3)

Description

[Chris H-C :chutten]

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.

Comment 1

[maera]

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.

Comment 2

[Chris H-C :chutten]

Certainly! Take your time. It's assigned to you.

Comment 3

[Dhi Aurrahman]

Attachment: Add a C++ Test for GetHistogramName

Comment 4

[Chris H-C :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.

Comment 5

[Dhi Aurrahman]

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.

Comment 6

[Dhi Aurrahman]

Attachment: Add a C++ Test for GetHistogramName

Comment 7

[Chris H-C :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=

Comment 8

[Dhi Aurrahman]

Attachment: Add a c++ test for GetHistogramName

Comment 9

[Chris H-C :chutten]

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?

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/37f8ff509e9d
Add a c++ test for GetHistogramName. r=chutten

Comment 11

[Dhi Aurrahman]

> (( 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?

Comment 12

[Narcis Beleuzu [:NarcisB]]

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

Comment 13

[Dhi Aurrahman]

(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?

Comment 14

[Chris H-C :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.

Comment 15

[Dhi Aurrahman]

Attachment: Add a c++ test for GetHistogramName

Comment 16

[Dhi Aurrahman]

Hi Chris, I have updated the patch. Please help me to double check it. Thanks!

Comment 17

[Chris H-C :chutten]

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.

Comment 18

[Chris H-C :chutten]

(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

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/54ad50a25826
Add a c++ test for GetHistogramName r=chutten

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/54ad50a25826