1710381 - Stop using ChromeUtils.import(..., this) for importing modules in toolkit/components/telemetry/tests/unit/

Status: RESOLVED FIXED

(Toolkit :: Telemetry, task)

Description

[Mark Banner (:standard8)]

The ChromeUtils.import(..., this) form of importing modules is a legacy import format that imports directly into the global object.

This bug is about fixing the cases for toolkit/components/telemetry/tests/unit/, in particular this list.

I'm happy to mentor this bug. Note: this bug will be auto-assigned when the first patch is attached.

Here's what to do:

1. Ensure you have a [working Firefox build](https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html, an artifact build is sufficient thought it doesn't matter if you have a full build.
2. Run ./mach eslint --setup to set up ESLint, then ensure your editor has an integration set-up. It is not essential to do this, but it helps to show if you've made a mistake, or if formatting needs fixing whilst you're still in a particular file.
3. Remove the relevant lines from .eslintrc.js to enable the rule for the files.
4. Run ./mach eslint toolkit/components/telemetry/tests/unit/ to see where the errors are.
5. Fix toolkit/components/telemetry/tests/unit/head.js first. In this particular file, the imports can be incorporated into the XPCOMUtils.defineLazyModuleGetters call. To check what the import is, load the particular .jsm file and look for the EXPORTED_SYMBOLS definition.

• Note that PromiseUtils.jsm is already included in the defineLazyModuleGetters call, so that line can simply be deleted.

6. Now fix the remaining test files.

• If the module is already imported via the head.js file, then the line can be removed.
• Otherwise, the import should be replaced, e.g.

ChromeUtils.import(
  "resource://testing-common/TelemetryArchiveTesting.jsm",
  this
);

becomes

const { TelemetryArchiveTesting } = ChromeUtils.import(
  "resource://testing-common/TelemetryArchiveTesting.jsm"
);

7. Don't forget to check ESLint again after your fixes, with ./mach eslint toolkit/components/telemetry/tests/unit/. If there are formatting errors from prettier, then you can add the --fix argument to automatically fix those.
8. Check that the tests pass, use ./mach xpcshell-test toolkit/components/telemetry/tests/unit/
9. Commit the changes and submit for review with a message something like Bug nnnnnnn - Stop using this argument in ChromeUtils.import for importing modules in toolkit/components/telemetry/tests/unit/. r?Standard8.

Comment 1

[gystemd]

Hello Mark,
I'd like to work on this issue. I encountered two problems while trying to fix these imports:

1. In head.js https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/tests/unit/head.js#314-317, when I change

    "resource://gre/modules/TelemetryScheduler.jsm",
    null
  );```
to 
```let scheduler = ChromeUtils.import(
    "resource://gre/modules/TelemetryScheduler.jsm"
 );```
I get the "TypeError: can't access property "setSchedulerTickTimeout", scheduler.Policy is undefined". I can't figure out why this happens.
I tried to import the module in the defineLazyModuleGetters call but didn't help.
2. Changing the current import form of XPCOMUtils to
```const {XPCOMUtils} = ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm", this);```
makes some tests fail with the error 'SyntaxError: redeclaration of const XPCOMUtils' for some files not listed in tests you mentioned, for instance test_TelemetryController.js and test_TelemetryControllerShutdown.js. Do I have to modify this tests and delete the import of XPCOMUtils?

Thanks in advance.

Comment 2

[gystemd]

(In reply to gystemd from comment #1)

Hello Mark,
I'd like to work on this issue. I encountered two problems while trying to fix these imports:

1. In head.js https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/tests/unit/head.js#314-317, when I change

    "resource://gre/modules/TelemetryScheduler.jsm",
    null
  );```
to 
```let scheduler = ChromeUtils.import(
    "resource://gre/modules/TelemetryScheduler.jsm"
 );```
I get the "TypeError: can't access property "setSchedulerTickTimeout", scheduler.Policy is undefined". I can't figure out why this happens.
I tried to import the module in the defineLazyModuleGetters call but didn't help.
2. Changing the current import form of XPCOMUtils to
```const {XPCOMUtils} = ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm", this);```
makes some tests fail with the error 'SyntaxError: redeclaration of const XPCOMUtils' for some files not listed in tests you mentioned, for instance test_TelemetryController.js and test_TelemetryControllerShutdown.js. Do I have to modify this tests and delete the import of XPCOMUtils?

Thanks in advance.

Sorry for the bad formatting, I don't have the permissions to modify it.

Comment 3

[Lasse]

Attachment: Bug 1710381 - Stop using this argument in ChromeUtils.import for importing modules in toolkit/components/telemetry/tests/unit/. r?Standard8

We also expanded the EXPORTED_SYMBOLS lists of some .jsm files, because otherwise we got the error that these symbols were undefined.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a02a8ff49547
Stop using this argument in ChromeUtils.import for importing modules in toolkit/components/telemetry/tests/unit/. r=Standard8,chutten

Comment 5

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/a02a8ff49547