1355498 - Refactor commonly used actions in functions within TestHistograms.cpp

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P3)

Description

[Alessio Placitelli [:Dexter]]

Sice we're using the same patterns in Histogram cpp tests, we could make them stand alone functions so other tests can easily re-use them without copy pasting the same code over and over.

(In reply to Alessio Placitelli [:Dexter] from bug 1345861 comment #13)
> Comment on attachment 8856588 [details] [diff] [review]
> 1345861.patch
> 
> Review of attachment 8856588 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ... this patch is basically reusing the majority of the
> very same code from your first test. This suggests that we can probably do
> some refactoring and provide the shared functionalities between the two
> tests as utility functions. I can spot at least 2 potentially refactoring
> points:
> 
> - We could have a function that gets an histogram and clears it. We could
> call it |GetAndClearHistogram|.
> - We could have a function that takes a snapshot of the histograms and
> returns a copy of the histogram data (e.g. |GetHistogramSnapshot|)

Comment 1

[Georg Fritzsche [:gfritzsche]]

Can you set this up for mentoring?

Comment 2

[Alessio Placitelli [:Dexter]]

I'll add more details later, this is already on Avikalpa's radar to pick up after bug 1345861 lands.

Comment 3

[Alessio Placitelli [:Dexter]]

Avikalpa, are you interested in working on this bug, as you already have context on it?

Comment 4

[Avikalpa Kundu [:kalpa]]

Yes. Will do so.

Thanks.

Comment 5

[Avikalpa Kundu [:kalpa]]

Attachment: 1355498.patch

|mTelemetry| is not recognized for some reason.

I'm unable to diagnose the problem.
Am I missing any |using Telemetry| includes?

Thanks.

Comment 6

[Alessio Placitelli [:Dexter]]

Comment on attachment 8857430 [details] [diff] [review]
1355498.patch

I won't get into the details there for now, let's wait for the patch to compile and to its job first :-)

As for the |mTelemetry|, it is from the test fixture used in the test cases, see the definition of TelemetryTestFixture in [1]. It's a pointer to an instance of nsITelemetry, wrapped in nsCOMPtr.

The nsITelemetry instance represents a singleton service within Telemetry, so you can simply get the same service in your functions, where needed, like that:

> nsCOMPtr<nsITelemetry> telemetry = do_GetService("@mozilla.org/base/telemetry;1");



[1] - https://dxr.mozilla.org/mozilla-central/rev/f40e24f40b4c4556944c762d4764eace261297f5/toolkit/components/telemetry/tests/gtest/TelemetryFixture.h#21

Comment 7

[Avikalpa Kundu [:kalpa]]

Attachment: 1355498.patch

I can't seem to get |mTelemetry| to work as expected.
Can you look into the problem ?

Comment 8

[Alessio Placitelli [:Dexter]]

W(In reply to Avikalpa Kundu [:kalpa] from comment #7)
> Created attachment 8862363 [details] [diff] [review]
> 1355498.patch
> 
> I can't seem to get |mTelemetry| to work as expected.
> Can you look into the problem ?

Would you mind expanding on what's happening?

Comment 9

[Avikalpa Kundu [:kalpa]]

The test is failing in if I create a single global instance or multiple instances of |nsCOMPtr<nsITelemetry>|.
I can't seem to understand the issue.

The patch has a global instance of |mTelemetry| and it failed as somehow|rv != NS_OK|.
The previous patch had multiple instances of |mTelemetry| and it was failing with same reason.

I can't seem to debug this issue.

Comment 10

[Alessio Placitelli [:Dexter]]

(In reply to Avikalpa Kundu [:kalpa] from comment #9)
> The test is failing in if I create a single global instance or multiple
> instances of |nsCOMPtr<nsITelemetry>|.
> I can't seem to understand the issue.
> 
> The patch has a global instance of |mTelemetry| and it failed as somehow|rv
> != NS_OK|.
> The previous patch had multiple instances of |mTelemetry| and it was failing
> with same reason.
> 
> I can't seem to debug this issue.

If that's creating issues, you can simply add a new parameter to your function to pass mTelemetry as an argument.

> GetAndClearHistogram(JSContext* cx, nsCOMPtr<nsITelemetry> telemetry, const nsACString &name)

Comment 11

[Avikalpa Kundu [:kalpa]]

Attachment: 1355498.patch

Same problem persists.

/mozilla/mozillaunified/toolkit/components/telemetry/tests/gtest/TestHistograms.cpp:23: Failure
      Expected: rv
      Which is: 4-byte object <05-40 00-80>
To be equal to: NS_OK
      Which is: 4-byte object <00-00 00-00>
Cannot fetch histogram

Comment 12

[Alessio Placitelli [:Dexter]]

(In reply to Avikalpa Kundu [:kalpa] from comment #11)
> Created attachment 8863734 [details] [diff] [review]
> 1355498.patch
> 
> Same problem persists.
> 
> /mozilla/mozillaunified/toolkit/components/telemetry/tests/gtest/
> TestHistograms.cpp:23: Failure
>       Expected: rv
>       Which is: 4-byte object <05-40 00-80>
> To be equal to: NS_OK
>       Which is: 4-byte object <00-00 00-00>
> Cannot fetch histogram

That last message is telling you what's wrong. It says that's expecting an NS_OK, but got something else, when doing the error check here:

>  nsresult rv = mTelemetry->GetHistogramById(name, cx, &testHistogram);
>  ASSERT_EQ(rv, NS_OK) << "Cannot fetch histogram";

If you take a closer look at the code that you changed, you'll notice that this problem is triggered by the second test (AccumulateKeyedCountHistogram). That's happening because, to get keyed histograms, you need to call |GetKeyedHistogramById| and not |GetHistogramById|.

To account for that, you could add a new boolean parameter to |GetAndClearHistogram|, |bool keyed|, to decide what to call.

When I find similar problem, if I can't spot the error by going through the code again, I end up commenting instructions until I obtain the least possible amount of code to reproduce the error. And then investigate from there. Next time you face a similar issue, try to do this and see if you manage to figure out what's wrong :-)

Comment 13

[Avikalpa Kundu [:kalpa]]

Attachment: 1355498.patch

> ./mach gtest 'Telemetry*'

Running GTest tests...
Note: Google Test filter = Telemetry*
[==========] Running 9 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 9 tests from TelemetryTestFixture
[ RUN      ] TelemetryTestFixture.AccumulateCountHistogram
[71918] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520012: file /mozilla/mozilla-unified/extensions/cookie/nsPermissionManager.cpp, line 2723
Assertion failure: mRawPtr != nullptr (You can't dereference a NULL nsCOMPtr with operator->().), at /mozilla/mozilla-unified/obj-ff-dbg/dist/include/nsCOMPtr.h:763
#01: TelemetryTestFixture_AccumulateCountHistogram_Test::TestBody()[/mozilla/mozilla-unified/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/gtest/XUL +0x48aa16a]
#02: testing::Test::Run()[/mozilla/mozilla-unified/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/gtest/XUL +0x45b7d51]
#03: testing::TestInfo::Run()[/mozilla/mozilla-unified/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/gtest/XUL +0x45b8c00]
#04: testing::TestCase::Run()[/mozilla/mozilla-unified/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/gtest/XUL +0x45b9033]
#05: testing::internal::UnitTestImpl::RunAllTests()[/mozilla/mozilla-unified/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/gtest/XUL +0x45c1bdb]
#06: testing::UnitTest::Run()[/mozilla/mozilla-unified/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/gtest/XUL +0x45c172d]
#07: mozilla::RunGTestFunc(int*, char**)[/mozilla/mozilla-unified/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/gtest/XUL +0x45ce798]
#08: XREMain::XRE_mainStartup(bool*)[/mozilla/mozilla-unified/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/gtest/XUL +0x3fc913e]
#09: XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&)[/mozilla/mozilla-unified/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/gtest/XUL +0x3fcf247]
#10: XRE_main(int, char**, mozilla::BootstrapConfig const&)[/mozilla/mozilla-unified/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/gtest/XUL +0x3fcf8a9]
#11: main[/mozilla/mozilla-unified/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/firefox +0xe78]

> Assertion failure: mRawPtr != nullptr (You can't dereference a NULL nsCOMPtr with operator->().), at /mozilla/mozilla-unified/obj-ff-dbg/dist/include/nsCOMPtr.h:763

Why is mTelemetry a |nullPtr|?
Should I do something after assigning to |do_GetService("@mozilla.org/base/telemetry;1")|?

PS: I probably would'nt get the error if I pass |mTelemetry| as a parameter to the functions. Then perhaps everything would go as fine.
But I wanted to know what's wrong.

Comment 14

[Alessio Placitelli [:Dexter]]

(In reply to Avikalpa Kundu [:kalpa] from comment #13)
> > Assertion failure: mRawPtr != nullptr (You can't dereference a NULL nsCOMPtr with operator->().), at /mozilla/mozilla-unified/obj-ff-dbg/dist/include/nsCOMPtr.h:763
> 
> Why is mTelemetry a |nullPtr|?
> Should I do something after assigning to
> |do_GetService("@mozilla.org/base/telemetry;1")|?
> 
> PS: I probably would'nt get the error if I pass |mTelemetry| as a parameter
> to the functions. Then perhaps everything would go as fine.
> But I wanted to know what's wrong.

The Telemetry service is only available within the test case/Telemetry fixture. So within the:

> TEST_F(TelemetryTestFixture, AccumulateCountHistogram)
> {
>  // here mTelemetry is valid
> }

You wouldn't want to add that as a global in the cpp file. You can either pass it as a parameter or request it within each function, assuming that functions are only called within the test cases.

The only way to enforce the previous assumption seems to be allowing passing mTelemetry to them.

Comment 15

[Avikalpa Kundu [:kalpa]]

I was incorrect in assuming if I pass |mTelemetry| as a parameter to the functions then perhaps everything would go as fine.

Instead I got the same error msg. as in comment #13

(In reply to Alessio Placitelli [:Dexter] from comment #13)
> The Telemetry service is only available within the test case/Telemetry fixture.
I was just wondering how can I call the Telemetryservice globally?

Comment 16

[Avikalpa Kundu [:kalpa]]

Attachment: 1355498.patch

Oh! I forgot the patch.

Comment 17

[Alessio Placitelli [:Dexter]]

(In reply to Avikalpa Kundu [:kalpa] from comment #15)
> I was incorrect in assuming if I pass |mTelemetry| as a parameter to the
> functions then perhaps everything would go as fine.
> 
> Instead I got the same error msg. as in comment #13

What do you mean? If you pass the telemetry service object to a function, it should work.
If not, please try to invest some time in figuring out why it doesn't work :)

> (In reply to Alessio Placitelli [:Dexter] from comment #13)
> > The Telemetry service is only available within the test case/Telemetry fixture.
> I was just wondering how can I call the Telemetryservice globally?

You cannot. The test environment is tied to the fixtures. The test Telemetry fixture sets up the environment and tears it down in each test.

Comment 18

[Avikalpa Kundu [:kalpa]]

Attachment: 1355498.patch

Comment 19

[Alessio Placitelli [:Dexter]]

Comment on attachment 8869809 [details] [diff] [review]
1355498.patch

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

This looks much better now, thanks. We're almost there, could you just take care of the nits below?

::: toolkit/components/telemetry/tests/gtest/TestHistograms.cpp
@@ +14,5 @@
> +namespace {
> +
> +void
> +GetAndClearHistogram(JSContext* cx, nsCOMPtr<nsITelemetry> mTelemetry,
> +                     const nsACString &name, bool is_keyed = false)

nit: let's not provide a default value for is_keyed.

@@ +17,5 @@
> +GetAndClearHistogram(JSContext* cx, nsCOMPtr<nsITelemetry> mTelemetry,
> +                     const nsACString &name, bool is_keyed = false)
> +{
> +  JS::RootedValue testHistogram(cx);
> +  JS::RootedValue rval(cx);

nit: let's move this declaration near where it's used.

@@ +21,5 @@
> +  JS::RootedValue rval(cx);
> +  nsresult rv;
> +
> +  // Get the histogram
> +  if(is_keyed)

nit: let's change this to

  nsresult rv = is_keyed ? mTelemetry->GetKeyedHistogramById(name, cx, &testHistogram)
                         : mTelemetry->GetHistogramById(name, cx, &testHistogram);

With ";" aligned to the "?" on the previous line.

@@ +36,5 @@
> +}
> +
> +void
> +GetSnapshots(JSContext* cx, nsCOMPtr<nsITelemetry> mTelemetry,
> +             const char* name, JS::MutableHandleValue valueOut, bool is_keyed = false)

nit: let's not provide a default for is_keyed

@@ +42,5 @@
> +  JS::RootedValue snapshot(cx);
> +  nsresult rv;
> +
> +  if(is_keyed)
> +    rv = mTelemetry->GetKeyedHistogramSnapshots(cx, &snapshot);

nit: as above, let's do something like

  nsresult rv = is_keyed ? mTelemetry->GetKeyedHistogramSnapshots(cx, &snapshot)
                         : mTelemetry->GetHistogramSnapshots(cx, &snapshot);

@@ +62,5 @@
> +  valueOut.set(histogram);
> +}
> +
> +void
> +GetPropertyFromHistogram(JSContext* cx, const char* name, JS::HandleValue histogram,

|GetHistogramFromSnapshot| and |GetPropertyFromHistogram| are basically identical, except for the assertion message.

Let's drop the first one and rename the second to something like |GetProperty|.

Let's also change the assertion message to:

"Cannot get property '" << name << "'";

Comment 20

[Avikalpa Kundu [:kalpa]]

Attachment: 1355498.patch

Thanks.

I've changed it accordingly.

Comment 21

[Alessio Placitelli [:Dexter]]

Comment on attachment 8871342 [details] [diff] [review]
1355498.patch

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

Looks good now, thank you for your time and patience!

Comment 22

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2720a25f4f0416346a904de4c8f03d7ac89e8e93

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/12e45fe24d0f
Refactor commonly used actions in functions within TestHistograms.cpp. r=Dexter

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/12e45fe24d0f