Closed
Bug 1346200
Opened 7 years ago
Closed 7 years ago
Remove B2G code branches from Telemetry JS modules
Categories
(Toolkit :: Telemetry, enhancement, P4)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: gfritzsche, Assigned: anejaalisha)
References
Details
(Whiteboard: [measurement:client])
Attachments
(1 file, 5 obsolete files)
25.67 KB,
patch
|
chutten
:
review+
|
Details | Diff | Splinter Review |
We still have logic for handling B2G / Firefox OS in our Telemetry JS modules. They are not needed anymore, we should remove them to reduce code complexity. The platform is internally called "gonk": https://dxr.mozilla.org/mozilla-central/search?q=path%3Atoolkit%2Fcomponents%2Ftelemetry+%22gonk%22+ext%3Ajs+ext%3Ajsm&redirect=false We should remove any special handling for that platform.
Reporter | ||
Comment 1•7 years ago
|
||
After the changes these commands should still run through fine: - running unit tests: mach test toolkit/components/telemetry/tests/unit - running the JS linter: mach eslint toolkit/components/telemetry
Reporter | ||
Updated•7 years ago
|
Assignee: nobody → anejaalisha
Assignee | ||
Comment 2•7 years ago
|
||
I have removed the branches with 'gonk' code. Need your feedback on the same! Thanks!
Attachment #8847045 -
Flags: feedback?(gfritzsche)
Reporter | ||
Comment 3•7 years ago
|
||
Comment on attachment 8847045 [details] [diff] [review] Bug1346200.patch Review of attachment 8847045 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, this looks pretty good already! Some notes: - There is no need to change any C++ files here, that part is handled in bug 1346208. - There is an eslint error, try running: mach eslint toolkit/components/telemetry ::: toolkit/components/telemetry/TelemetryEnvironment.jsm @@ +32,1 @@ > "resource://gre/modules/LightweightThemeManager.jsm"); Please align this with the "this" on the preceding line. @@ +523,1 @@ > personaId = theme.id; Please correct the indentation here (2 spaces per indentation level). @@ +1270,5 @@ > * @return Object containing the device information data, or null if > * not a portable device. > */ > _getDeviceData() { > + if (!["android"].includes(AppConstants.platform)) { We can just change this into: if (AppConstants.platform != "android") { Lets do the same for all the other places below where there is only one element left in the array.
Attachment #8847045 -
Flags: feedback?(gfritzsche) → feedback+
Assignee | ||
Comment 4•7 years ago
|
||
Made the changes:)
Attachment #8847045 -
Attachment is obsolete: true
Attachment #8848500 -
Flags: review?(gfritzsche)
Reporter | ||
Comment 5•7 years ago
|
||
Have you tried running the tests with this patch applied?
Running:
> mach test toolkit/components/telemetry/tests/unit
... i see test failures in two tests.
Flags: needinfo?(anejaalisha)
Reporter | ||
Comment 6•7 years ago
|
||
Note that it can be easier to find the errors in the output when running only one of the failing tests at a time. E.g.: > mach test test_TelemetrySession.js Then you look for the first clear test failure in the output, e.g.: > 0:06.50 TEST_STATUS: Thread-3 test_checkSubsessionScalars FAIL [test_checkSubsessionScalars : 688] telemetry.test.unsigned_int_kind must be cleared with the new subsession. - false == true > .../obj/_tests/xpcshell/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js:test_checkSubsessionScalars:688 And start investigating what's wrong around that line.
Assignee | ||
Comment 7•7 years ago
|
||
When I was running the tests earlier, they all passed.But now when I try running this command again: mach test toolkit/components/telemetry/tests/unit I get this error: Build configuration changed. Regenerating backend. Error running mach: ['test', 'toolkit/components/telemetry/tests/unit'] The error occurred in code that was called by the mach command. This is either a bug in the called code itself or in the way that mach is calling it. You should consider filing a bug for this issue. If filing a bug, please include the full output of mach, including this error message. The details of the failure are as follows: Exception: Binary expected at /home/alisha/gecko-dev/obj-x86_64-pc-linux-gnu/dist/bin/xpcshell does not exist. File "/home/alisha/gecko-dev/testing/mach_commands.py", line 342, in test test_objects=tests, **kwargs) File "/home/alisha/gecko-dev/python/mach/mach/registrar.py", line 123, in dispatch return self._run_command_handler(handler, context=context, **kwargs) File "/home/alisha/gecko-dev/python/mach/mach/registrar.py", line 90, in _run_command_handler result = fn(**kwargs) File "/home/alisha/gecko-dev/testing/xpcshell/mach_commands.py", line 275, in run_xpcshell_test return xpcshell.run_test(**params) File "/home/alisha/gecko-dev/testing/xpcshell/mach_commands.py", line 61, in run_test return self.run_suite(**kwargs) File "/home/alisha/gecko-dev/testing/xpcshell/mach_commands.py", line 47, in run_suite return self._run_xpcshell_harness(**kwargs) File "/home/alisha/gecko-dev/testing/xpcshell/mach_commands.py", line 84, in _run_xpcshell_harness kwargs["xpcshell"] = self.get_binary_path('xpcshell') File "/home/alisha/gecko-dev/python/mozbuild/mozbuild/base.py", line 336, in get_binary_path raise Exception('Binary expected at %s does not exist.' % path) I am not sure what went wrong.
Flags: needinfo?(anejaalisha)
Assignee | ||
Comment 8•7 years ago
|
||
I got what the problem was! The tests are now passing. I have committed it and created the patch.
Assignee | ||
Comment 9•7 years ago
|
||
Attachment #8848500 -
Attachment is obsolete: true
Attachment #8848500 -
Flags: review?(gfritzsche)
Attachment #8848728 -
Flags: review?(gfritzsche)
Reporter | ||
Comment 10•7 years ago
|
||
Comment on attachment 8848728 [details] [diff] [review] Bug1346200.patch Review of attachment 8848728 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, this passes eslint & tests fine for me on desktop. However, there are some issues left. There are some logic errors in here, especially about Android functionality. With your patch applied, i still see "gonk" in toolkit/components/telemetry in: - TelemetrySession.jsm - tests/unit/head.js While i didn't mention this before, there is also a last reference in docs/data/environment.rst. Would you mind removing that as well? ::: toolkit/components/telemetry/TelemetryEnvironment.jsm @@ +28,5 @@ > XPCOMUtils.defineLazyModuleGetter(this, "ctypes", > "resource://gre/modules/ctypes.jsm"); > +Cu.import("resource://gre/modules/AddonManager.jsm"); > + XPCOMUtils.defineLazyModuleGetter(this, "LightweightThemeManager", > + "resource://gre/modules/LightweightThemeManager.jsm"); Nit: Please fix the indentation of the "XPCOMUtils..." line. @@ +518,5 @@ > async _updateAddons() { > this._environment._log.trace("_updateAddons"); > let personaId = null; > + let theme = LightweightThemeManager.currentTheme; > + personaId = theme.id; You dropped an important check here, we still need that: if (theme) { ... Otherwise we will get errors if theme is null or undefined. @@ +1292,5 @@ > version: forceToStringOrNull(getSysinfoProperty("version", null)), > locale: forceToStringOrNull(getSystemLocale()), > }; > > + if (AppConstants.platform != "android") { This checked for running on that platform, so you need to use ==. @@ +1421,5 @@ > }; > > if (AppConstants.platform === "win") { > data.isWow64 = getSysinfoProperty("isWow64", null); > + } else if (AppConstants.platform != "android") { Dito, ==. ::: toolkit/components/telemetry/TelemetryStorage.jsm @@ +105,5 @@ > */ > var Policy = { > now: () => new Date(), > getArchiveQuota: () => ARCHIVE_QUOTA_BYTES, > + getPendingPingsQuota: () => (AppConstants.platform != "android") Dito. ::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js @@ -417,5 @@ > - // on Gonk. > - if (gIsGonk) { > - Assert.ok(!("addonCompatibilityCheckEnabled" in data.settings), "Must not be available on Gonk."); > - } else { > - Assert.equal(data.settings.addonCompatibilityCheckEnabled, AddonManager.checkCompatibility); You can't drop this part. @@ +1171,1 @@ > Assert.equal(data.addons.persona, personaId, "The correct Persona Id must be reported."); Drop the "let personaId ...", just use the PERSONA_ID direct in the assert.
Attachment #8848728 -
Flags: review?(gfritzsche) → feedback+
Assignee | ||
Comment 11•7 years ago
|
||
This is the new patch with the changes you recommended in the last comment. But this patch only contains the last commits. How can I merge the previous patch and this one? Is there a process to do so or I have to use some merging tool like 'meld'?
Flags: needinfo?(gfritzsche)
Reporter | ||
Comment 12•7 years ago
|
||
Which way are you working with patches? If you are working with mercurial queues, we have an article on it here: https://developer.mozilla.org/en-US/docs/Mozilla/Mercurial/Queues#Basic_Commands ... this comes down to doing the changes and then using "qrefresh" to update the patch. If needed, i'm sure someone on IRC in #introduction can help sort issues with this quickly.
Flags: needinfo?(gfritzsche)
Assignee | ||
Comment 13•7 years ago
|
||
Attachment #8848728 -
Attachment is obsolete: true
Attachment #8849528 -
Attachment is obsolete: true
Attachment #8851225 -
Flags: review?(chutten)
Comment 14•7 years ago
|
||
Comment on attachment 8851225 [details] [diff] [review] Bug1346200.patch Review of attachment 8851225 [details] [diff] [review]: ----------------------------------------------------------------- Just one last "B2G" in toolkit/components/telemetry/docs/data/environment.rst where it's talking about kernelVersion, and we're set!
Attachment #8851225 -
Flags: review?(chutten) → review+
Assignee | ||
Comment 15•7 years ago
|
||
Attachment #8851225 -
Attachment is obsolete: true
Attachment #8853594 -
Flags: review?(chutten)
Comment 16•7 years ago
|
||
Comment on attachment 8853594 [details] [diff] [review] Bug1346200.patch Review of attachment 8853594 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, well done! Do the unit tests and eslint pass?
Attachment #8853594 -
Flags: review?(chutten) → review+
Assignee | ||
Comment 17•7 years ago
|
||
(In reply to Chris H-C :chutten from comment #16) > Comment on attachment 8853594 [details] [diff] [review] > Bug1346200.patch > > Review of attachment 8853594 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good, well done! Do the unit tests and eslint pass? Yes both the tests pass successfully.
Comment 18•7 years ago
|
||
Then we should probably get this in the tree :) Thank you for your contribution!
Keywords: checkin-needed
Comment 19•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c46e2ef0d7ed Remove B2G code branches from Telemetry JS modules. r=chutten
Keywords: checkin-needed
Comment 20•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c46e2ef0d7ed
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•