Closed Bug 1459144 Opened 6 years ago Closed 6 years ago

Enable better GeckoView gtests

Categories

(Toolkit :: Telemetry, enhancement, P1)

Product:
Toolkit
Component:
Telemetry
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox61 --- wontfix
firefox62 --- fixed

People

(Reporter: Dexter, Assigned: Dexter)

References

Details

Attachments

(3 files)

Bug 1459144 - Allocate enough memory for all the processes when deserializing histograms.
59 bytes, text/x-review-board-request
chutten
: review+
Details
Bug 1459144 - Fix CountHistogram deserialization for GeckoView.
59 bytes, text/x-review-board-request
chutten
: review+
janerik
: review+
Details
Bug 1459144 - Add gtest coverage for GeckoView measurements' persistence.
59 bytes, text/x-review-board-request
janerik
: review+
chutten
: review+
Details 
In GeckoView gtests, we're mocking TelemetryScalar::* functions as I had problems compiling and linking TelemetryScalar.cpp against xul-gtest.

We should investigate if we can add it back to enable more comprehensive gtest coverage.
Blocks: 1453591
Priority: -- → P3
Assignee: nobody → alessio.placitelli
Priority: P3 → P1
Summary: Investigate adding TelemetryScalar.cpp to GeckoView gtests → Enable better GeckoView gtests
Comment on attachment 8979624 [details]
Bug 1459144 - Add gtest coverage for GeckoView measurements' persistence.

https://reviewboard.mozilla.org/r/245758/#review251864

This is good. I have already wrote some tests for the scalar semantics based on the changes to gtest here.

::: commit-message-b1d58:1
(Diff revision 1)
> +Bug 1459144 - Add gtest coverag for GeckoView measurements' persistence. r?chutten,janerik

coverage.

::: commit-message-b1d58:3
(Diff revision 1)
> +Bug 1459144 - Add gtest coverag for GeckoView measurements' persistence. r?chutten,janerik
> +
> +This patch changes GeckoView persistence code so that it will always gets compiled

gets -> get

::: toolkit/components/telemetry/geckoview/gtest/TestGeckoView.cpp:339
(Diff revision 1)
> +  ASSERT_TRUE(fileExists) << "The persisted measurements must exist on the disk";
> +
> +  // Clear the in-memory scalars again. They will be restored from the disk.
> +  Unused << mTelemetry->ClearScalars();
> +
> +  // Load the persisted file again: this will trigger the TestLoad* functions

This comment is now incorrect, isn't it?

::: toolkit/components/telemetry/geckoview/gtest/TestGeckoView.cpp:350
(Diff revision 1)
> +  JS::RootedValue scalarsSnapshot(cx.GetJSContext());
> +  JS::RootedValue keyedScalarsSnapshot(cx.GetJSContext());
> +  GetScalarsSnapshot(false, cx.GetJSContext(), &scalarsSnapshot);
> +  GetScalarsSnapshot(true, cx.GetJSContext(), &keyedScalarsSnapshot);
> +
> +  // Verify that the scalars were correctly persisted.

"were correctly persisted and restored."

::: toolkit/components/telemetry/geckoview/gtest/TestGeckoView.cpp:434
(Diff revision 1)
> +  GetProperty(cx.GetJSContext(), "sum", histogram,  &sum);
> +
> +  // Check that the "sum" stored in the histogram matches with |kExpectedValue|
> +  uint32_t uSum = 0;
> +  JS::ToUint32(cx.GetJSContext(), sum, &uSum);
> +  ASSERT_EQ(uSum, kExpectedUintValue) << "The histogram is not returning expected value";

"not returning the expected value"

::: toolkit/components/telemetry/geckoview/gtest/TestGeckoView.cpp:448
(Diff revision 1)
> +  ASSERT_FALSE(expectedKeyData.isUndefined())
> +    << "Cannot find the expected key in the keyed histogram data";
> +  GetProperty(cx.GetJSContext(), "sum", expectedKeyData,  &sum);
> +  JS::ToUint32(cx.GetJSContext(), sum, &uSum);
> +  ASSERT_EQ(uSum, kExpectedKeyedSum)
> +    << "The histogram is not returning expected sum for 'gv_key'";

"the expected sum"
Attachment #8979624 - Flags: review?(jrediger) → review+
Comment on attachment 8979623 [details]
Bug 1459144 - Fix CountHistogram deserialization for GeckoView.

https://reviewboard.mozilla.org/r/245756/#review251870
Attachment #8979623 - Flags: review?(jrediger) → review+
Comment on attachment 8979622 [details]
Bug 1459144 - Allocate enough memory for all the processes when deserializing histograms.

https://reviewboard.mozilla.org/r/245754/#review252222

::: commit-message-77c06:2
(Diff revision 1)
> +Bug 1459144 - Allocate enough memory for all the processes when deserializing histograms. r?chutten
> +

Why?
Attachment #8979622 - Flags: review?(chutten) → review+
Comment on attachment 8979623 [details]
Bug 1459144 - Fix CountHistogram deserialization for GeckoView.

https://reviewboard.mozilla.org/r/245756/#review252226

Ah, this is because count histograms are special and use the 0-bucket as the scalar value, isn't it?

Pretty sure the old code was a bug, or at least had odd expected behaviour.
Attachment #8979623 - Flags: review?(chutten) → review+
Comment on attachment 8979624 [details]
Bug 1459144 - Add gtest coverage for GeckoView measurements' persistence.

https://reviewboard.mozilla.org/r/245758/#review252230

...probably not worthwhile to add tests that ensure we overwrite histogram and scalar activity that happens before we load the persistence file, is it...

::: toolkit/components/telemetry/TelemetryHistogram.h
(Diff revision 2)
>  #include "mozilla/TelemetryProcessEnums.h"
>  
>  #include "mozilla/TelemetryComms.h"
>  #include "nsXULAppAPI.h"
>  
> -#if defined(MOZ_TELEMETRY_GECKOVIEW)

Is there a way to check to see if we're building xul-gtest instead of xul? Some define? Because that'll let us continue excluding these GV-only portions of the codebase in Desktop xul builds.

::: toolkit/components/telemetry/geckoview/gtest/TestGeckoView.cpp:144
(Diff revision 2)
>    rv = file->Exists(&aFileExists);
>    ASSERT_EQ(NS_OK, rv) << "nsIFile::Exists must not fail";
>  }
>  
> -void
> -CheckJSONEqual(JSContext* aCx, JS::HandleValue aData, JS::HandleValue aDataOther)
> +/**
> + * An helper class to wait for the internal "data loaded"

Since "h" is voiced in "helper" the correct article is "A" instead of "An".

::: toolkit/components/telemetry/geckoview/gtest/TestGeckoView.cpp:158
(Diff revision 2)
> -
> -void
> -TestSerializeScalars(JSONWriter& aWriter)
> -{
> +  {
> -  // Report the same data that's in kSampleData for scalars.
> -  // We only want to make sure that I/O and parsing works, as telemetry
> +    nsCOMPtr<nsIObserverService> observerService =
> +      mozilla::services::GetObserverService();

This can fail. Maybe a comment to state that we're happy to have this crash the tests if that's the case?

::: toolkit/components/telemetry/geckoview/gtest/TestGeckoView.cpp:267
(Diff revision 2)
>  }
>  
> +
> +
> +/**
> + * Test that we can correctly persist the scalar data.

This isn't the right comment for this test

::: toolkit/components/telemetry/geckoview/gtest/TestGeckoView.cpp:288
(Diff revision 2)
> +  // Check that the topic is triggered when the measuements file exists.
> +  WritePersistenceFile(nsDependentCString(kSampleData));
> +  CheckPersistenceFileExists(fileExists);
> +  ASSERT_TRUE(fileExists) << "The persisted measurements must exist on the disk";
> +
> +  // Check that the data loaded topic is triggered if no measurement file exists.

But the file does exist.

::: toolkit/components/telemetry/geckoview/gtest/TestGeckoView.cpp:317
(Diff revision 2)
>    // Init the persistence: this will trigger the measurements to be written
>    // to disk off-the-main thread.
>    TelemetryGeckoViewPersistence::InitPersistence();
> +  loadingFinished->WaitForNotification();
> +
> +  // Set some scalars: we can only test the parent process as we don't support other

Actually we sorta do. Since all of the storage is in the parent process we can test multi-process support without spinning up multiple processes. I used this to help test event summaries.

Not sure if you want to, but I think we could if you did.

::: toolkit/components/telemetry/geckoview/gtest/TestGeckoView.cpp:393
(Diff revision 2)
> +  Telemetry::Accumulate(Telemetry::TELEMETRY_TEST_KEYED_COUNT, NS_LITERAL_CSTRING("gv_key"),
> +                        keyedSamples);
> +
> +
> +  // Dispatch the persisting task: we don't wait for the timer to expire
> +  // as we need a reliable and reproducible way to kick off this. We ensure

nit: *kick this off
Attachment #8979624 - Flags: review?(chutten)
Comment on attachment 8979624 [details]
Bug 1459144 - Add gtest coverage for GeckoView measurements' persistence.

https://reviewboard.mozilla.org/r/245758/#review252230

> Actually we sorta do. Since all of the storage is in the parent process we can test multi-process support without spinning up multiple processes. I used this to help test event summaries.
> 
> Not sure if you want to, but I think we could if you did.

Do you mean by using [SummarizeEvents](https://searchfox.org/mozilla-central/rev/dc6d85680539cb7e5fe2843d38a30a0581bfefe1/toolkit/components/telemetry/TelemetryScalar.h#74-75)? Mh, yes, this seems a nice workaround! I'm not sure I'd like to depend on other features for simulating something that should be easy in the test itself :( Given that we're also attempting to turn on xpcshell tests, and we support content processes there, what about I defer this kind of testing to bug 1461965?
Comment on attachment 8979624 [details]
Bug 1459144 - Add gtest coverage for GeckoView measurements' persistence.

https://reviewboard.mozilla.org/r/245758/#review252230

> Is there a way to check to see if we're building xul-gtest instead of xul? Some define? Because that'll let us continue excluding these GV-only portions of the codebase in Desktop xul builds.

We don't :( We just have `ENABLE_TEST` which generically tells us that this build includes tests, but it's not specifically tied to gtests. I dug into that while writing the first part of the gtest coverage :(
Comment on attachment 8979624 [details]
Bug 1459144 - Add gtest coverage for GeckoView measurements' persistence.

https://reviewboard.mozilla.org/r/245758/#review252230

> We don't :( We just have `ENABLE_TEST` which generically tells us that this build includes tests, but it's not specifically tied to gtests. I dug into that while writing the first part of the gtest coverage :(

And we can't "just" put a define in our own tests/getest/moz.build that we can read out?

> Do you mean by using [SummarizeEvents](https://searchfox.org/mozilla-central/rev/dc6d85680539cb7e5fe2843d38a30a0581bfefe1/toolkit/components/telemetry/TelemetryScalar.h#74-75)? Mh, yes, this seems a nice workaround! I'm not sure I'd like to depend on other features for simulating something that should be easy in the test itself :( Given that we're also attempting to turn on xpcshell tests, and we support content processes there, what about I defer this kind of testing to bug 1461965?

Fine by me.
Comment on attachment 8979624 [details]
Bug 1459144 - Add gtest coverage for GeckoView measurements' persistence.

https://reviewboard.mozilla.org/r/245758/#review252230

> And we can't "just" put a define in our own tests/getest/moz.build that we can read out?

That's exactly what I was planning to do initially (see `MOZ_TELEMETRY_GECKOVIEW` :-) ). However, we would need to compile and link Telemetry\*.cpp twice: once against libxul (with the GV/GTEST undefined), once against gtest-libxul (with the GV/GTEST stuff defined). That would result (or, I'd better say, resulted) in double defined symbols for all the functions that are not conditionally enabled/disabled. Think, for example, to all our `TelemetryScalar::*` functions. The natural solution would be to move the GV specific code to a separate  cpp file: however they heavily rely on `internal_*` functions so this would not be easily achievable. After talking this through with Georg, we decided to simply compile the code on all the platforms given the negligible increase in the executable size and the fact the we control enabling the feature via `GetCurrentProduct()`.
Comment on attachment 8979624 [details]
Bug 1459144 - Add gtest coverage for GeckoView measurements' persistence.

https://reviewboard.mozilla.org/r/245758/#review252230

> That's exactly what I was planning to do initially (see `MOZ_TELEMETRY_GECKOVIEW` :-) ). However, we would need to compile and link Telemetry\*.cpp twice: once against libxul (with the GV/GTEST undefined), once against gtest-libxul (with the GV/GTEST stuff defined). That would result (or, I'd better say, resulted) in double defined symbols for all the functions that are not conditionally enabled/disabled. Think, for example, to all our `TelemetryScalar::*` functions. The natural solution would be to move the GV specific code to a separate  cpp file: however they heavily rely on `internal_*` functions so this would not be easily achievable. After talking this through with Georg, we decided to simply compile the code on all the platforms given the negligible increase in the executable size and the fact the we control enabling the feature via `GetCurrentProduct()`.

Well that's a bummer. Thank you for patiently going over your already-tread ground with me. I should've known you would have tried all the clever stuff already :)
Comment on attachment 8979624 [details]
Bug 1459144 - Add gtest coverage for GeckoView measurements' persistence.

https://reviewboard.mozilla.org/r/245758/#review252230

> Well that's a bummer. Thank you for patiently going over your already-tread ground with me. I should've known you would have tried all the clever stuff already :)

No Chris, *thank you* for asking that. I should have mentioned it in the first place, without assuming it was clear! :D
Pushed by alessio.placitelli@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e211ce684bec
Allocate enough memory for all the processes when deserializing histograms. r=chutten
https://hg.mozilla.org/integration/autoland/rev/bbc8cb27a851
Fix CountHistogram deserialization for GeckoView. r=chutten,janerik
https://hg.mozilla.org/integration/autoland/rev/8935a0cabd57
Add gtest coverage for GeckoView measurements' persistence. r=janerik
Comment on attachment 8979624 [details]
Bug 1459144 - Add gtest coverage for GeckoView measurements' persistence.

https://reviewboard.mozilla.org/r/245758/#review252988
Attachment #8979624 - Flags: review?(chutten) → review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: