1427765 - Extend C++ Test Coverage for non-keyed Histograms referenced by string

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 calling Accumulate on a non-keyed histogram when we reference it by name instead of its enum. We do have coverage for that API entry point for keyed histograms here: https://searchfox.org/mozilla-central/rev/51cd1093c5c2113e7b041e8cc5c9bf2186abda13/toolkit/components/telemetry/tests/gtest/TestHistograms.cpp#94

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#L1940

This bug is to extend or add a test that ensures that we can accumulate to non-keyed histograms by string.

Comment 1

[Archit Jugran]

Hey , I would like to work on this bug;I googled and read about gtest; I am a newbie and  could not understand what you meant by "You can see that the entire function is uncovered according to codecov.io" and what are the green and red lines when i open that link.

Comment 2

[Chris H-C :chutten]

No worries! I'm sorry I wasn't clear.

codecov.io is a tool for displaying which lines of code are run during tests. This lets you highlight areas that aren't run by the tests (the red lines) which gives us hints about how to write new tests.

The bug here is to extend the test that accumulates to non-keyed histograms by enum id to also test that accumulating to it by string name also works.

If you have any more questions, please do ask them here and I'll get back to you as quickly as I can.

Comment 3

[Archit Jugran]

Hey chris, I have another doubt.now i understood that i need to make that function on line 1940 green somehow by adding tests. but what exactly do you mean by adding a test?i mean what is a test(piece of code or something else?) Thank you a lot :)

Comment 4

[Archit Jugran]

Sorry I wasn't clear in my above comment, i mean to ask that where are we testing calling Accumulate on  a non keyed histogram by enum (except the fact that it is green).. i could not find a test call for the already green code in Telemetry.cpp

Comment 5

[Chris H-C :chutten]

No problem, I'm here to help!

The tests that are exercising this code are in the toolkit/components/telemetry/tests/ directory. Some will be in the gtest directory, specifically in the file TestHistograms.cpp

It is in that file that we test non-keyed histograms by enum id in the test called AccumulateCountHistogram. On line 25 we have

Telemetry::Accumulate(Telemetry::TELEMETRY_TEST_COUNT, kExpectedValue);

This bug is about adding another line under it that uses the string literal "TELEMETRY_TEST_COUNT" to accumulate a second time to the histogram. Then the assert will need to be changed to expect a different value (accumulating twice means we can expect twice the value).

Then, the next time the code coverage report is run, we will have made those lines green because we will be testing the code path that accumulates to non-keyed histograms by string.

Does this make sense?

Comment 6

[Chris H-C :chutten]

Hello! Are you stuck, is there anything I can help with?

Comment 7

[Archit Jugran]

Hi. Sorry for the delay. I will definitely work on it from 3rd March . Too busy till then with major exams.

Comment 8

[Chris H-C :chutten]

Very good, I look forward to your contribution when you have the time.

Comment 9

[Chris H-C :chutten]

How is this going? If you're still busy, I can unassign this to take this off of your plate. We'll still have plenty of bugs when you have time later :)

Comment 10

[Archit Jugran]

ok sure ! sorry for the delay

Comment 11

[pranjal2k16]

Hey Chris! Can you pls explain me what I can do to fix this bug (Considering I am a complete newbie)?

Comment 12

[Chris H-C :chutten]

(( Redirecting assignee to pranjal2k16 since Archit is busy for now ))

To help Mozilla out with this bug, here's the steps:

0) Comment here on the bug that you want to volunteer to help. I (or someone else) will assign it to you. (( We've done this now :D ))
1) Download and build the Firefox source code: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build
- If you have any problems, please ask on IRC (https://wiki.mozilla.org/Irc) in the #introduction channel. They're there to help you get started.
- You can also read the Developer Guide, which has answers to most development questions: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction
2) Start working on this bug. The necessary information should be in Comment#5.
- If you have any problems with this bug, please comment on this bug and set the needinfo flag for me. Also, you can find me and my teammates on the #telemetry channel on IRC (https://wiki.mozilla.org/Irc) most hours of most days.
3) Build your change with `mach build` and test your change with `mach test toolkit/components/telemetry/tests/` and `mach gtest Telemetry*`. Also check your changes for adherence to our style guidelines by using `mach lint`
4) Submit the patch (including an automated test, if applicable) for review. Mark me as a reviewer so I'll get an email to come look at your code.
- Here's the guide: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch
5) After a series of reviews and changes to your patch, I'll mark it for checkin or push it to autoland. Your code will soon be shipping to Firefox users worldwide!
6) ...now you get to think about what kind of bug you'd like to work on next. Let me know what you're interested in and I can help you find your next contribution.

Does that help?

Comment 13

[pranjal2k16]

Unable to build firefox. Everytime I try to do it,it show package pango was not found in the pkg-config search path.How to remove this???

Comment 14

[Chris H-C :chutten]

I'm afraid I don't know what that means or how to fix it. Luckily, I know that on the #introduction IRC channel (https://wiki.mozilla.org/Irc) are many people who probably _do_ know how to help with this issue. Can you ask there?

Comment 15

[Kiran]

Hi I just build firefox for desktop , and found this bug.
Can I work on this?

After going through above comments,

I am trying to fix it like this.

TEST_F(TelemetryTestFixture, AccumulateCountHistogram)
{
...........................................

  // Accumulate in the histogram
  Telemetry::Accumulate(Telemetry::TELEMETRY_TEST_COUNT, kExpectedValue);
  Telemetry::Accumulate(Telemetry::TELEMETRY_TEST_COUNT, kExpectedValue);//Added this line


  ASSERT_EQ(uSum, 2*kExpectedValue) << "The histogram is not returning expected value";//Modify assert by 2*expectedValue
}

can I proceed?

I am newBie over here, Please help me.

Comment 16

[Chris H-C :chutten]

(In reply to Kiran from comment #15)
> Hi I just build firefox for desktop , and found this bug.
> Can I work on this?

This bug is currently assigned, so it isn't available to be worked on. 

That being said, :pranja2k16, have you been able to make headway on this bug? Or shall I unassign it for now?

Comment 17

[pranjal2k16]

Unassign it for now(In reply to Chris H-C :chutten from comment #16)
> (In reply to Kiran from comment #15)
> > Hi I just build firefox for desktop , and found this bug.
> > Can I work on this?
> 
> This bug is currently assigned, so it isn't available to be worked on. 
> 
> That being said, :pranja2k16, have you been able to make headway on this
> bug? Or shall I unassign it for now?

Unassign it for now.

Comment 18

[Chris H-C :chutten]

Kiran, this bug is now available to be worked on if you're still interested?

Your example solution isn't quite right. Those are using the version of Accumulate that take enum values instead of string. We want to test the version of Accumulate that uses the string name instead of the enum value.

Comment 19

[Kiran]

Ya sure I will work on this.

Comment 20

[Kiran]

Attachment: secondPatch.diff

Fixed Bug 1427765

Comment 21

[Chris H-C :chutten]

Comment on attachment 8962186 [details] [diff] [review]
secondPatch.diff

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

Thank you for your contribution!

Be sure to read the instructions in Comment#12 about how to test and submit your patch (setting the review flag would make sure I don't lose track of this bug :) ). With the small change I highlighted, and following those instructions, we'll be able to get this into the tree in short order.

::: toolkit/components/telemetry/tests/gtest/TestHistograms.cpp
@@ +42,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, 2*kExpectedValue) << "The histogram is not returning expected value";

I think it would be better for readability to update the value of kExpectedValue instead of adjusting it here.

Comment 22

[Kiran]

Attachment: secondPatch.diff

Modified patch as per review comments

Comment 23

[Chris H-C :chutten]

Comment on attachment 8962833 [details] [diff] [review]
secondPatch.diff

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

Thank you for this new patch. Well, I say patch, but it appears as though it's only a diff. It will need to be a full commit, including an informative and properly-formatted commit message (see instruction number 4), before it can be submitted to the tree.

Something like this, perhaps?

bug 1427765 - Ensure we test accumulating histograms by string r?chutten

::: toolkit/components/telemetry/tests/gtest/TestHistograms.cpp
@@ +26,5 @@
>  
>    // Accumulate in the histogram
>    Telemetry::Accumulate(Telemetry::TELEMETRY_TEST_COUNT, kExpectedValue);
> +  Telemetry::Accumulate("TELEMETRY_TEST_COUNT", kExpectedValue);
> +  kExpectedValue = 2 * kExpectedValue;

I'm sorry, I was unclear. I meant changing the value on line 18 so it would be 200 (and still const (that's what the `k` means)). We'll have to accumulate `kExpectedValue / 2` in each call.

Comment 24

[Kiran]

Attachment: secondPatch.diff

Please check this diff so that i can submit a patch for same.

Comment 25

[Chris H-C :chutten]

Sadly I can't review this, it doesn't appear to have been detected properly as a patch.

Please read https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch 

Following those instructions we should be able to get a patch up for my review.

Comment 26

[Chris H-C :chutten]

Unassigning due to inactivity. If you come back with a new patch, I'll happily assign it right back!

Comment 27

[Sidharth]

Hi Chris, 

I am a newbie in C++. I want to increase my skill level. I would like to work on the same.

Would it be possible for you to Assign this task to me?

Comment 28

[Chris H-C :chutten]

Please go ahead, and let me know if you have any questions. Be sure to read the state of this bug so far. The instructions in Comment#12 are particularly relevant to someone new to working on the mozilla-central codebase.

Comment 29

[Kiran]

Attachment: my.patch

Sorry for delay,
Please check this attached patch if possible.

Comment 30

[Chris H-C :chutten]

Thank you for coming back with your patch Kiran.

Sidharth, is it okay if I reassign this back to Kiran in light of his attached patch? I could use your help maybe on bug 1458125 instead, if that is an appropriate substitute.

Comment 31

[Chris H-C :chutten]

It seems as though sidharth may have moved on to other things, so I'll take a look at Kiran's patch now.

Comment 32

[Chris H-C :chutten]

Comment on attachment 8972373 [details] [diff] [review]
my.patch

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

This is great, thanks!

Could you format this as a patch with an appropriate commit message (some help on that can be found here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch) and then I can mark it for checkin!

Comment 33

[Kiran]

Can you help me in adding commit message?
I generated patch using follwing command
hg diff >my.patch

Comment 34

[Chris H-C :chutten]

Here's some helpful instructions: https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 35

[Chris H-C :chutten]

Kiran, were you able to generate a patch? We're nearly done.

Comment 36

[Kiran]

Hi,
Unfortunately i unable to generate the patch with commit message.
I tried with hg bzexport but this extension is not working for me.Also after going through all documents I didn't find way.
Can you help me with same?

Comment 37

[Chris H-C :chutten]

./mach mercurial-setup should have set you up with the appropriate extensions to either export to bugzilla or to send to mozreview.

Without bzexport or mozreview you could try `hg export . > my-change.patch`

Having a working setup for formatting commits is crucial for getting your patches into the tree, so if it's okay we should spend some time getting this one-time setup done to make things smooth now and through the future.

If you have any problems while you're trying to make this work, please connect to the #introduction channel on irc.mozilla.org. There are instructions here: https://wiki.mozilla.org/Irc

#introduction is full of helpful people, and they will likely have more knowledge in troubleshooting setup problems than I have.

Comment 38

[Chris H-C :chutten]

Attachment: bug 1427765 - Add gtest coverage for accumulating using strings

Review commit: https://reviewboard.mozilla.org/r/253812/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/253812/

Comment 39

[Chris H-C :chutten]

Comment on attachment 8988573 [details]
bug 1427765 - Add gtest coverage for accumulating using strings

https://reviewboard.mozilla.org/r/253812/#review260488

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8d42dde4eb46
Add gtest coverage for accumulating using strings r=chutten

Comment 41

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/8d42dde4eb46