Closed Bug 1461965 Opened 6 years ago Closed 6 years ago

Investigate enabling xpcshell tests for GV telemetry code

Categories

(Toolkit :: Telemetry, enhancement, P1)

Product:
Toolkit
Component:
Telemetry
Platform:
Unspecified
Android
Type:
enhancement

Tracking

()

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

People

(Reporter: Dexter, Assigned: Dexter)

References

(Blocks 1 open bug)

Details

(Whiteboard: [geckoview:klar:p2])

Attachments

(3 files)

Bug 1461965 - Always build TelemetryGeckoViewPersistence.cpp on all platforms.
59 bytes, text/x-review-board-request
chutten
: review+
janerik
: review+
Details
Bug 1461965 - Enable xpcshell test coverage for GeckoView Telemetry.
59 bytes, text/x-review-board-request
chutten
: review+
janerik
: review+
froydnj
: review+
Details
Bug 1461965 - Add GeckoView test coverage for content histograms.
59 bytes, text/x-review-board-request
chutten
: review+
janerik
: review+
Details 
We have no way to tell, in xpcshell tests, if we're running GV or not. In order to run xpcshell tests for GV specific code, we need to fix that.

One possible approach could be to have GV-specific xpcshell tests in a separate directory and always enable the GV mode preference for these tests.
We should test that we're able to serialize/deserialize real scalar and histogram data.
Blocks: 1452550
Priority: -- → P2
Here's a proposal to add xpcshell test coverage for GV persistence stuff.

* Why is this needed? Why can't we use GTEST? *
GeckoView persistence code is exclusively compiled for the Android platform. Moreover, the code is only executed on the GeckoView product (which is different than Fennec!). To work around this, we always compile and link the persistence code to "gtest-xul", which is only used for linking against GTESTs.

However, this only gets us so far: TelemetryScalar.cpp/TelemetryHistogram.cpp can't be naively linked to that gtest library (see bug  1459144). As a consequence of that, all the serialization/deserialization paths are untested.

In addition to that, we can't simply run GV only xpcshell tests.

* What can we do? *

One possible way to enable GV only xpcshell tests is to:

- create a new test-android only IDL that exposes the methods from [1], i.e. nsITelemetryGeckoViewTest;
- create an Android-only xpcshell test that sets the current product to "GeckoView";
- setup the required GV environment variables;
- manually simulate the "persistence" cycle, as done in gtests [2], by calling nsITelemetryGeckoViewTest.

* Drawbacks *

[1] - https://searchfox.org/mozilla-central/rev/d4b9e50875ad7e5d20f2fee6a53418315f6dfcc0/toolkit/components/telemetry/geckoview/TelemetryGeckoViewPersistence.h
[2] - https://searchfox.org/mozilla-central/rev/d4b9e50875ad7e5d20f2fee6a53418315f6dfcc0/toolkit/components/telemetry/geckoview/gtest/TestGeckoView.cpp#305-309
Flags: needinfo?(jrediger)
Flags: needinfo?(chutten)
Having the full persistence cycle properly tested, including both serialization and deserialization is essential.
The proposal sounds like a good first approach, even though it requires a bit more setup code on our side.
The good thing I see is if we should ever get proper native xpcshell tests for GeckoView, our test code should remain largely unchanged and we can simply remove the setup code.
Flags: needinfo?(jrediger)
We can't be the only team struggling with the testing story for GV. Are there other teams whose approaches we could mimic?
Flags: needinfo?(chutten)
(In reply to Chris H-C :chutten from comment #4)
> We can't be the only team struggling with the testing story for GV. Are
> there other teams whose approaches we could mimic?

As far as I know, we're in a peculiar position: the other teams that are involved in this have Java test coverage in place [1], which is enough. We, however, conditionally enable/disable C++ code based on the platform & the product, putting us in a lucky privileged spot :-(

Eugen, are you aware of any other team working on GV having to deal with xpcshell-tests?

[1] - https://developer.mozilla.org/en-US/docs/Mozilla/Geckoview-Junit_Tests
Flags: needinfo?(esawin)
Adding whiteboard tag [geckoview:klar:p1] because this bug is not blocker Klar+GeckoView release, but we should decide what to do about xpcshell tests soon.
status-firefox61: --- → wontfix
OS: Unspecified → Android
Whiteboard: [geckoview:klar:p2]
(In reply to Alessio Placitelli [:Dexter] from comment #5)
> Eugen, are you aware of any other team working on GV having to deal with
> xpcshell-tests?

We haven't dealt with xpcshell support for GV since none of the GV-specific bits have required that yet. I'm sorry that you have to pave the way for that and I don't know enough about xpcshell to be helpful (jchen knows more, but looks like you've already talked about it).
Flags: needinfo?(esawin)
Note for self: we should add test coverage for multi-process scalar pending operations once we enable this.
(In reply to Eugen Sawin [:esawin] from comment #7)
> (In reply to Alessio Placitelli [:Dexter] from comment #5)
> > Eugen, are you aware of any other team working on GV having to deal with
> > xpcshell-tests?
> 
> We haven't dealt with xpcshell support for GV since none of the GV-specific
> bits have required that yet. I'm sorry that you have to pave the way for
> that and I don't know enough about xpcshell to be helpful (jchen knows more,
> but looks like you've already talked about it).

Hey Chris, I'm afraid we're in an unique position with the Telemetry code :) What a luck, huh? What about the proposal from comment 2?

@Georg, what's your take on the proposal from comment 2?
Flags: needinfo?(gfritzsche)
Flags: needinfo?(chutten)
It's a little weird adding JS APIs to GV, which specifically eschews our JSMs from their logic... but if you can pull it off it will fit our current testing story with a minimum of fuss. The goal is, after all, to make it easy to write tests (well, the goal is to make it _possible_, but I prefer to aim high :)


Two notes:

Will we also be responsible for getting treeherder to run xpcshell tasks on the GV platform, or is that being handled elsewhere?

If possible it'd be nice to tell moz.build to not even bother building the idl for Desktop.
Flags: needinfo?(chutten)
(In reply to Chris H-C :chutten from comment #10)
> Two notes:
> 
> Will we also be responsible for getting treeherder to run xpcshell tasks on
> the GV platform, or is that being handled elsewhere?

This will not run as a separate suite. It will run as part of our usual xpcshell tests, on all the platforms.

> If possible it'd be nice to tell moz.build to not even bother building the
> idl for Desktop.

I think that should be possible!
Assignee: nobody → alessio.placitelli
Priority: P2 → P1
Blocks: 1466490
Comment on attachment 8983437 [details]
Bug 1461965 - Always build TelemetryGeckoViewPersistence.cpp on all platforms.

https://reviewboard.mozilla.org/r/249318/#review255484

::: toolkit/components/telemetry/geckoview/gtest/TestGeckoView.cpp:16
(Diff revision 1)
>  #include "nsIOutputStream.h"
>  #include "nsITelemetry.h"
>  #include "nsJSUtils.h"
>  #include "nsNetUtil.h"
> +#include "nsThreadUtils.h"
> +#include "nsPrintfCString.h"

Why are these includes now required?
Attachment #8983437 - Flags: review?(jrediger) → review+
Comment on attachment 8983438 [details]
Bug 1461965 - Enable xpcshell test coverage for GeckoView Telemetry.

https://reviewboard.mozilla.org/r/249320/#review255486

::: toolkit/components/build/moz.build:31
(Diff revision 1)
>      '../perfmonitoring',
>      '../protobuf',
>      '../reputationservice',
>      '../startup',
>      '../statusfilter',
> +    '../telemetry',

I have no idea about the build system.
Why are these things added here?

::: toolkit/components/build/nsToolkitCompsCID.h:195
(Diff revision 1)
>    "@mozilla.org/addons/policy-service;1"
> +
> +#if defined(ENABLE_TESTS)
> +#define NS_TELEMETRYGECKOVIEWTESTING_CID \
> +  { 0xaaa3f7f2, 0x8ef0, 0x41ec, { 0x8d, 0x3, 0xae, 0xd6, 0x67, 0xcf, 0x7f, 0xa2 } };
> +#endif

As before, I'd just like to know why these things are added in this file specifically.

::: toolkit/components/telemetry/geckoview/nsITelemetryGeckoViewTesting.idl:7
(Diff revision 1)
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "nsISupports.idl"
> +
> +interface nsIObserver;

Why is the observer  interface required here? It's not referenced in this definition file anymore.

::: toolkit/components/telemetry/geckoview/nsITelemetryGeckoViewTesting.idl:17
(Diff revision 1)
> + * THIS IS NOT AN API TO BE USED BY EXTENSIONS! ONLY USED BY MOZILLA TESTS.
> + */
> +[scriptable, builtinclass, uuid(019feb07-e5dd-48e6-aa59-fc98bcb65e7f)]
> +interface nsITelemetryGeckoViewTesting : nsISupports
> +{
> +  void initPersistence();

Should we have short docs here?

::: toolkit/components/telemetry/moz.build:225
(Diff revision 1)
> +    XPIDL_SOURCES += [
> +        'geckoview/nsITelemetryGeckoViewTesting.idl'
> +    ]
> +
> +    SOURCES += [
> +        'geckoview/TelemetryGeckoViewTesting.cpp'

This file is not included in this patch. Forgot to commit it?
Attachment #8983438 - Flags: review?(jrediger) → review-
Comment on attachment 8983439 [details]
Bug 1461965 - Add GeckoView test coverage for content histograms.

https://reviewboard.mozilla.org/r/249322/#review255494

::: toolkit/components/telemetry/tests/unit/test_GeckoView.js:68
(Diff revision 1)
> +add_task(async function setup() {
> +  // Init the profile.
> +  let profileDir = do_get_profile(true);
> +
> +  // Set geckoview mode.
> +  Services.prefs.setBoolPref("toolkit.telemetry.isGeckoViewMode", true);

When we implemented `CurrentProduct` we made the explicit decision that the identified product is only changed when that preference is changed _and_ `SetCurrentProduct` is called (which only has an effect on Android).

Therefore internal Telemetry code will still work in non-Geckoview mode, correct?

::: toolkit/components/telemetry/tests/unit/test_GeckoView_content_histograms.js:14
(Diff revision 1)
> +// test_GeckoView.js. It assumes to be in the content
> +// process.
> +function run_test() {
> +  // Get the histograms and set some values in the content process.
> +  Services.telemetry.getHistogramById("TELEMETRY_TEST_MULTIPRODUCT")
> +  					.add(73);

Mixed tabs and spaces.

::: toolkit/components/telemetry/tests/unit/test_GeckoView_content_histograms.js:16
(Diff revision 1)
> +function run_test() {
> +  // Get the histograms and set some values in the content process.
> +  Services.telemetry.getHistogramById("TELEMETRY_TEST_MULTIPRODUCT")
> +  					.add(73);
> +  Services.telemetry.getKeyedHistogramById("TELEMETRY_TEST_KEYED_COUNT")
> +  					.add("content-test-key", 37);

Mixed tabs and spaces.
Attachment #8983439 - Flags: review?(jrediger) → review+
Comment on attachment 8983439 [details]
Bug 1461965 - Add GeckoView test coverage for content histograms.

https://reviewboard.mozilla.org/r/249322/#review255494

> When we implemented `CurrentProduct` we made the explicit decision that the identified product is only changed when that preference is changed _and_ `SetCurrentProduct` is called (which only has an effect on Android).
> 
> Therefore internal Telemetry code will still work in non-Geckoview mode, correct?

This is true *within* Telemetry (internal Telemetry code), but not *outside* of Telemetry, see [ContentProcessSingleton](https://searchfox.org/mozilla-central/rev/292d295d6b084b43b70de26a42e68513bb7b36a3/toolkit/components/processsingleton/ContentProcessSingleton.js#11,27-32): we could probably provide an IDL method to fetch the current product from nsITelemetry (and update the check here). Otherwise, with the pref flip and on products different than GeckoView, the linked code will crash because we won't package the `GeckoViewTelemetryController.jsm` file :(
Comment on attachment 8983438 [details]
Bug 1461965 - Enable xpcshell test coverage for GeckoView Telemetry.

https://reviewboard.mozilla.org/r/249320/#review255486

> I have no idea about the build system.
> Why are these things added here?

This change is needed in order for the build system to find Telemetry headers in `nsToolkitCompsCID.*`.

> As before, I'd just like to know why these things are added in this file specifically.

This is where all the contract ids and names for the IDL interfaces live, for the Toolkit component.

> This file is not included in this patch. Forgot to commit it?

Aaargh, right!
Comment on attachment 8983437 [details]
Bug 1461965 - Always build TelemetryGeckoViewPersistence.cpp on all platforms.

https://reviewboard.mozilla.org/r/249318/#review255506

::: toolkit/components/telemetry/geckoview/gtest/TestGeckoView.cpp:16
(Diff revision 1)
>  #include "nsIOutputStream.h"
>  #include "nsITelemetry.h"
>  #include "nsJSUtils.h"
>  #include "nsNetUtil.h"
> +#include "nsThreadUtils.h"
> +#include "nsPrintfCString.h"

This is weird, I know :( Moving `TelemetryGeckoViewPersistence.cpp` to a different compilation unit triggered all sorts of errors (functions not declared, unknown stuff, ...) that required me to add this headers. I'm not sure why it wasn't complaining before.
Flags: needinfo?(gfritzsche)
Comment on attachment 8983437 [details]
Bug 1461965 - Always build TelemetryGeckoViewPersistence.cpp on all platforms.

https://reviewboard.mozilla.org/r/249318/#review255570
Attachment #8983437 - Flags: review?(chutten) → review+
Comment on attachment 8983438 [details]
Bug 1461965 - Enable xpcshell test coverage for GeckoView Telemetry.

https://reviewboard.mozilla.org/r/249320/#review255572

::: toolkit/components/build/moz.build:31
(Diff revision 2)
>      '../perfmonitoring',
>      '../protobuf',
>      '../reputationservice',
>      '../startup',
>      '../statusfilter',
> +    '../telemetry',

This was odd to me, too. I guess it's just the difference between a component and a service, huh.
Attachment #8983438 - Flags: review?(chutten) → review+
Comment on attachment 8983439 [details]
Bug 1461965 - Add GeckoView test coverage for content histograms.

https://reviewboard.mozilla.org/r/249322/#review255566

::: toolkit/components/telemetry/tests/unit/test_GeckoView.js:22
(Diff revision 2)
> + * @return {Promise} A promise resolved after the execution in the other process
> + *         finishes.
> + */
> +async function run_in_child(aFileName) {
> +  const PREF_GECKOVIEW_MODE = "toolkit.telemetry.isGeckoViewMode";
> +  // We don't ship GeckoViewTelemetryController.jsm outside of GeckoView. If

Don't we ship it across MOZ_WIDGET_ANDROID?

::: toolkit/components/telemetry/tests/unit/test_GeckoView.js:29
(Diff revision 2)
> +  // other platforms because ContentProcessSingleton.js requires it. Work
> +  // around this by temporarily setting the pref to false.
> +  const currentValue = Services.prefs.getBoolPref(PREF_GECKOVIEW_MODE, false);
> +  Services.prefs.setBoolPref(PREF_GECKOVIEW_MODE, false);
> +  await run_test_in_child(aFileName);
> +  Services.prefs.setBoolPref(PREF_GECKOVIEW_MODE, currentValue);

I've now seen this enough to wish for a pushPref/popPref:

Services.prefs.pushBoolPref(PREF_GECKOVIEW_MODE, false);
await run_test_in_child(aFileName);
Assert.equal(false, Services.prefs.popPref(PREF_GECKOVIEW_MODE));

Too easy to misuse, though, I guess.
Attachment #8983439 - Flags: review?(chutten) → review+
Comment on attachment 8983439 [details]
Bug 1461965 - Add GeckoView test coverage for content histograms.

https://reviewboard.mozilla.org/r/249322/#review255734

::: toolkit/components/telemetry/tests/unit/test_GeckoView.js:22
(Diff revision 2)
> + * @return {Promise} A promise resolved after the execution in the other process
> + *         finishes.
> + */
> +async function run_in_child(aFileName) {
> +  const PREF_GECKOVIEW_MODE = "toolkit.telemetry.isGeckoViewMode";
> +  // We don't ship GeckoViewTelemetryController.jsm outside of GeckoView. If

Yes, good point. We don't ship that file outside of Android. Fixing the comment.

::: toolkit/components/telemetry/tests/unit/test_GeckoView.js:29
(Diff revision 2)
> +  // other platforms because ContentProcessSingleton.js requires it. Work
> +  // around this by temporarily setting the pref to false.
> +  const currentValue = Services.prefs.getBoolPref(PREF_GECKOVIEW_MODE, false);
> +  Services.prefs.setBoolPref(PREF_GECKOVIEW_MODE, false);
> +  await run_test_in_child(aFileName);
> +  Services.prefs.setBoolPref(PREF_GECKOVIEW_MODE, currentValue);

Heh, yes, I agree. There appears to be something for mochitests, but not for xpcshell :( See [SpecialPowers](https://searchfox.org/mozilla-central/rev/3737701cfab93ccea04c0e9cab211ad10f931d87/accessible/tests/browser/head.js#16)
Hey Nathan,

could you kindly review the changeset "Bug 1461965 - Enable xpcshell test coverage for GeckoView Telemetry."?

The idea is to allow testing GeckoView Telemetry persistence using xpcshell tests (which are not supported for GeckoView, that's why we need this workaround). To do that, we implement a test-only IDL interface that drives the API defined at [1].

- Is this the right way to add a new IDL to the build system? Are you the right person to review this?
- Is there a better way to only export the IDL in xpcshell tests?
- Are we required to export the contract name/ids in the nsToolkitComps* files?

[1] - https://searchfox.org/mozilla-central/rev/3737701cfab93ccea04c0e9cab211ad10f931d87/toolkit/components/telemetry/geckoview/TelemetryGeckoViewPersistence.h
Flags: needinfo?(nfroyd)
(In reply to Alessio Placitelli [:Dexter] from comment #28)
> - Is this the right way to add a new IDL to the build system? Are you the
> right person to review this?

Yes, and sure.

> - Is there a better way to only export the IDL in xpcshell tests?

No. =/

> - Are we required to export the contract name/ids in the nsToolkitComps*
> files?

You are not required to define the names in nsToolkitCompsCID.h; you can define the contract name/ids only in nsToolkitCompsModule.cpp if you like.  I'm not sure exactly what you mean by "export", so if you meant something different than the above, please ask for further clarification!

If you wanted to make the idl methods harder to call from C++, you could include [implicit_jscontext] on the methods, which would add a JSContext* argument to the methods.  You wouldn't use the JSContext in the actual implementation, of course.
Flags: needinfo?(nfroyd)
Comment on attachment 8983438 [details]
Bug 1461965 - Enable xpcshell test coverage for GeckoView Telemetry.

https://reviewboard.mozilla.org/r/249320/#review255912

::: toolkit/components/telemetry/geckoview/nsITelemetryGeckoViewTesting.idl:16
(Diff revisions 1 - 2)
>   */
>  [scriptable, builtinclass, uuid(019feb07-e5dd-48e6-aa59-fc98bcb65e7f)]
>  interface nsITelemetryGeckoViewTesting : nsISupports
>  {
> +  /**
> +   * The following methods maps to the functions with the

maps -> map

::: toolkit/components/telemetry/geckoview/nsITelemetryGeckoViewTesting.idl:24
(Diff revisions 1 - 2)
>    void initPersistence();
>    void deInitPersistence();
>    void clearPersistenceData();
> +
> +  /**
> +   * This function enqueues a persist action into the Telemetry

Don't start with "This function".

"Enqueues a persist action ..."

::: toolkit/components/telemetry/geckoview/nsITelemetryGeckoViewTesting.idl:26
(Diff revisions 1 - 2)
>    void clearPersistenceData();
> +
> +  /**
> +   * This function enqueues a persist action into the Telemetry
> +   * persist thread: measurements might be written to the disk
> +   * after the it returns.

remove "the"
Attachment #8983438 - Flags: review?(jrediger) → review+
(In reply to Nathan Froyd [:froydnj] from comment #29)
> (In reply to Alessio Placitelli [:Dexter] from comment #28)
> > - Are we required to export the contract name/ids in the nsToolkitComps*
> > files?
> 
> You are not required to define the names in nsToolkitCompsCID.h; you can
> define the contract name/ids only in nsToolkitCompsModule.cpp if you like. 
> I'm not sure exactly what you mean by "export", so if you meant something
> different than the above, please ask for further clarification!

Ah no, that's exactly what I needed :) Looks like it won't change much, so I'm leaving things as they are.

> If you wanted to make the idl methods harder to call from C++, you could
> include [implicit_jscontext] on the methods, which would add a JSContext*
> argument to the methods.  You wouldn't use the JSContext in the actual
> implementation, of course.

This is a great trick, thanks! I didn't think of that :)

I've flagged you for review on the IDL file now :)
Comment on attachment 8983438 [details]
Bug 1461965 - Enable xpcshell test coverage for GeckoView Telemetry.

https://reviewboard.mozilla.org/r/249320/#review256220

::: toolkit/components/telemetry/geckoview/TelemetryGeckoViewTesting.h:9
(Diff revision 3)
> +
> +#include "nsITelemetryGeckoViewTesting.h"
> +
> +class TelemetryGeckoViewTestingImpl final : public nsITelemetryGeckoViewTesting
> +{
> +  ~TelemetryGeckoViewTestingImpl() {}

Just `= default;`, please?

::: toolkit/components/telemetry/geckoview/TelemetryGeckoViewTesting.cpp:19
(Diff revision 3)
> +  // We don't need aCx. It's there to make this test function harder to
> +  // call from C++.
> +  mozilla::Unused << aCx;

FWIW, you can define unnamed parameters:

```
NS_IMETHODIMP
TelemetryGeckoViewTestingImpl::InitPersistence(JSContext*)
{
  ...
}
```

which is slightly nicer.  I think you can just explain the rationale for the unused `JSContext*` parameter once, before defining all the methods, too.

::: toolkit/components/telemetry/geckoview/TelemetryGeckoViewTesting.cpp:29
(Diff revision 3)
> +  // We don't need aCx. It's there to make this test function harder to
> +  // call from C++.
> +  mozilla::Unused << aCx;

Same here.

::: toolkit/components/telemetry/geckoview/TelemetryGeckoViewTesting.cpp:39
(Diff revision 3)
> +  // We don't need aCx. It's there to make this test function harder to
> +  // call from C++.
> +  mozilla::Unused << aCx;

Same here.

::: toolkit/components/telemetry/geckoview/TelemetryGeckoViewTesting.cpp:49
(Diff revision 3)
> +  // We don't need aCx. It's there to make this test function harder to
> +  // call from C++.
> +  mozilla::Unused << aCx;

Same here.
Attachment #8983438 - Flags: review?(nfroyd) → review+
Depends on: 1467759
Pushed by alessio.placitelli@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/68a68375557b
Always build TelemetryGeckoViewPersistence.cpp on all platforms. r=chutten,janerik
https://hg.mozilla.org/integration/autoland/rev/a9ef2fb7b1bc
Enable xpcshell test coverage for GeckoView Telemetry. r=chutten,froydnj,janerik
https://hg.mozilla.org/integration/autoland/rev/823c3fb08d60
Add GeckoView test coverage for content histograms. r=chutten,janerik
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: