Closed Bug 1441904 Opened 2 years ago Closed 2 years ago

Test that TelemetryEnvironment gets the device data properly on Android

Categories

(Toolkit :: Telemetry, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: chutten, Assigned: akriti.v10, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good next bug][lang=js])

Attachments

(1 file, 4 obsolete files)

TelemetryEnvironment pulls in some device data on Android. This is untested[1].

This bug is to write an Android-only TelemetryEnvironment xpcshell test that ensures that the device data is stored in the Environment and has the structure documented here[2] under system.device (basically, it has the fields model, manufacturer, hardware, isTablet).

To write an android-only test we will need to add a new test file to toolkit/components/telemetry/tests/unit and ensure that it has the right skip-if notation in the xpcshell.ini file.

To test locally you will need to set up an Firefox for Android build environment[3]. Alternatively, since this is a simple test, if you have try server access we can use it to run the tests for us.

If you have any questions, please reach out here and ni? a mentor.

[1]: https://codecov.io/gh/marco-c/gecko-dev/src/master/toolkit/components/telemetry/TelemetryEnvironment.jsm#L1721
[2]: https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/data/environment.html
[3]: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_for_Android_build
Can i try this one?
You certainly may. I have assigned it to you.

Let me know if working on both this and bug 1438896 at the same time becomes a problem.
Assignee: nobody → akriti.v10
Status: NEW → ASSIGNED
The `test_TelemetryEnvironment.js` already contains something like this-

if (gIsAndroid) {
    let deviceData = data.system.device;
    Assert.ok(checkNullOrString(deviceData.model));
    Assert.ok(checkNullOrString(deviceData.manufacturer));
    Assert.ok(checkNullOrString(deviceData.hardware));
    Assert.ok(checkNullOrBool(deviceData.isTablet));

Do we need to separate this out into another test file?
Flags: needinfo?(chutten)
Well that's interesting, isn't it. The code coverage report says we don't test it, but that test certainly is testing it.

... ah-ha! Looking at xpcshell.ini, test_TelemetryEnvironment.js is skipped if the OS is "android"

So that test segment will never execute. (gIsAndroid will always be false)

To fix this, we'll need to pull the android-specific stuff out of test_TelemetryEnvironment.js into a new file (maybe test_TelemetryEnvironment_android.js?) so that the tests will actually run :)

Does this make sense?
Flags: needinfo?(chutten)
Are you still working on this bug? Do you need any further help on this bug? It seems like we found the problem and can probably work out a quick fix.
Flags: needinfo?(akriti.v10)
Yes Chris, i want to work on this bug and will upload a patch soon.
Flags: needinfo?(akriti.v10)
Comment on attachment 8968184 [details]
Bug 1441904 : Android xpcshell test. ,

https://reviewboard.mozilla.org/r/236862/#review242754

The review set is missing test_TelemetryAndroidEnvironment.js ...did it get left out of your changeset?

::: toolkit/components/telemetry/tests/unit/xpcshell.ini:34
(Diff revision 1)
>  [test_TelemetryStorage.js]
>  [test_SubsessionChaining.js]
>  tags = addons
>  [test_TelemetryEnvironment.js]
> -skip-if = os == "android"
> +[test_TelemetryAndroidEnvironment.js]
> +#skip-if = os == "android"

We do still want to skip testing test_TelemetryEnvironment.js on Android, though, so you should leave this notation in place for it.
Attachment #8968184 - Flags: review?(chutten)
Attachment #8968184 - Attachment is obsolete: true
Comment on attachment 8968494 [details]
Bug 1441904 : Test that TelemetryEnvironment gets the device data properly on Android. ,

https://reviewboard.mozilla.org/r/237188/#review242994


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/telemetry/tests/unit/test_TelemetryAndroidEnvironment.js:68
(Diff revision 1)
> +  }
> +}
> +
> +
> +add_task(async function test_systemEnvironment() {
> +  environmentData = TelemetryEnvironment.currentEnvironment;

Error: 'environmentdata' is not defined. [eslint: no-undef]

::: toolkit/components/telemetry/tests/unit/test_TelemetryAndroidEnvironment.js:69
(Diff revision 1)
> +}
> +
> +
> +add_task(async function test_systemEnvironment() {
> +  environmentData = TelemetryEnvironment.currentEnvironment;
> +  checkSystemSection(environmentData);

Error: 'environmentdata' is not defined. [eslint: no-undef]
Attachment #8968494 - Attachment is obsolete: true
Attachment #8968494 - Flags: review?(chutten)
Attachment #8968506 - Attachment is obsolete: true
Attachment #8968506 - Flags: review?(chutten)
Comment on attachment 8968510 [details]
Bug 1441904 : Test that TelemetryEnvironment gets the device data properly on Android. ,

https://reviewboard.mozilla.org/r/237208/#review243130

One little thing to correct, and we'll be on our way.

A question: were you wanting to remove the dead test code from test_TelemetryEnvironment.js as part of this bug as well? Or would you like to file a new bug for removing that?

::: toolkit/components/telemetry/tests/unit/test_TelemetryAndroidEnvironment.js:48
(Diff revision 1)
> + *         boolean.
> + */
> +function checkNullOrBool(aValue) {
> +  return aValue === null || (typeof aValue == "boolean");
> +}
> +

We don't need quite this much whitespace, I don't think :)
Attachment #8968510 - Flags: review?(chutten) → review+
Attachment #8968510 - Attachment is obsolete: true
Comment on attachment 8968883 [details]
Bug 1441904 : Testing Android Device data. ,

https://reviewboard.mozilla.org/r/237616/#review243378

This looks like this should work, thank you!
Attachment #8968883 - Flags: review?(chutten) → review+
Pushed by chutten@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/116248a2b4ac
Testing Android Device data. , r=chutten
https://hg.mozilla.org/mozilla-central/rev/116248a2b4ac
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.