1362421 - Enable eslint on caps/tests/mochitest/browser_checkloaduri.js

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, enhancement)

Description

[Geoff Brown [:gbrown]]

In bug 1361859, eslint is enabled on caps/, but caps/tests/mochitest/browser_checkloaduri.js is still ignored.

It still has these issues:

gbrown@mozpad:~/src$ ./mach eslint caps
/home/gbrown/src/caps/tests/mochitest/browser_checkloaduri.js
  264:13  error  'testURL' is already declared in the upper scope.    no-shadow (eslint)
  265:13  error  'ssm' is already declared in the upper scope.        no-shadow (eslint)
  266:13  error  'baseFlags' is already declared in the upper scope.  no-shadow (eslint)
  267:13  error  'makeURI' is assigned a value but never used.        no-unused-vars (eslint)

✖ 4 problems (4 errors, 0 warnings)

Comment 1

[Hemant Singh Patwal]

In my case I have no errors:

HP@hemant /c/mozilla-source/mozilla-central
$ ./mach eslint caps/home/gbrown/src/caps/tests/mochitest/browser_checkloaduri.js
? 0 problems (0 errors, 0 warnings)

Comment 2

[Geoff Brown [:gbrown]]

It looks like you included the path from the output in my example, including /home/gbrown.

Try "./mach eslint caps".

Comment 3

[Hemant Singh Patwal]

(In reply to Geoff Brown [:gbrown] from comment #2)
> It looks like you included the path from the output in my example, including
> /home/gbrown.
> 
> Try "./mach eslint caps".

HP@hemant /c/mozilla-source/mozilla-central
$ ./mach eslint caps
? 0 problems (0 errors, 0 warnings)

Comment 4

[Hemant Singh Patwal]

Sorry for Comment1.
More accurately :

HP@hemant /c/mozilla-source/mozilla-central
$ ./mach eslint caps\tests\mochitest\browser_checkloaduri.js
? 0 problems (0 errors, 0 warnings)

Comment 5

[Geoff Brown [:gbrown]]

Oh, of course! This file is currently excluded from linting at https://dxr.mozilla.org/mozilla-central/rev/b21b974d60d3075ae24f6fb1bae75d0f122f28fc/.eslintignore#81. Try commenting that out and then run mach again.

Comment 6

[Hemant Singh Patwal]

Attachment: Bug 1362421 - Enable eslint on caps/tests/mochitest/browser_checkloaduri.js.

Review commit: https://reviewboard.mozilla.org/r/138596/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/138596/

Comment 7

[Hemant Singh Patwal]

I have promoted no-eval to warn as of now .What to do with it!
Now |./mach eslint --fix caps/tests/mochitest/browser_checkloaduri.js| gives only 1 warning

$ ./mach eslint --fix caps/tests/mochitest/browser_checkloaduri.js
c:\mozilla-source\mozilla-central\caps\tests\mochitest\browser_checkloaduri.js
  264:19  warning  eval can be harmful.  no-eval (eslint)

? 1 problem (0 errors, 1 warning)

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 8867003 [details]
Bug 1362421 - Enable eslint on caps/tests/mochitest/browser_checkloaduri.js.

https://reviewboard.mozilla.org/r/138596/#review141998

Thank you for the patch Hemant, this turned out a little more complicated than I expected, but should be easy to fix up in a nice way.

::: .eslintignore:81
(Diff revision 1)
>  browser/extensions/activity-stream/vendor/**
>  # imported from chromium
>  browser/extensions/mortar/**
>  
>  # caps/ exclusions
> -caps/tests/mochitest/browser_checkloaduri.js
> +#caps/tests/mochitest/browser_checkloaduri.js

These lines can just be removed now.

::: caps/tests/mochitest/.eslintrc.js:9
(Diff revision 1)
>    "extends": [
>      "plugin:mozilla/mochitest-test",
>      "plugin:mozilla/browser-test"
> -  ]
> +  ],
> +  "rules": {
> +    "no-eval": "warn",

With the changes I've suggested to browser_checkloaduri.js this shouldn't be necessary.

::: caps/tests/mochitest/browser_checkloaduri.js:264
(Diff revision 1)
>    yield BrowserTestUtils.withNewTab("http://www.example.com/", function* (browser) {
>      yield ContentTask.spawn(
>        browser,
>        testURL.toString(),
>        function* (testURLFn) {
> -        let testURL = eval("(" + testURLFn + ")");
> +        testURL = eval("(" + testURLFn + ")");

For now, lets do what we did in bug 1359350 and whitelist the eval here:

```
// eslint-disable-next-line no-eval
testURL = eval(....etc
```

::: caps/tests/mochitest/browser_checkloaduri.js:264
(Diff revision 1)
>    yield BrowserTestUtils.withNewTab("http://www.example.com/", function* (browser) {
>      yield ContentTask.spawn(
>        browser,
>        testURL.toString(),
>        function* (testURLFn) {
> -        let testURL = eval("(" + testURLFn + ")");
> +        testURL = eval("(" + testURLFn + ")");

For the no-shadow and no-unused-vars issue, I think we need to go a slightly different route.

The ContentTask.spawn (a few lines above), is indicating that this function will be run in the content process - a separate process to the main code.

Therefore, in that process these variables won't actually be defined.

Since testURL is used in both the main process and the content process, we also can't simply rename the variables in one.

I would suggest that we just whitelist these:

```
let testURL = eval("(" + testURLFn + ")"); // eslint-disable-line no-shadow
```
and do the same for ssm, baseFlags and makeURI (though makeURI should be `no-unused-vars` rather than `no-shadow`).

Comment 9

[Hemant Singh Patwal]

Comment on attachment 8867003 [details]
Bug 1362421 - Enable eslint on caps/tests/mochitest/browser_checkloaduri.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138596/diff/1-2/

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 8867003 [details]
Bug 1362421 - Enable eslint on caps/tests/mochitest/browser_checkloaduri.js.

https://reviewboard.mozilla.org/r/138596/#review142452

Thank you for the updates. Looks good now. r=Standard8.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bbe1cf88947a
Enable eslint on caps/tests/mochitest/browser_checkloaduri.js. r=standard8

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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