Closed Bug 1229142 Opened 4 years ago Closed 4 years ago

Create some eslintrc defaults for the main test suites

Categories

(Testing :: General, defect)

defect
Not set

Tracking

(firefox46 fixed)

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: mossop, Assigned: mossop)

References

Details

Attachments

(2 files)

Each test suite defines a few predefined globals for test files to use. We should share those in eslintrc files that can be included from the test directories.
Bug 1229142: Create some eslintrc defaults for the main test suites. r?Standard8
Attachment #8693879 - Flags: review?(standard8)
Blocks: eslint
Comment on attachment 8693879 [details]
MozReview Request: Bug 1229142: Link browser and toolkit test directory to the shared eslintrc files. r?Standard8

https://reviewboard.mozilla.org/r/26607/#review24419

::: testing/mochitest/browser.eslintrc:5
(Diff revision 1)
> +    "no-unused-vars": [1, {"vars": "local", "args": "none"}]

Isn't http://mxr.mozilla.org/mozilla-central/source/testing/eslint-plugin-mozilla/lib/rules/import-headjs-globals.js meant to help with this?

If so, I don't think we should be defining this rule.

If that doesn't, then I think this should probably be a '2', we shouldn't have any excuse for unused variables.

::: testing/mochitest/browser.eslintrc:10
(Diff revision 1)
> +    "add_task": true,

I don't know why devtools has these all as 'true' - that means they're writeable and I think we shouldn't be overwriting/assigning to most of these (gBrowser might be one valid case).

::: testing/mochitest/browser.eslintrc:32
(Diff revision 1)
> +    "setTimeout": true,

I'd have thought setTimeout would be part of the standard "browser" env specified in the .eslintrc. So I'm not sure its really needed here.
Attachment #8693879 - Flags: review?(standard8)
https://reviewboard.mozilla.org/r/26607/#review24419

> Isn't http://mxr.mozilla.org/mozilla-central/source/testing/eslint-plugin-mozilla/lib/rules/import-headjs-globals.js meant to help with this?
> 
> If so, I don't think we should be defining this rule.
> 
> If that doesn't, then I think this should probably be a '2', we shouldn't have any excuse for unused variables.

It helps with undeclared variables that are defined in head.js and referenced in other files. It doesn't help with variables defined in head.js but not used elsewhere in head.js.

I think it makes more sense to explicitly add `/* eslint-disable no-unused-vars */` or `/* exported ... */` to head.js files, though, or extend the import-headjs-globals rule to mark them as used. I want to know about unused globals in my tests.
Blocks: 1231827
(In reply to Kris Maglione [:kmag] from comment #3)
> https://reviewboard.mozilla.org/r/26607/#review24419
> 
> > Isn't http://mxr.mozilla.org/mozilla-central/source/testing/eslint-plugin-mozilla/lib/rules/import-headjs-globals.js meant to help with this?
> > 
> > If so, I don't think we should be defining this rule.
> > 
> > If that doesn't, then I think this should probably be a '2', we shouldn't have any excuse for unused variables.
> 
> It helps with undeclared variables that are defined in head.js and
> referenced in other files. It doesn't help with variables defined in head.js
> but not used elsewhere in head.js.
> 
> I think it makes more sense to explicitly add `/* eslint-disable
> no-unused-vars */` or `/* exported ... */` to head.js files, though, or
> extend the import-headjs-globals rule to mark them as used. I want to know
> about unused globals in my tests.

Yeah I think that makes more sense, in the interest of getting this landed though I'm going to leave it in then file a new bug to take care of removing it and cleaning up any errors it introduces.
Depends on: 1229224
Comment on attachment 8693879 [details]
MozReview Request: Bug 1229142: Link browser and toolkit test directory to the shared eslintrc files. r?Standard8

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26607/diff/1-2/
Attachment #8693879 - Attachment description: MozReview Request: Bug 1229142: Create some eslintrc defaults for the main test suites. r?Standard8 → MozReview Request: Bug 1229142: Link browser and toolkit test directory to the shared eslintrc files. r?Standard8
Attachment #8693879 - Flags: review?(standard8)
Attachment #8700058 - Flags: review?(standard8) → review+
Comment on attachment 8700058 [details]
MozReview Request: Bug 1229142: Add shared eslintrc files for the different test suites. r?Standard8

https://reviewboard.mozilla.org/r/28549/#review25515

Looks good. I've not been able to test it as eslint seems messed up locally for me, but the changes look good now.
Comment on attachment 8693879 [details]
MozReview Request: Bug 1229142: Link browser and toolkit test directory to the shared eslintrc files. r?Standard8

https://reviewboard.mozilla.org/r/26607/#review25517
Attachment #8693879 - Flags: review?(standard8) → review+
https://hg.mozilla.org/mozilla-central/rev/f13750b0247d
https://hg.mozilla.org/mozilla-central/rev/f52dbc5e835f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Blocks: 1237122
You need to log in before you can comment on or make changes to this bug.