Closed Bug 1305648 Opened 5 years ago Closed 5 years ago

Add test coverage for the C++ Scalar API

Categories

(Toolkit :: Telemetry, defect, P1)

defect
Points:
3

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: Dexter, Assigned: Dexter)

References

(Blocks 1 open bug)

Details

(Whiteboard: [measurement:client])

Attachments

(1 file, 7 obsolete files)

As bug 1305577 points out, we really need test coverage for the C++ API in Telemetry. We can start with the Scalars API and then move on to the other APIs.
Blocks: 1275517
Points: --- → 3
Priority: -- → P3
Whiteboard: [measurement:client]
After discussing this with :gfritzsche, it turns out we have some nice docs about adding C++ test coverage [1]. That guide states that the preferred way to write tests is using GTests [2], so we should go for that.

One nice example of GTests in toolkit can be found at [3]. The DXR search at [4] reveals many other examples.

[1] - https://developer.mozilla.org/en-US/docs/Compiled-code_automated_tests
[2] - https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/GTest
[3] - https://dxr.mozilla.org/mozilla-central/source/toolkit/profile/gtest/TestProfileLock.cpp
[4] - https://dxr.mozilla.org/mozilla-central/search?q=%22%23include+%5C%22gtest%2Fgtest.h%5C%22%22+path%3Atoolkit&redirect=false
Priority: P3 → P2
Priority: P2 → P1
Assignee: nobody → alessio.placitelli
Attached patch bug1305648.patch (obsolete) — Splinter Review
I'm not flagging for review just yet, as this patch crashes at [1] when taking a snapshot in the "ScalarString" test.

[1] - https://dxr.mozilla.org/mozilla-central/rev/e3279760cd977aac30bd9e8032d3ee71f55d2a67/js/src/jsapi.cpp#802
The crash seems to be triggered by this call [1] when taking a snapshot, after a string scalar is set.
It crashes because |zone| is null when [2] is hit.

I have no idea why this is null and how to make it non null.

[1] - https://dxr.mozilla.org/mozilla-central/rev/e3279760cd977aac30bd9e8032d3ee71f55d2a67/toolkit/components/telemetry/TelemetryScalar.cpp#1785
[2] - https://dxr.mozilla.org/mozilla-central/rev/e3279760cd977aac30bd9e8032d3ee71f55d2a67/js/src/jsapi.cpp#802
Status: NEW → ASSIGNED
Jeff, do you have any clue about the crash from comment 3?
I'm setting up the JS environment for the tests in TelemetryFixture.h:SetUp()
Flags: needinfo?(jwalden+bmo)
I've solved the problem, thanks anyway Jeff!
Flags: needinfo?(jwalden+bmo)
Attached patch bug1305648.patch (obsolete) — Splinter Review
Georg, this works now, thanks to :bz. I've refactored the code a bit, and I think it's ready for prime time!
Attachment #8805968 - Attachment is obsolete: true
Attachment #8808104 - Flags: review?(gfritzsche)
Comment on attachment 8808104 [details] [diff] [review]
bug1305648.patch

Hi h4writer! This patch adds C++ API test coverage to Telemetry.

Would you kindly take a look at the test setup JS-bits of this patch? That basically means sanity checking the JS setup bits in TelemetryFixture.h. For some background, an instance of the TelemetryTestFixture class is instantiated with each test.

The rest of the patch will be reviewed by Georg Fritzsche.
Attachment #8808104 - Flags: feedback?(hv1989)
Attached patch bug1305648.patch (obsolete) — Splinter Review
I've updated the code thanks to the feedback from the #jsapi folks.
Attachment #8808104 - Attachment is obsolete: true
Attachment #8808104 - Flags: review?(gfritzsche)
Attachment #8808104 - Flags: feedback?(hv1989)
Attachment #8810326 - Flags: review?(gfritzsche)
Comment on attachment 8810326 [details] [diff] [review]
bug1305648.patch

r=me on the TelemetryFixture.h bits, assuming the naming makes sense, if you change it to fail if Init() fails.
Attachment #8810326 - Flags: review+
Comment on attachment 8810326 [details] [diff] [review]
bug1305648.patch

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

::: toolkit/components/telemetry/tests/gtest/TestScalars.cpp
@@ +16,5 @@
> +
> +#define EXPECTED_STRING "Nice, expected and creative string."
> +
> +void
> +CheckUintScalar(const char* aName, JSContext* aCx, JS::HandleValue aSnapshot, uint32_t expectedValue)

Nit: You can put all the functions in this file in an unnamed namespace (or make them static).
This means they have internal linkage and are not visible or linkable externally.

@@ +23,5 @@
> +  JS::RootedValue value(aCx);
> +  JS::Rooted<JSObject*> scalarObj(aCx, &aSnapshot.toObject());
> +  ASSERT_TRUE(JS_GetProperty(aCx, scalarObj, aName, &value)) << "The test scalar must be reported.";
> +  ASSERT_TRUE(value.isInt32()) << "The scalar value must be of the correct type.";
> +  ASSERT_EQ(static_cast<uint32_t>(value.toInt32()), expectedValue) << "The scalar value must match the expected value.";

Why not return the value instead and make it `GetUintScalar()` etc.?
That way the individual test function can decide what to check (equal, greater or equal, ...).

Also, please check that the value is >=0 before casting to uint.

@@ +78,5 @@
> +  CheckBoolScalar(aKey, aCx, keyedScalar, expectedValue);
> +}
> +
> +void
> +GetScalarsSnapshot(bool aKeyed, JSContext* aCx, JS::MutableHandle<JS::Value> aResult)

You can return the result directly from this function, no need for out-params.

@@ +110,5 @@
> +  // Set the test scalar to a known value.
> +  const uint32_t kInitialValue = 1172015;
> +  const uint32_t kExpectedUint = 1172017;
> +  Telemetry::ScalarSet(Telemetry::ScalarID::TELEMETRY_TEST_UNSIGNED_INT_KIND, kInitialValue);
> +  Telemetry::ScalarAdd(Telemetry::ScalarID::TELEMETRY_TEST_UNSIGNED_INT_KIND, 2);

We have 3 hard-coded numbers now, while you only need two.
E.g.:
initial = 42;
expected = initial + 2;
...
add(expected - initial);

@@ +112,5 @@
> +  const uint32_t kExpectedUint = 1172017;
> +  Telemetry::ScalarSet(Telemetry::ScalarID::TELEMETRY_TEST_UNSIGNED_INT_KIND, kInitialValue);
> +  Telemetry::ScalarAdd(Telemetry::ScalarID::TELEMETRY_TEST_UNSIGNED_INT_KIND, 2);
> +
> +  // Take a snapshot of the scalars.

"Check the recorded value." or so?
Ditto the comment below and in the other functions.

@@ +119,5 @@
> +  CheckUintScalar("telemetry.test.unsigned_int_kind", mContext, scalarsSnapshot, kExpectedUint);
> +
> +  // Try to use SetMaximum.
> +  const uint32_t kExpectedUintMaximum = kExpectedUint * 2;
> +  Telemetry::ScalarSetMaximum(Telemetry::ScalarID::TELEMETRY_TEST_UNSIGNED_INT_KIND, kExpectedUintMaximum);

What about testing with unsupported types for this histogram?

@@ +131,5 @@
> +TEST_F(TelemetryTestFixture, ScalarBoolean) {
> +  Unused << mTelemetry->ClearScalars();
> +
> +  // Set the test scalar to a known value.
> +  Telemetry::ScalarSet(Telemetry::ScalarID::TELEMETRY_TEST_BOOLEAN_KIND, true);

Also test Add, SetMaximum, Set(uint), Set(string)?
Ditto on extended coverage for the following functions.

@@ +183,5 @@
> +
> +  GetScalarsSnapshot(true, mContext, &scalarsSnapshot);
> +  // The first key should be different and te second is expected to be the same.
> +  CheckKeyedUintScalar(kScalarName, "key1", mContext, scalarsSnapshot, kExpectedUintMaximum);
> +  CheckKeyedUintScalar(kScalarName, "key2", mContext, scalarsSnapshot, kKey2Value);

What about checking that only the given keys above were recorded?
If we have a `GetKeyedUintScalar()` function, it could return e.g. an std::map<nsCString,ScalarType>, which would make it easy to test things here.

@@ +203,5 @@
> +  // Make sure that the keys contain the expected values.
> +  const char* kScalarName = "telemetry.test.keyed_boolean_kind";
> +  CheckKeyedBoolScalar(kScalarName, "key1", mContext, scalarsSnapshot, false);
> +  CheckKeyedBoolScalar(kScalarName, "key2", mContext, scalarsSnapshot, true);
> +}

Also keyed string scalars?
Attachment #8810326 - Flags: review?(gfritzsche) → feedback+
(In reply to Georg Fritzsche [:gfritzsche] from comment #10)
> Comment on attachment 8810326 [details] [diff] [review]
> bug1305648.patch
> 
> Review of attachment 8810326 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +23,5 @@
> > +  JS::RootedValue value(aCx);
> > +  JS::Rooted<JSObject*> scalarObj(aCx, &aSnapshot.toObject());
> > +  ASSERT_TRUE(JS_GetProperty(aCx, scalarObj, aName, &value)) << "The test scalar must be reported.";
> > +  ASSERT_TRUE(value.isInt32()) << "The scalar value must be of the correct type.";
> > +  ASSERT_EQ(static_cast<uint32_t>(value.toInt32()), expectedValue) << "The scalar value must match the expected value.";
> 
> Why not return the value instead and make it `GetUintScalar()` etc.?
> That way the individual test function can decide what to check (equal,
> greater or equal, ...).

As discussed over IRC, unfortunately, this is not possible, as we cannot use ASSERT_* gtest macros in non-void returning functions, as mentioned here: https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#my-compiler-complains-void-value-not-ignored-as-it-ought-to-be-what-does-this-mean

> @@ +119,5 @@
> > +  CheckUintScalar("telemetry.test.unsigned_int_kind", mContext, scalarsSnapshot, kExpectedUint);
> > +
> > +  // Try to use SetMaximum.
> > +  const uint32_t kExpectedUintMaximum = kExpectedUint * 2;
> > +  Telemetry::ScalarSetMaximum(Telemetry::ScalarID::TELEMETRY_TEST_UNSIGNED_INT_KIND, kExpectedUintMaximum);
> 
> What about testing with unsupported types for this histogram?

Do you mean testing, for example, a float with an unsigned scalar?
Shouldn't this type of issues be handled by the casting/type enforcing of the C++ API?

> @@ +131,5 @@
> > +TEST_F(TelemetryTestFixture, ScalarBoolean) {
> > +  Unused << mTelemetry->ClearScalars();
> > +
> > +  // Set the test scalar to a known value.
> > +  Telemetry::ScalarSet(Telemetry::ScalarID::TELEMETRY_TEST_BOOLEAN_KIND, true);
> 
> Also test Add, SetMaximum, Set(uint), Set(string)?
> Ditto on extended coverage for the following functions.

Using unsupported functions for a particular scalar type (e.g. setting a uint scalar to a string), throws, as per https://dxr.mozilla.org/mozilla-central/rev/47f42f21541b9b98ad7db82edb996b29065debd0/toolkit/components/telemetry/TelemetryScalar.cpp#184

In general, gtest would allow us to expect an exception with ASSERT_ANY_THROW. Unfortunately, our builds have exception handling disabled, so that's a no-go :(

> @@ +183,5 @@
> > +
> > +  GetScalarsSnapshot(true, mContext, &scalarsSnapshot);
> > +  // The first key should be different and te second is expected to be the same.
> > +  CheckKeyedUintScalar(kScalarName, "key1", mContext, scalarsSnapshot, kExpectedUintMaximum);
> > +  CheckKeyedUintScalar(kScalarName, "key2", mContext, scalarsSnapshot, kKey2Value);
> 
> What about checking that only the given keys above were recorded?
> If we have a `GetKeyedUintScalar()` function, it could return e.g. an
> std::map<nsCString,ScalarType>, which would make it easy to test things here.

I guess we could do that, but this part is already thoroughly covered in the JS tests. Since the recording engine is the same (only the API is changing), is it worth adding it here too?

> @@ +203,5 @@
> > +  // Make sure that the keys contain the expected values.
> > +  const char* kScalarName = "telemetry.test.keyed_boolean_kind";
> > +  CheckKeyedBoolScalar(kScalarName, "key1", mContext, scalarsSnapshot, false);
> > +  CheckKeyedBoolScalar(kScalarName, "key2", mContext, scalarsSnapshot, true);
> > +}
> 
> Also keyed string scalars?

We don't support keyed scalars, as per bug 1277806 comment 2.
Flags: needinfo?(gfritzsche)
(In reply to Alessio Placitelli [:Dexter] from comment #11)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #10)
> > @@ +119,5 @@
> > > +  CheckUintScalar("telemetry.test.unsigned_int_kind", mContext, scalarsSnapshot, kExpectedUint);
> > > +
> > > +  // Try to use SetMaximum.
> > > +  const uint32_t kExpectedUintMaximum = kExpectedUint * 2;
> > > +  Telemetry::ScalarSetMaximum(Telemetry::ScalarID::TELEMETRY_TEST_UNSIGNED_INT_KIND, kExpectedUintMaximum);
> > 
> > What about testing with unsupported types for this histogram?
> 
> Do you mean testing, for example, a float with an unsigned scalar?
> Shouldn't this type of issues be handled by the casting/type enforcing of
> the C++ API?

Right, for SetMaximum there is probably nothing else to test.

> > @@ +131,5 @@
> > > +TEST_F(TelemetryTestFixture, ScalarBoolean) {
> > > +  Unused << mTelemetry->ClearScalars();
> > > +
> > > +  // Set the test scalar to a known value.
> > > +  Telemetry::ScalarSet(Telemetry::ScalarID::TELEMETRY_TEST_BOOLEAN_KIND, true);
> > 
> > Also test Add, SetMaximum, Set(uint), Set(string)?
> > Ditto on extended coverage for the following functions.
> 
> Using unsupported functions for a particular scalar type (e.g. setting a
> uint scalar to a string), throws, as per
> https://dxr.mozilla.org/mozilla-central/rev/
> 47f42f21541b9b98ad7db82edb996b29065debd0/toolkit/components/telemetry/
> TelemetryScalar.cpp#184

The C++ API doesn't throw, only the JS API.
We should make sure that we get the expected observable behavior when e.g. using:
ScalarSet(UINT_SCALAR, "some string");

The value of the scalar should not change here.

> > @@ +183,5 @@
> > > +
> > > +  GetScalarsSnapshot(true, mContext, &scalarsSnapshot);
> > > +  // The first key should be different and te second is expected to be the same.
> > > +  CheckKeyedUintScalar(kScalarName, "key1", mContext, scalarsSnapshot, kExpectedUintMaximum);
> > > +  CheckKeyedUintScalar(kScalarName, "key2", mContext, scalarsSnapshot, kKey2Value);
> > 
> > What about checking that only the given keys above were recorded?
> > If we have a `GetKeyedUintScalar()` function, it could return e.g. an
> > std::map<nsCString,ScalarType>, which would make it easy to test things here.
> 
> I guess we could do that, but this part is already thoroughly covered in the
> JS tests. Since the recording engine is the same (only the API is changing),
> is it worth adding it here too?

There are still different code paths here, so it would be great to at least cover "set of keys for scalar matches exactly".
Flags: needinfo?(gfritzsche)
Attached patch bug1305648.patch (obsolete) — Splinter Review
Attachment #8810326 - Attachment is obsolete: true
Attachment #8814900 - Flags: review?(gfritzsche)
Comment on attachment 8814900 [details] [diff] [review]
bug1305648.patch

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

I still think the value checking could be handled better (have JS object handling code and value checking clearly separated), but this is test code and we can improve that when we revisit this.

::: toolkit/components/telemetry/tests/gtest/TestScalars.cpp
@@ +11,5 @@
> +#include "nsITelemetry.h"
> +#include "Telemetry.h"
> +#include "TelemetryFixture.h"
> +
> +#include <map>

This is unused.

@@ +24,5 @@
> +CheckUintScalar(const char* aName, JSContext* aCx, JS::HandleValue aSnapshot, uint32_t expectedValue)
> +{
> +  // Validate the value of the test scalar.
> +  JS::RootedValue value(aCx);
> +  JS::Rooted<JSObject*> scalarObj(aCx, &aSnapshot.toObject());

Nit: JS::RootedObject
Ditto below.
Attachment #8814900 - Flags: review?(gfritzsche) → review+
Attached patch bug1305648.patch (obsolete) — Splinter Review
This also fixes a bustage on try due to a wrong casting of the "test" string to bool.
Attachment #8814900 - Attachment is obsolete: true
Attachment #8815292 - Flags: review+
Attached patch bug1305648.patch (obsolete) — Splinter Review
There were some failures on try due to the static analysis. I've slightly changed the patch to address that.
Attachment #8815292 - Attachment is obsolete: true
Attachment #8815769 - Flags: feedback?(gfritzsche)
Comment on attachment 8815769 [details] [diff] [review]
bug1305648.patch

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

::: toolkit/components/telemetry/tests/gtest/TelemetryFixture.h
@@ +36,5 @@
> +// AutoJSAPI is annotated with MOZ_STACK_CLASS and thus cannot be
> +// used as a member of TelemetryTestFixture, since gtest instantiates
> +// that on the heap. To work around the problem, we need to call the
> +// following macro at the beginning of each Telemetry test.
> +#define GTEST_INIT_JSAPI(globalObj) \

Please let's try to avoid macros where possible.
Here it doesn't seem to win us much over a small function or helper class.
Attachment #8815769 - Flags: feedback?(gfritzsche)
Attached patch bug1305648.patch (obsolete) — Splinter Review
I've removed the macro and made sure that everything looks fine on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=274a76e5226aa7249df042620b9f6f1a74b0e532
Attachment #8815769 - Attachment is obsolete: true
Attachment #8818534 - Flags: review?(gfritzsche)
Comment on attachment 8818534 [details] [diff] [review]
bug1305648.patch

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

::: toolkit/components/telemetry/tests/gtest/TelemetryFixture.h
@@ +42,5 @@
> +// global JS object in.
> +class MOZ_RAII AutoJSContextWithGlobal {
> +public:
> +  explicit AutoJSContextWithGlobal(JSObject* aGlobalObject);
> +  operator JSContext*() const;

We should avoid implicit conversions where possible.
Lets make this an explicit method, `getJSContext()` or so.

@@ +54,5 @@
> +  : mCx(nullptr)
> +{
> +  // The JS API must initialize correctly.
> +  MOZ_ALWAYS_TRUE(mJsAPI.Init(aGlobalObject));
> +  mCx = mJsAPI.cx();

Could you not just return `mJsAPI.cx()` in the `getJSContext()` method below instead of using a member?
Attachment #8818534 - Flags: review?(gfritzsche) → review+
Attached patch bug1305648.patchSplinter Review
Attachment #8818534 - Attachment is obsolete: true
Attachment #8819835 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/9be35cfb0e2e
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Blocks: 1345861
No longer blocks: 1345861
You need to log in before you can comment on or make changes to this bug.