1367235 - Enable eslint on testing/xpcshell

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, enhancement)

Comment 1

[Geoff Brown [:gbrown]]

Attachment: enable eslint on testing/xpcshell - mechanical changes

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.

Comment 2

[Geoff Brown [:gbrown]]

Attachment: additional changes

https://treeherder.mozilla.org/#/jobs?repo=try&revision=caca304864ceeb4194718b4d6e8147f9abd83550

Comment 3

[Mark Banner (:standard8)]

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.

Comment 4

[Geoff Brown [:gbrown]]

Attachment: additional changes

Updated for initial review comments.

Can you clarify why you prefer eslint-disable-line vs globals for run_test?

Comment 5

[Mark Banner (: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 6

[Mark Banner (:standard8)]

Comment on attachment 8870830 [details] [diff] [review]
additional changes

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

r=Standard8

Comment 7

[Pulsebot]

Pushed by [e-mail]:
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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5cd8e5586752
https://hg.mozilla.org/mozilla-central/rev/0d7a8241dfb0