1312698 - Add test coverage for the C++ Histogram API

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P3)

Description

[Georg Fritzsche [:gfritzsche]]

We need to add C++ unit test coverage for the Histogram C++ API.
Bug 1305648 lays the ground-work for this.

Comment 1

[Alessio Placitelli [:Dexter]]

Now that bug 1305648 landed, some additional details:

- We need to create a new cpp file to contain the histogram tests.
- We need to let the build system know about the the newly created file by adding its name here [1].
- Test coverage needs to be added for the functions highlighted at [2]. A list of call sites (to get an idea about how they are used) can be found at [3]. Basically, all the "Accumulate*" functions in the "Telemetry::" namespace need to be covered, with the exception of |AccumulateChild| and |AccumulateChildKeyed|.
- In [4] an example of how to setup a test case can be found.

[1] - http://searchfox.org/mozilla-central/rev/51aa673e28802fe6ea89f4793721fc55334a6ac8/toolkit/components/telemetry/tests/gtest/moz.build#14
[2] - http://searchfox.org/mozilla-central/rev/51aa673e28802fe6ea89f4793721fc55334a6ac8/toolkit/components/telemetry/Telemetry.h#63,72,83,95,105,121,130,154,156
[3] - https://dxr.mozilla.org/mozilla-central/search?q=%22Telemetry%3A%3A%22+ext%3Acpp&redirect=false
[4] - http://searchfox.org/mozilla-central/rev/51aa673e28802fe6ea89f4793721fc55334a6ac8/toolkit/components/telemetry/tests/gtest/TestScalars.cpp#130

Comment 2

[Avikalpa Kundu [:kalpa]]

It's taking me sometime to wrap my head around JSAPI and Gecko.
Also I'm having some questions regarding the JSAPI and GC Rooting.
----------------------------------------------------------------------------------------------------------
from: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/GC_Rooting_Guide

As I understand it, (I will definitely be wrong on some points; So please do correct me)

All GC things stored on the STACK must use JS::Rooted<T> (exposed API).
GC things include 5 types (they are like the FILE * of stdio.h)
	1. JS::Context* : Any memory allocation occurs through this type in a JS::Runtime.
	2. JS::Value and jsid: If it is a pointer type stored then the GC applies to this type.
	3. JS::Object*, JS::String* and JS::Script* must always be garbage collected.

1. Exposed type of the engine is the template class JS::Rooted<T>.
2. Both JS::MutableHandle<T> and JS::Handle<T> internally casts to JS::Rooted<T> 
3. JS::Handle<T> cannot be created manually.

The exposed API of doing so is,
JS::RootedValue  abc (context, initialValue-optional)
This acts a a clean sane wrapper. 

Question 1 ->
What does "Rooting" actually mean?

Question 2 ->
What is the difference between 1 & 2?

1. const auto abc = JS::RootedValue def (context-cx, JS_GetSomeValue(cx))
2. JS::HandleValue abc = JS::RootedValue def (context-cx, JS_GetSomeValue(cx))

1. auto& abc = JS::RootedValue def (context-cx, JS_GetSomeValue(cx))
2. JS::MutableHandle abc = JS::RootedValue def (context-cx, JS_GetSomeValue(cx))

I ask this since JS::Handle is made only to reuse rooted elements.
And JS::MutableHandle is made to mutate existing elements.

Question 3 ->
There are extra allocation performance costs due to JS::Handle and 
JS::MutableHandle casting to JS::Rooted. 
A macro might be better suited as this way like, 

SM_JS_VALUE  abc (context, initialValue-optional) [SM stands for spidermonkey].

and then handle or mutable handle can be made like,

const auto handle = abc;
auto &mutHandle = abc;

Extra conversions and extra 'this' pointers are avoided then.
That is, there is a performance cost for the current API.

Obviously, I'm missing some points here and I have not deeply explored the codebase but seeing from 'jsapi.h'[1] I think the added functionality can be provided in JS::Rooted itself. Can anyone tell me the reasons for doing so?
----------------------------------------------------------------------------------------------------------
[1]http://searchfox.org/mozilla-central/source/js/src/jsapi.h

Thanks.

Comment 3

[Alessio Placitelli [:Dexter]]

(In reply to Avikalpa Kundu from comment #2)
> It's taking me sometime to wrap my head around JSAPI and Gecko.
> Also I'm having some questions regarding the JSAPI and GC Rooting.

Don't worry, take your time!
I just want to clarify that you are not required to dive too deep into the JS API mechanics for this bug.

After wrapping your head about the API a bit, you could start by partially implementing what's described in comment 1: create the new test file and add a very simple initial test.

For example, you could add a test that simply calls

// Increments the test histogram
Telemetry::Accumulate(Telemetry::TELEMETRY_TEST_COUNT, 1);

Then get the histogram by calling:

JS::Rooted<JS::Value> hist;
Telemetry::GetHistogramById("TELEMETRY_TEST_COUNT", ..., hist);

Then we can deal with getting a snapshot of the histogram by calling the "snapshot" method of the histogram object (probably through JS_CallFunctionName [1])

> Question 1 ->
> What does "Rooting" actually mean?

The rooting API [2] contains a nice definition for this along with a minimal example.

> Question 2 ->
> What is the difference between 1 & 2?
> 
> 1. const auto abc = JS::RootedValue def (context-cx, JS_GetSomeValue(cx))
> 2. JS::HandleValue abc = JS::RootedValue def (context-cx,
> JS_GetSomeValue(cx))
> 
> 1. auto& abc = JS::RootedValue def (context-cx, JS_GetSomeValue(cx))
> 2. JS::MutableHandle abc = JS::RootedValue def (context-cx,
> JS_GetSomeValue(cx))
> 
> I ask this since JS::Handle is made only to reuse rooted elements.
> And JS::MutableHandle is made to mutate existing elements.

AFAIK, JS::Handle and MutableHandle should only be used to pass things around. The comment at [2] also contains a nice explaination about when/why to use them.

> Question 3 ->
> There are extra allocation performance costs due to JS::Handle and 
> JS::MutableHandle casting to JS::Rooted. 
> A macro might be better suited as this way like, 
> 
> SM_JS_VALUE  abc (context, initialValue-optional) [SM stands for
> spidermonkey].
> 
> and then handle or mutable handle can be made like,
> 
> const auto handle = abc;
> auto &mutHandle = abc;
> 
> Extra conversions and extra 'this' pointers are avoided then.
> That is, there is a performance cost for the current API.
> 
> Obviously, I'm missing some points here and I have not deeply explored the
> codebase but seeing from 'jsapi.h'[1] I think the added functionality can be
> provided in JS::Rooted itself. Can anyone tell me the reasons for doing so?

Since this is test coverage code, clarity wins over performance and the latter isn't the main requirement.
But if you feel like there's some improvement that can be done elsewhere in the code or in the JS API, that is outside of the scope of this bug, feel free to file a bug in the related component or to discuss the improvement over IRC in #jsapi @ irc.mozilla.org!


[1] - https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_CallFunction
[2] - http://searchfox.org/mozilla-central/rev/790b2cb423ea1ecb5746722d51633caec9bab95a/js/public/RootingAPI.h#37

Comment 4

[Avikalpa Kundu [:kalpa]]

Thanks. I'll try to post the suggestion to the correct place.

How do you actually want me to test all Accumulate* ?

Do you want to ->

1. Make histogram of each type to Histograms.json and check the data. If so how should I test the data? And what are the boundary conditions for each "kind" to be tested, if any?

2. The Scalars in (TestScalars.cpp) are tested as "types". In case of histogram it should be "kinds". So, do you want me to test each kinds and correctness of the data in Histograms.json? Again, How should I actually test the data?

Comment 5

[Alessio Placitelli [:Dexter]]

(In reply to Avikalpa Kundu from comment #4)
> Thanks. I'll try to post the suggestion to the correct place.
> 
> How do you actually want me to test all Accumulate* ?

Testing *all* of the Accumulate usages might be too much. Let's keep it simple for now and just test the scenario outlined in comment 3. 

> Do you want to ->
> 
> 1. Make histogram of each type to Histograms.json and check the data. If so
> how should I test the data? And what are the boundary conditions for each
> "kind" to be tested, if any?

Don't worry, that's not needed. Just check the COUNT kind using the test histogram mentioned in comment 3. We'll go from there once this bit is complete.

> 2. The Scalars in (TestScalars.cpp) are tested as "types". In case of
> histogram it should be "kinds". So, do you want me to test each kinds and
> correctness of the data in Histograms.json? Again, How should I actually
> test the data?

Once the first test (count histogram accumulation) is complete, we'll decide how to move on. I think we should be testing each kind of histogram, but that's not an issue for *now*. Let's start with a minimal test case and build from there after that.

Comment 6

[Avikalpa Kundu [:kalpa]]

How to run only the telemetry gtests?

Comment 7

[Alessio Placitelli [:Dexter]]

(In reply to Avikalpa Kundu from comment #6)
> How to run only the telemetry gtests?

./mach gtest Telemetry*

Comment 8

[Avikalpa Kundu [:kalpa]]

(In reply to Alessio Placitelli [:Dexter] from comment #7)
> (In reply to Avikalpa Kundu from comment #6)
> > How to run only the telemetry gtests?
> 
> ./mach gtest Telemetry*

Unfortunately, that doesn't work.

Here's the output if it helps.
https://docs.google.com/document/d/1qJBYhTu-208LohS5k58xYU9xlNDLc532SJ6kfaQMWAU/edit?usp=sharing

Comment 9

[Avikalpa Kundu [:kalpa]]

Attachment: Just the Skeleton structure of patch

I've attached a skeleton patch.
Could you verify that I'm proceeding correctly?

Comment 10

[Alessio Placitelli [:Dexter]]

(In reply to Avikalpa Kundu from comment #8)
> (In reply to Alessio Placitelli [:Dexter] from comment #7)
> > (In reply to Avikalpa Kundu from comment #6)
> > > How to run only the telemetry gtests?
> > 
> > ./mach gtest Telemetry*
> 
> Unfortunately, that doesn't work.
> 
> Here's the output if it helps.
> https://docs.google.com/document/d/1qJBYhTu-
> 208LohS5k58xYU9xlNDLc532SJ6kfaQMWAU/edit?usp=sharing

I'm not entirely sure why this command fails on your system. You might try without the "*" at the end, to see if it fixes the problem.

Comment 11

[Avikalpa Kundu [:kalpa]]

(In reply to Alessio Placitelli [:Dexter] from comment #10)
> 
> I'm not entirely sure why this command fails on your system. You might try
> without the "*" at the end, to see if it fixes the problem.

Unfortunately, the problem persists.

Comment 12

[Alessio Placitelli [:Dexter]]

Comment on attachment 8831440 [details] [diff] [review]
Just the Skeleton structure of patch

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

That's a good start, thanks!

In order to check the values stored within an histogram in order to make sure that Accumulate is working correctly, we need to do two things:

1 - Clear the current value in the histogram before calling Telemetry::Accumulate, otherwise we won't be able to predict the value it will hold after the call.
2 - For the count histogram, we need to fetch the latest snapshot and can simply check the "sum" property on it.

For the first point, we could simply grab the "histogram object" for the test histogram by calling |GetHistogramById| and then calling the function "clear" on that object.

In comment 3, I was mistaken, we have no function |Telemetry::GetHistogramById| but you can access it as:

>  JS::RootedValue testHistogram(cx.GetJSContext());
>  mTelemetry->GetHistogramById(NS_LITERAL_CSTRING("TELEMETRY_TEST_COUNT"), cx.GetJSContext(), &testHistogram);

You would then be able to call |JS_CallFunctionName| on it and execute "clear" and, with that, the histogram will be empty.

For the second point, after calling the |Accumulate| function, you could access the current histogram snapshots here [1]. You can do that like:

> JS::RootedValue snapshot(cx.GetJSContext());
> mTelemetry->GetHistogramSnapshots(cx.GetJSContext(), &snapshot);

The |snapshot| object will contain a property for each recorded histogram (it can contain more than TELEMETRY_TEST_COUNT!). You can fetch the histogram that you need to check using the |JS_GetProperty| function [2].

Once you have the snapshot for that histogram, you can use |JS_GetProperty| again to get its "sum" property and compare it against the value you passed to the |Accumulate| function.

One last thing: it looks like this patch also creates an empty "gtest/q!" file. Please remove it from the patch.

Please don't hesitate to ask if anything is unclear!

[1] - http://searchfox.org/mozilla-central/rev/f31f538622d0dfb2d869d4babc4951f6001e9be2/toolkit/components/telemetry/nsITelemetry.idl#67
[2] - https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_GetProperty

::: toolkit/components/telemetry/Histograms.json
@@ +10670,5 @@
>      "bug_numbers": [1314220],
>      "description": "The time duration from tab's media was blocked to unblocked. Now we record from 1 to 90 seconds, but the record by milliseconds, so the bucket is like [1000ms, 2000ms], [2000ms, 3000ms], e.t.c.",
>      "releaseChannelCollection": "opt-out"
> +  },
> +  "TELEMETRY_TEST_HISTOGRAM_FLAG": {

I don't think we need to add new histograms right now. Please drop these changes.

::: toolkit/components/telemetry/tests/gtest/TestHistograms.cpp
@@ +3,5 @@
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */
> +
> +#include "gtest/gtest.h"
> +#include "js/jsapi.h"

This include is not needed (and you won't be able to include it anyway :-) )

@@ +27,5 @@
> +  nsCOMPtr<nsITelemetry> telemetry = do_GetService("@mozilla.org/base/telemetry;1");
> +
> +  JS::RootedValue histogramsSnapshot(aCx);
> +  JS::MutableHandleValue rval;
> +                                   

nit: please remove the trailing whitespaces here and below

@@ +38,5 @@
> +}
> +
> +}
> +
> +TEST_F(TelemetryTestFixture, Accumulate) {

Instead of calling this test Accumulate, let's name it AccumulateCountHistogram

@@ +42,5 @@
> +TEST_F(TelemetryTestFixture, Accumulate) {
> +  AutoJSContextWithGlobal cx(mCleanGlobal);
> +  JS::RootedValue result(cx.GetJSContext());
> +  
> +  Telemetry::Accumulate(Telemetry::TELEMETRY_TEST_HISTOGRAM_COUNT, 1);

We already have a test histogram in Telemetry, so we don't need to define a new one. Let's use TELEMETRY_TEST_COUNT instead of TELEMETRY_TEST_HISTOGRAM_COUNT.

@@ +49,5 @@
> +  CheckCountHistogram(cx.GetJSContext(), result);
> +}
> +
> +/*
> +TEST_F(TelemetryTestFixture, AccumumateCategoricalEnum) {

nit: Let's remove the commented block here.

We'll just concentrate on the test on the previous lines in this bug: we'll file new bugs and move from there to extend the coverage once the first test works!

::: toolkit/components/telemetry/tests/gtest/moz.build
@@ +12,4 @@
>  
>  UNIFIED_SOURCES = [
>      'TestScalars.cpp',
> +    'TestHistograms.cpp'

Please move this before the previous entry, otherwise the ./mach gtest command will fail (but I'm not that's triggering the problem you're seeing). The build system wants the files here to be sorted :)

Comment 13

[Alessio Placitelli [:Dexter]]

Avikalpa, how are you doing? Do you need any support on this patch? Feel free to get in touch if you do!

Comment 14

[Avikalpa Kundu [:kalpa]]

(In reply to Alessio Placitelli [:Dexter] from comment #13)
> Avikalpa, how are you doing? Do you need any support on this patch? Feel
> free to get in touch if you do!

Thanks Alessio.

I spent the last week learning vim and hg in depth. Hence, the conversation gap.
I'll get back to working on the patch from tomorrow.

My apologies for the delay.

Comment 15

[Avikalpa Kundu [:kalpa]]

Attachment: Bug 1312698 - Add test coverage for the C++ Histogram API

My apologies for the long time I took. I've uploaded the patch for review.

./mach gtest Telemety* runs but contains error of TelemetryFixture.h after patching. Hence I was unable to check for errors in my file. Log here [1].

./mach gtest Telemety* runs but the entire Telemetry tests are disabled before patching. Log here [2].

PS: I'm currently on changeset 3629511a266b (25 feb) if it helps.

Do you want me to refactor the patch for reuse in other histogram tests?

One last thing, I don't understand IDL files and XPCOM. Where can I learn more about them? How to practically use XPCOM and 'read' the IDLs? All the MDN articles speak about design and internals but not how to read them. How is a line in idl is converted to c++ and where are they put in directory and how to use them in c++ code? How to use them in python code? etc. Currently I'm guessing and figuring out what happens. A practical use case documentation and some examples will be a lot of help.

PS: It turns out after each pull I am supposed to clobber and build for 
./mach gtest to run successfully. Hence, it was not running previously. Is there any faster way to run gtest since after each pull mach does a whole build which takes a lot of time. Also, after each pull mach doesn't do incremental build. Am I missing some mach tricks?

[1] https://gist.github.com/avikalpa/bfecd0ce59c74fe3da0d85988ac89f22
[2] https://gist.github.com/avikalpa/ea625f55d1042483a9697765dae1117b

Comment 16

[Avikalpa Kundu [:kalpa]]

Attachment: Bug 1312698 - Add test coverage for the C++ Histogram API

Improved formatting.

Comment 17

[Alessio Placitelli [:Dexter]]

Comment on attachment 8841359 [details] [diff] [review]
Bug 1312698 - Add test coverage for the C++ Histogram API

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

No need to apologize, thank you for submitting the patch!

Please make sure to test your patch locally before requesting a review. If you are not sure about whether your patch is causing a problem, remove it and try to build without it. If your patch is causing problems and you are not sure about the problem, you could narrow down the issue by commenting blocks of code.

For the building part, unfortunately there's no quicker way other than the one you're using due to the way gtest are linked against the rest of the compiled code. However, a clobber shouldn't be needed unless it's mandated from updates in the code that you checked in. Also, there's no hard requirement to update your working tree while you're still working on this patch! 

Regarding the IDL questions:

- IDL is a generic language to describe objects. Code can be generated out of IDL definitions in various languages. Mozilla uses an IDL dialect called XPIDL. See https://developer.mozilla.org/en-US/docs/Mozilla/XPIDL
- Gecko uses the xpidl tool to produce code out of IDL files (https://developer.mozilla.org/en-US/docs/Mozilla/XPIDL/xpidl)
- These are some of the primitives available in IDL files: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Primitive

::: toolkit/components/telemetry/tests/gtest/TestHistograms.cpp
@@ +1,5 @@
> +/* vim:set ts=2 sw=2 sts=0 et: */
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */
> +

You're missing

#include "gtest/gtest.h"

before the other includes, that's why you're seeing the build errors.

@@ +7,5 @@
> +#include "nsITelemetry.h"
> +#include "Telemetry.h"
> +#include "TelemetryFixture.h"
> +
> +#define EXPECTED_VALUE 100

Don't use a macro here. Let's use a |const uint32_t|.

@@ +13,5 @@
> +TEST_F(TelemetryTestFixture, AccumulateCountHistogram)
> +{
> +  AutoJSContextWithGlobal cx(mCleanGlobal);
> +  
> +  JS::RootedValue snapshot(cx.GetJSContext()),

Please move the variable declaration closer to where they are use, for readability.

@@ +17,5 @@
> +  JS::RootedValue snapshot(cx.GetJSContext()),
> +                  testHistogram(cx.GetJSContext()),
> +                  rval(cx.GetJSContext());
> +  
> +  JS::MutableHandleValue histogram, sum;

These should probably be JS::RootedValue too.

@@ +19,5 @@
> +                  rval(cx.GetJSContext());
> +  
> +  JS::MutableHandleValue histogram, sum;
> +  
> +  ASSERT_TRUE(mTelemetry->GetHistogramById(

You cannot use ASSERT_TRUE here as GetHistogramById doesn't return a boolean. It returns a |nsresult| and you should check if the returned value is NS_OK. gtest offers the ASSERT_EQ macro that could be helpful for that.

@@ +24,5 @@
> +      NS_LITERAL_CSTRING("TELEMETRY_TEST_COUNT"),
> +      cx.GetJSContext(),
> +      &testHistogram)) << "Cannot fetch histogram";
> +  
> +  JS_CallFunctionName(cx.GetJSContext(), testHistogram, "clear",

|testHistogram| is expected to be a RootedObject rather than a RootedValue here. See: http://searchfox.org/mozilla-central/source/js/src/jsapi.cpp#2841

The good news is that you can get a RootedObject out of a RootedValue like this:

JS::RootedObject histogramObj(cx.GetJSContext(), &histogram.toObject());

@@ +25,5 @@
> +      cx.GetJSContext(),
> +      &testHistogram)) << "Cannot fetch histogram";
> +  
> +  JS_CallFunctionName(cx.GetJSContext(), testHistogram, "clear",
> +                      const JS::HandleValueArray::empty(), &rval);

There's no need for this |const| here. You should also wrap this function in an ASSERT_TRUE() as it returns a boolean.

@@ +29,5 @@
> +                      const JS::HandleValueArray::empty(), &rval);
> +  
> +  Telemetry::Accumulate(Telemetry::TELEMETRY_TEST_COUNT, EXPECTED_VALUE);
> +  
> +  ASSERT_TRUE(mTelemetry->GetHistogramByID(cx.GetJSContext(), &snapshot))

I don't think this call is needed.

@@ +33,5 @@
> +  ASSERT_TRUE(mTelemetry->GetHistogramByID(cx.GetJSContext(), &snapshot))
> +      << "Cannot call histogram snapshots";
> +  
> +  mTelemetry->GetHistogramSnapshots(cx.GetJSContext(), &snapshot);
> +  JS_GetProperty(cx.GetJSContext(), snapshot, "TELEMETRY_TEST_COUNT", &histogram);

JS_GetProperty axpects a RootedObject. You can get a RootedObject out of a Rootedvalue as described above.
Moreover, let's not remove the same variable.

@@ +34,5 @@
> +      << "Cannot call histogram snapshots";
> +  
> +  mTelemetry->GetHistogramSnapshots(cx.GetJSContext(), &snapshot);
> +  JS_GetProperty(cx.GetJSContext(), snapshot, "TELEMETRY_TEST_COUNT", &histogram);
> +  JS_GetProperty(cx.GetContext(), histogram, "sum", &sum);

This should be GetJSContext rather than GetContext. Also, histogram should be an object.

Comment 18

[Avikalpa Kundu [:kalpa]]

Attachment: 1312698.patch

Comment 19

[Avikalpa Kundu [:kalpa]]

(In reply to Alessio Placitelli [:Dexter] from comment #17)

> Don't use a macro here. Let's use a |const uint32_t|.

I used a |const int32_t| here since the compiler was complaining about comparing int with an unsigned int. It also complained if I simply compared without conversion to int32.

Hopefully this is not an issue. :-)

Comment 20

[Alessio Placitelli [:Dexter]]

Comment on attachment 8843460 [details] [diff] [review]
1312698.patch

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

Avikalpa, did you make sure this patch compiles and does what you're expecting? I saw that you dropped the histogram snapshotting part that was present in your previous patch. Was there any reason for doing that?

The test could use a bit more commenting. Also, please remove trailing whitespaces from your patch.

::: toolkit/components/telemetry/tests/gtest/TestHistograms.cpp
@@ +10,5 @@
> +#include "TelemetryFixture.h"
> +
> +TEST_F(TelemetryTestFixture, AccumulateCountHistogram)
> +{
> +  const int32_t EXPECTED_VALUE = 100;

Let's change this to the uint32_t type and make |sum| emit an uint32_t type as well below.

Also, please rename EXPECTED_VALUE to kExpectedValue.

@@ +12,5 @@
> +TEST_F(TelemetryTestFixture, AccumulateCountHistogram)
> +{
> +  const int32_t EXPECTED_VALUE = 100;
> +  AutoJSContextWithGlobal cx(mCleanGlobal);
> +  

Remove the trailing whitespaces here and below.

@@ +15,5 @@
> +  AutoJSContextWithGlobal cx(mCleanGlobal);
> +  
> +  JS::RootedValue testHistogram(cx.GetJSContext());
> +  JS::RootedValue rval(cx.GetJSContext());
> +  

Trailing whitespaces.

You could also add a short comment before the next line to explain what the next block does, e.g.

"// Get a reference to the test histogram and make sure it's empty."

@@ +16,5 @@
> +  
> +  JS::RootedValue testHistogram(cx.GetJSContext());
> +  JS::RootedValue rval(cx.GetJSContext());
> +  
> +  ASSERT_EQ(mTelemetry->GetHistogramById(NS_LITERAL_CSTRING("TELEMETRY_TEST_COUNT"),

Please define a constant variable to hold the name of the histogram and use it here and below. Place the constant at the beginning of the test, near the the expected value constant, e.g. const char* kHistogramName = ...

@@ +17,5 @@
> +  JS::RootedValue testHistogram(cx.GetJSContext());
> +  JS::RootedValue rval(cx.GetJSContext());
> +  
> +  ASSERT_EQ(mTelemetry->GetHistogramById(NS_LITERAL_CSTRING("TELEMETRY_TEST_COUNT"),
> +      cx.GetJSContext(), &testHistogram), NS_OK) << "Cannot fetch histogram";

nit: please align |cx.GetJSContext()| to the beginning of |mTelemetry->...| of the line above. Or, to make it more readable, you could save the return value of |GetHistogramById| and check that with ASSERT_EQ. For example:

nsresult rv = mTelemetry->GetHistogramById(...);
ASSERT_EQ(rv, NS_OK) << "Cannot fetch histogram".

@@ +21,5 @@
> +      cx.GetJSContext(), &testHistogram), NS_OK) << "Cannot fetch histogram";
> +
> +  JS::RootedObject testHistogramObj(cx.GetJSContext(), &testHistogram.toObject());
> +  ASSERT_TRUE(JS_CallFunctionName(cx.GetJSContext(), testHistogramObj,
> +        "clear", JS::HandleValueArray::empty(), &rval)) << "Cannot clear histogram";

Please align "clear" to |JS_CallFunctionName| on the line above.

@@ +23,5 @@
> +  JS::RootedObject testHistogramObj(cx.GetJSContext(), &testHistogram.toObject());
> +  ASSERT_TRUE(JS_CallFunctionName(cx.GetJSContext(), testHistogramObj,
> +        "clear", JS::HandleValueArray::empty(), &rval)) << "Cannot clear histogram";
> +
> +  Telemetry::Accumulate(Telemetry::TELEMETRY_TEST_COUNT, EXPECTED_VALUE);

nit: You could add a comment before this line explaining what you're doing, e.g. "// Increment the value stored in the histogram."

@@ +26,5 @@
> +
> +  Telemetry::Accumulate(Telemetry::TELEMETRY_TEST_COUNT, EXPECTED_VALUE);
> +
> +  JS::RootedValue snapshot(cx.GetJSContext());
> +  ASSERT_EQ(mTelemetry->GetHistogramById(NS_LITERAL_CSTRING("TELEMETRY_TEST_COUNT"),

Is this line needed? Is there any reason why we can't we use |testHistogram| in the lines below?

@@ +30,5 @@
> +  ASSERT_EQ(mTelemetry->GetHistogramById(NS_LITERAL_CSTRING("TELEMETRY_TEST_COUNT"),
> +        cx.GetJSContext(), &snapshot), NS_OK) << "Cannot call histogram snapshots";
> +
> +  JS::RootedValue histogram(cx.GetJSContext());
> +  JS::RootedObject snapshotObj(cx.GetJSContext(), &snapshot.toObject());

What happened to the |mTelemetry->GetHistogramSnapshots(cx.GetJSContext(), &snapshot);| line from the previous patch?
How are you getting the histogram snapshot?

@@ +32,5 @@
> +
> +  JS::RootedValue histogram(cx.GetJSContext());
> +  JS::RootedObject snapshotObj(cx.GetJSContext(), &snapshot.toObject());
> +  JS_GetProperty(cx.GetJSContext(), snapshotObj, "TELEMETRY_TEST_COUNT", &histogram);
> +  

Trailing whitespaces.

@@ +35,5 @@
> +  JS_GetProperty(cx.GetJSContext(), snapshotObj, "TELEMETRY_TEST_COUNT", &histogram);
> +  
> +  JS::RootedValue sum(cx.GetJSContext());
> +  JS::RootedObject histogramObj(cx.GetJSContext(), &histogram.toObject());
> +  JS_GetProperty(cx.GetJSContext(), histogramObj, "sum", &sum); 

Trailing whitespaces.

@@ +36,5 @@
> +  
> +  JS::RootedValue sum(cx.GetJSContext());
> +  JS::RootedObject histogramObj(cx.GetJSContext(), &histogram.toObject());
> +  JS_GetProperty(cx.GetJSContext(), histogramObj, "sum", &sum); 
> +  

Trailing whitespaces.

@@ +37,5 @@
> +  JS::RootedValue sum(cx.GetJSContext());
> +  JS::RootedObject histogramObj(cx.GetJSContext(), &histogram.toObject());
> +  JS_GetProperty(cx.GetJSContext(), histogramObj, "sum", &sum); 
> +  
> +  ASSERT_EQ(sum.toInt32(), EXPECTED_VALUE) << "The histogram is not returning expected value";

You could cast |sum.toInt32()| to uint32_t or use |JS::ToUint32| to get the unsigned value from sum.

Comment 21

[Alessio Placitelli [:Dexter]]

Please also add a commit message in the patch, e.g. "Bug 1312698 - Add test coverage for the C++ Histogram API. r?dexter"

Comment 22

[Avikalpa Kundu [:kalpa]]

Attachment: Bug 1312698 - Add test coverage for the C++ Histogram API. r?dexter

(In reply to Alessio Placitelli [:Dexter] from comment #20)

> Please define a constant variable to hold the name of the histogram and use it here and below. Place the constant at the beginning of the test, near the the expected value constant, e.g. const char* kHistogramName = ...

I've tried 5 ways, 

const char* kHistogramName = "TELEMETRY_TEST_COUNT";
...
NS_LITERAL_CSTRING(kHistogramName),

const char* kHistogramName = NS_LITERAL_CSTRING("TELEMETRY_TEST_COUNT");
...
kHistogramName,

const char* kHistogramName = "TELEMETRY_TEST_COUNT";
...
kHistogramName,

const nsACString_internal kHistogramName = NS_LITERAL_CSTRING("TELEMETRY_TEST_COUNT");
...
kHistogramName,

const nsACString_internal kHistogramName = "TELEMETRY_TEST_COUNT";
...
NS_LITERAL_CSTRING(kHistogramName),

In all 5 ways, compiler complains.

> What happened to the |mTelemetry->GetHistogramSnapshots(cx.GetJSContext(), &snapshot);| line from the previous patch?
How are you getting the histogram snapshot?

It was a mistake. Thanks for helping out.

Current Problem -> mozilla-unified/toolkit/components/telemetry/tests/gtest/TestHistograms.cpp:65:7: error: no member named 'ToUint32' in namespace 'JS'
 0:04.16   JS::ToUint32(cx.GetJSContext(), sum, &uSum);
 0:04.16   ~~~~^

Am I missing any |include|?

Comment 23

[Alessio Placitelli [:Dexter]]

Comment on attachment 8844029 [details] [diff] [review]
Bug 1312698 - Add test coverage for the C++ Histogram API. r?dexter

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

We're almost there, thank you for the updates. The patch looks good overall now and should be ok with the changes below.

Please don't forget to add a commit message to the patch, e.g. "Bug 1312698 - Add test coverage for the C++ Histogram API. r?dexter"

::: toolkit/components/telemetry/tests/gtest/TestHistograms.cpp
@@ +2,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */
> +
> +// Procedure to test any histogram in telemetry

nit: let's drop this comment from here (lines 6 to 13)

@@ +11,5 @@
> +// - Get the histogram from the snapshot by |JS_GetProperty|.
> +// - Get the required entry from the histogram by |JS_GetProperty|.
> +// - Test the entry from the histogram with |kExpectedValue|.
> +
> +#include "gtest/gtest.h"

ToUint32 is defined in Conversions.h. Just add 

#include "js/Conversions.h"

below this line.

@@ +25,5 @@
> +  JS::RootedValue testHistogram(cx.GetJSContext());
> +  JS::RootedValue rval(cx.GetJSContext());
> +
> +  // Get the histogram by |GetHistogramById|
> +  nsresult kFetchHistogram = mTelemetry->GetHistogramById(

Please don't use the "k" prefix for non-constants, as it should only be used with constants. See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Naming_and_Formatting_code

Change this to |nsresult rv = |.

@@ +31,5 @@
> +      cx.GetJSContext(),
> +      &testHistogram);
> +  ASSERT_EQ(kFetchHistogram, NS_OK) << "Cannot fetch histogram";
> +
> +  // Clear all previous entires by calling js method |clear| on it

nit: "Clear the stored value"

@@ +41,5 @@
> +      JS::HandleValueArray::empty(),
> +      &rval);
> +  ASSERT_TRUE(kClearHistogram) << "Cannot clear histogram";
> +
> +  // |Accumulate| entry in the histogram

nit: "Accumulate in the histogram"

@@ +44,5 @@
> +
> +  // |Accumulate| entry in the histogram
> +  Telemetry::Accumulate(Telemetry::TELEMETRY_TEST_COUNT, kExpectedValue);
> +
> +  // Take a snapshot of the histogram by |GetHistogramById|

nit: drop the "by |GetHistogramById|" part

@@ +46,5 @@
> +  Telemetry::Accumulate(Telemetry::TELEMETRY_TEST_COUNT, kExpectedValue);
> +
> +  // Take a snapshot of the histogram by |GetHistogramById|
> +  JS::RootedValue snapshot(cx.GetJSContext());
> +  nsresult kSnapshotHistogram = mTelemetry->GetHistogramSnapshots(cx.GetJSContext(), &snapshot);

You can use the |rv| variable defined above, so just call |rv = mTelemetry-> ...| here.

@@ +49,5 @@
> +  JS::RootedValue snapshot(cx.GetJSContext());
> +  nsresult kSnapshotHistogram = mTelemetry->GetHistogramSnapshots(cx.GetJSContext(), &snapshot);
> +  ASSERT_EQ(kSnapshotHistogram, NS_OK) << "Cannot call histogram snapshots";
> +
> +  // Get the histogram from the snapshot by |JS_GetProperty|

Change this comment to "Get a snapshot for all the histograms"

@@ +54,5 @@
> +  JS::RootedValue histogram(cx.GetJSContext());
> +  JS::RootedObject snapshotObj(cx.GetJSContext(), &snapshot.toObject());
> +  JS_GetProperty(cx.GetJSContext(), snapshotObj, "TELEMETRY_TEST_COUNT", &histogram);
> +
> +  // Get the required entry from the histogram by |JS_GetProperty|

Change this comment to "Get the snapshot for the test histogram".

@@ +59,5 @@
> +  JS::RootedValue sum(cx.GetJSContext());
> +  JS::RootedObject histogramObj(cx.GetJSContext(), &histogram.toObject());
> +  JS_GetProperty(cx.GetJSContext(), histogramObj, "sum", &sum);
> +
> +  // Test the entry from the histogram with |kExpectedValue|

Change this to "Check that the value stored in the histogram matches with |kExpectedValue|".

@@ +60,5 @@
> +  JS::RootedObject histogramObj(cx.GetJSContext(), &histogram.toObject());
> +  JS_GetProperty(cx.GetJSContext(), histogramObj, "sum", &sum);
> +
> +  // Test the entry from the histogram with |kExpectedValue|
> +  uint32_t* uSum;

This should be |uint32_t uSum = 0;|, not a pointer.

@@ +62,5 @@
> +
> +  // Test the entry from the histogram with |kExpectedValue|
> +  uint32_t* uSum;
> +  JS::ToUint32(cx.GetJSContext(), sum, &uSum);
> +  ASSERT_EQ(*uSum, kExpectedValue) << "The histogram is not returning expected value";

You don't need to dereference uSum here, so change this to be |ASSERT_EQ(uSum...|

Comment 24

[Avikalpa Kundu [:kalpa]]

Attachment: Bug 1312698 - Add test coverage for the C++ Histogram API. r?dexter

Comment 25

[Alessio Placitelli [:Dexter]]

Comment on attachment 8844586 [details] [diff] [review]
Bug 1312698 - Add test coverage for the C++ Histogram API. r?dexter

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

This looks much better now, however the patch doesn't contain any commit message.

To change the commit message, with the patch applied, execute "hg qref -e".

Once you've updated the complete patch, I'll take another look but I think we should be good to go!

::: toolkit/components/telemetry/tests/gtest/TestHistograms.cpp
@@ +25,5 @@
> +  ASSERT_EQ(rv, NS_OK) << "Cannot fetch histogram";
> +
> +  // Clear the stored value
> +  JS::RootedObject testHistogramObj(cx.GetJSContext(), &testHistogram.toObject());
> +  bool kClearHistogram = JS_CallFunctionName(

Let's not use the "k" prefix for non-constants. Moreover, since JS_CallFunctionName returns a boolean, just do like this:

> ASSERT_TRUE(JS_CallFunctionName( ...

We're doing it differently for other calls because they return nsresult.

@@ +51,5 @@
> +  JS::RootedValue sum(cx.GetJSContext());
> +  JS::RootedObject histogramObj(cx.GetJSContext(), &histogram.toObject());
> +  JS_GetProperty(cx.GetJSContext(), histogramObj, "sum", &sum);
> +
> +  // Test the entry from the histogram with |kExpectedValue|

As mentioned, please change this to "Check that the value stored in the histogram matches with |kExpectedValue|".

Comment 26

[Avikalpa Kundu [:kalpa]]

Attachment: 1312698.patch

Comment 27

[Alessio Placitelli [:Dexter]]

Comment on attachment 8844842 [details] [diff] [review]
1312698.patch

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

This looks good now, thanks. Let me push it to try for you: we need to make sure it works on all the supported platforms.

Comment 28

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2caa1f2e8221

Comment 29

[Alessio Placitelli [:Dexter]]

Looks like all tests are green, we're good to land this! Congrats Avikalpa!

Comment 30

[Avikalpa Kundu [:kalpa]]

Thanks for all the help along the way!

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fccd4aa0100f
Add test coverage for the C++ Histogram API. r=dexter

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fccd4aa0100f