Remove B2G code branches from Telemetry JS modules

RESOLVED FIXED in Firefox 55

Status

()

P4
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: gfritzsche, Assigned: anejaalisha)

Tracking

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [measurement:client])

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

2 years ago
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

2 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

2 years ago
Assignee: nobody → anejaalisha
(Assignee)

Comment 2

2 years ago
Posted patch Bug1346200.patch (obsolete) — Splinter Review
I have removed the branches with 'gonk' code. Need your feedback on the same!
Thanks!
Attachment #8847045 - Flags: feedback?(gfritzsche)
(Reporter)

Comment 3

2 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

2 years ago
Posted patch Bug1346200.patch (obsolete) — Splinter Review
Made the changes:)
Attachment #8847045 - Attachment is obsolete: true
Attachment #8848500 - Flags: review?(gfritzsche)
(Reporter)

Comment 5

2 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

2 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

2 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

2 years ago
I got what the problem was! The tests are now passing.
I have committed it and created the patch.
(Assignee)

Comment 9

2 years ago
Posted patch Bug1346200.patch (obsolete) — Splinter Review
Attachment #8848500 - Attachment is obsolete: true
Attachment #8848500 - Flags: review?(gfritzsche)
Attachment #8848728 - Flags: review?(gfritzsche)
(Reporter)

Comment 10

2 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

2 years ago
Posted patch Bug1346200.patch (obsolete) — Splinter Review
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

2 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

2 years ago
Posted patch Bug1346200.patch (obsolete) — Splinter Review
Attachment #8848728 - Attachment is obsolete: true
Attachment #8849528 - Attachment is obsolete: true
Attachment #8851225 - Flags: review?(chutten)
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

2 years ago
Attachment #8851225 - Attachment is obsolete: true
Attachment #8853594 - Flags: review?(chutten)
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

2 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.
Then we should probably get this in the tree :)

Thank you for your contribution!
Keywords: checkin-needed

Comment 19

2 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c46e2ef0d7ed
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.