Closed Bug 1362421 Opened 3 years ago Closed 3 years ago

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

Categories

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

enhancement
Not set
normal

Tracking

(firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: gbrown, Assigned: hemantsingh1612, Mentored)

References

Details

(Keywords: good-first-bug, Whiteboard: [lang=js])

Attachments

(1 file)

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)
Mentor: standard8
Keywords: good-first-bug
Whiteboard: [lang=js]
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)
It looks like you included the path from the output in my example, including /home/gbrown.

Try "./mach eslint caps".
(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)
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)
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.
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)
Flags: needinfo?(standard8)
Assignee: nobody → hemantsingh1612
Flags: needinfo?(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`).
Attachment #8867003 - Flags: review?(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.
Attachment #8867003 - Flags: review?(standard8) → review+
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bbe1cf88947a
Enable eslint on caps/tests/mochitest/browser_checkloaduri.js. r=standard8
https://hg.mozilla.org/mozilla-central/rev/bbe1cf88947a
Status: NEW → RESOLVED
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.