Closed Bug 1367235 Opened 3 years ago Closed 3 years ago

Enable eslint on testing/xpcshell


(Firefox Build System :: Lint and Formatting, enhancement)

Not set


(firefox55 fixed)

Tracking Status
firefox55 --- fixed


(Reporter: gbrown, Assigned: gbrown)




(2 files, 1 obsolete file)

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 */
> +    _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

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]:

Attachment #8870830 - Flags: review?(standard8) → review+
Pushed by
Enable eslint on testing/xpcshell - mechanical updates; r=Standard8
Additional changes for eslint on testing/xpcshell; r=Standard8
Closed: 3 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.