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

RESOLVED FIXED in Firefox 55

Status

RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: gbrown, Assigned: hemantsingh1612, Mentored)

Tracking

({good-first-bug})

Trunk
mozilla55
good-first-bug

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [lang=js])

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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]
(Assignee)

Comment 1

2 years ago
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)
(Reporter)

Comment 2

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

Try "./mach eslint caps".
(Assignee)

Comment 3

2 years ago
(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)
(Assignee)

Comment 4

2 years ago
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)
(Reporter)

Comment 5

2 years ago
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 hidden (mozreview-request)
(Assignee)

Comment 7

2 years ago
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 8

2 years ago
mozreview-review
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 hidden (mozreview-request)

Comment 10

2 years ago
mozreview-review
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+

Comment 11

2 years ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bbe1cf88947a
Enable eslint on caps/tests/mochitest/browser_checkloaduri.js. r=standard8

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bbe1cf88947a
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Updated

9 months ago
Product: Testing → Firefox Build System
You need to log in before you can comment on or make changes to this bug.