Closed
Bug 1441904
Opened 6 years ago
Closed 6 years ago
Test that TelemetryEnvironment gets the device data properly on Android
Categories
(Toolkit :: Telemetry, enhancement, P3)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla61
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
Assignee | ||
Comment 1•6 years ago
|
||
Can i try this one?
Reporter | ||
Comment 2•6 years ago
|
||
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
Assignee | ||
Comment 3•6 years ago
|
||
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)
Reporter | ||
Comment 4•6 years ago
|
||
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)
Reporter | ||
Comment 5•6 years ago
|
||
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)
Assignee | ||
Comment 6•6 years ago
|
||
Yes Chris, i want to work on this bug and will upload a patch soon.
Flags: needinfo?(akriti.v10)
Comment hidden (mozreview-request) |
Reporter | ||
Comment 8•6 years ago
|
||
mozreview-review |
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)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8968184 -
Attachment is obsolete: true
Comment 10•6 years ago
|
||
mozreview-review |
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]
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8968494 -
Attachment is obsolete: true
Attachment #8968494 -
Flags: review?(chutten)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8968506 -
Attachment is obsolete: true
Attachment #8968506 -
Flags: review?(chutten)
Reporter | ||
Comment 13•6 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8968510 -
Attachment is obsolete: true
Reporter | ||
Comment 15•6 years ago
|
||
mozreview-review |
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+
Comment 16•6 years ago
|
||
Pushed by chutten@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/116248a2b4ac Testing Android Device data. , r=chutten
Comment 17•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/116248a2b4ac
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•6 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•