1305648 - Add test coverage for the C++ Scalar API

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Alessio Placitelli [:Dexter]]

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.

Comment 1

[Alessio Placitelli [:Dexter]]

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

Comment 2

[Alessio Placitelli [:Dexter]]

Attachment: bug1305648.patch

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

Comment 3

[Alessio Placitelli [:Dexter]]

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

Comment 4

[Alessio Placitelli [:Dexter]]

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

Comment 5

[Alessio Placitelli [:Dexter]]

I've solved the problem, thanks anyway Jeff!

Comment 6

[Alessio Placitelli [:Dexter]]

Attachment: bug1305648.patch

Georg, this works now, thanks to :bz. I've refactored the code a bit, and I think it's ready for prime time!

Comment 7

[Alessio Placitelli [:Dexter]]

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.

Comment 8

[Alessio Placitelli [:Dexter]]

Attachment: bug1305648.patch

I've updated the code thanks to the feedback from the #jsapi folks.

Comment 9

[Boris Zbarsky [:bzbarsky]]

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.

Comment 10

[Georg Fritzsche [:gfritzsche]]

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?

Comment 11

[Alessio Placitelli [:Dexter]]

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

Comment 12

[Georg Fritzsche [: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".

Comment 13

[Alessio Placitelli [:Dexter]]

Attachment: bug1305648.patch

Comment 14

[Georg Fritzsche [: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.

Comment 15

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=45d7208375ab

Comment 16

[Alessio Placitelli [:Dexter]]

Attachment: bug1305648.patch

This also fixes a bustage on try due to a wrong casting of the "test" string to bool.

Comment 17

[Alessio Placitelli [:Dexter]]

Attachment: bug1305648.patch

There were some failures on try due to the static analysis. I've slightly changed the patch to address that.

Comment 18

[Alessio Placitelli [:Dexter]]

Try push for the latest patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3073154419fe265d5338b57bd614c7f9f6d058f1

Comment 19

[Georg Fritzsche [: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.

Comment 20

[Alessio Placitelli [:Dexter]]

Attachment: bug1305648.patch

I've removed the macro and made sure that everything looks fine on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=274a76e5226aa7249df042620b9f6f1a74b0e532

Comment 21

[Georg Fritzsche [: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?

Comment 22

[Alessio Placitelli [:Dexter]]

Attachment: bug1305648.patch

Comment 23

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9be35cfb0e2e66aa906b556fe9b8e6d8824c347e
Bug 1305648 - Add test coverage for the C++ Scalar API. r=gfritzsche,bz

Comment 24

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

https://hg.mozilla.org/mozilla-central/rev/9be35cfb0e2e