Enable eslint on testing/xpcshell

RESOLVED FIXED in Firefox 55

Status

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: gbrown, Assigned: gbrown)

Tracking

(Blocks 1 bug)

Trunk
mozilla55
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

No description provided.
There's a lot to do under testing/, so this bug only concerns itself with testing/xpcshell. 

This first patch has the simple, mechanical changes - white space, bracket placement and such.
Attachment #8870581 - Flags: review?(standard8)
Attachment #8870581 - Flags: review?(standard8) → review+
Comment on attachment 8870583 [details] [diff] [review]
additional changes

Review of attachment 8870583 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there, I think we can just clarify where some of the globals are coming from though.

::: testing/xpcshell/dbg-actors.js
@@ +4,5 @@
>  
>  "use strict";
>  
> +/* import-globals-from head.js */
> +/* globals Cu, DebuggerServer */

I think you can drop both of these and use just:

/* import-globals-from ../../devtools/server/main.js */

(that seems to be loaded into the same scope as this file by head.js).

::: testing/xpcshell/head.js
@@ +13,5 @@
> +/* the following are generally defined by the harness */
> +/* globals _HEAD_FILES, _HEAD_JS_PATH, _JSDEBUGGER_PORT, _JSCOV_DIR,
> +    _MOZINFO_JS_PATH, _TEST_FILE, _TEST_NAME, _TESTING_MODULES_DIR,
> +    _XPCSHELL_PROCESS:true */
> +/* globals Cc, Ci, load, run_test, sendCommand */

For load and sendCommand, I think we should put them in a separate comment section and point to the fact they're defined by XPCShellImpl.cpp

https://dxr.mozilla.org/mozilla-central/rev/96e18bec9fc8a5ce623c16167c12756bbe190d73/js/xpconnect/src/XPCShellImpl.cpp#672

For run_test, I think we should do a `// eslint-disable-line no-undef` for that line.

For Cc and Ci, please add a reference that these are currently expected to be defined by the test files that use the functions they are in. Unfortunately the Cc/Ci/Cu/Cr story is a mess. I think I've thought of a way around this, but it will take a different bug to get it fixed.
Attachment #8870583 - Flags: review?(standard8)
Updated for initial review comments.

Can you clarify why you prefer eslint-disable-line vs globals for run_test?
Attachment #8870583 - Attachment is obsolete: true
Attachment #8870830 - Flags: review?(standard8)
(In reply to Geoff Brown [:gbrown] from comment #4)
> Can you clarify why you prefer eslint-disable-line vs globals for run_test?

I was originally thinking about it that I didn't want this file declaring it as globals for other files. However, it is also inconsistent with what I've done before. So maybe we should put it back to a global definition and just add a comment that it may be declared in test files.
Comment on attachment 8870830 [details] [diff] [review]
additional changes

Review of attachment 8870830 [details] [diff] [review]:
-----------------------------------------------------------------

r=Standard8
Attachment #8870830 - Flags: review?(standard8) → review+
Pushed by gbrown@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cd8e5586752
Enable eslint on testing/xpcshell - mechanical updates; r=Standard8
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d7a8241dfb0
Additional changes for eslint on testing/xpcshell; r=Standard8
https://hg.mozilla.org/mozilla-central/rev/5cd8e5586752
https://hg.mozilla.org/mozilla-central/rev/0d7a8241dfb0
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Product: Testing → Firefox Build System
You need to log in before you can comment on or make changes to this bug.