Closed Bug 1457835 Opened 3 years ago Closed 3 years ago

Enable ESLint on testing/mochitest

Categories

(Testing :: Mochitest, enhancement)

Product:
Testing
Component:
Mochitest
Version:
Version 3
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox61 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

Attachments

(2 files)

Bug 1457835 - Enable ESLint for testing/mochitest (automatic changes).
59 bytes, text/x-review-board-request
jmaher
: review+
Details
Bug 1457835 - Enable ESLint for testing/mochitest (manual changes).
59 bytes, text/x-review-board-request
jmaher
: review+
Details 
We should start getting the benefits of ESLint on our test harness code.
Comment on attachment 8971951 [details]
Bug 1457835 - Enable ESLint for testing/mochitest (automatic changes).

https://reviewboard.mozilla.org/r/240710/#review246668

r- for the debugger statement; let me know if I am misunderstanding something.

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:1307
(Diff revision 3)
>                                                  .then(() => {
>              let minidumpDirectory = getMinidumpDirectory();
>              let extrafile = minidumpDirectory.clone();
> -            extrafile.append(dumpID + '.extra');
> +            extrafile.append(dumpID + ".extra");
>              if (extrafile.exists()) {
>                dump(`\nNo .extra file for dumpID: ${dumpID}\n`);

how comethis isn't fixed to "

::: testing/mochitest/browser-test.js:1257
(Diff revision 3)
>    }
>  
>    if (gConfig.debugOnFailure) {
>      // You've hit this line because you requested to break into the
>      // debugger upon a testcase failure on your test run.
> -    debugger;
> +

by removing 'debugger' the entre if clause is just a comment now- I suspect this is done with a script and we need to put this back?

::: testing/mochitest/manifestLibrary.js:29
(Diff revision 3)
>        dump("TEST-SKIPPED | " + path + " | " + obj.disabled + "\n");
>        continue;
>      }
> -    if (params.testRoot != 'tests' && params.testRoot !== undefined) {
> -      name = params.baseurl + '/' + params.testRoot + '/' + path;
> -      links[name] = {'test': {'url': name, 'expected': obj['expected'], 'uses-unsafe-cpows': obj['uses-unsafe-cpows']}};
> +    if (params.testRoot != "tests" && params.testRoot !== undefined) {
> +      name = params.baseurl + "/" + params.testRoot + "/" + path;
> +      links[name] = {"test": {"url": name, "expected": obj.expected, "uses-unsafe-cpows": obj["uses-unsafe-cpows"]}};

why did we change to obj.expected, but not obj.uses-unsafe-cpows ?  both here and the else clause
Attachment #8971951 - Flags: review?(jmaher) → review-
Comment on attachment 8971952 [details]
Bug 1457835 - Enable ESLint for testing/mochitest (manual changes).

https://reviewboard.mozilla.org/r/240712/#review246670

just one question/nit

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:1302
(Diff revision 3)
>          let dumpID;
>          if (AppConstants.MOZ_CRASHREPORTER) {
>            dumpID = subject.getPropertyAsAString("dumpID");
>            if (!dumpID) {
> -            return reject("dumpID was not present despite crash reporting " +
> -                          "being enabled");
> +            reject("dumpID was not present despite crash reporting being enabled");
> +            return;

previous changes did s/reject(/return reject(/, this is doing the opposite, why?
Attachment #8971952 - Flags: review?(jmaher) → review+
can I also see a try push showing this on all tests for linux and android (can be just opt or debug)
Comment on attachment 8971951 [details]
Bug 1457835 - Enable ESLint for testing/mochitest (automatic changes).

https://reviewboard.mozilla.org/r/240710/#review246668

> how comethis isn't fixed to "

This is a template string, not a normal string.

> by removing 'debugger' the entre if clause is just a comment now- I suspect this is done with a script and we need to put this back?

Oops, yes, I'll add that back.

> why did we change to obj.expected, but not obj.uses-unsafe-cpows ?  both here and the else clause

This is the dot-notation rule: https://eslint.org/docs/rules/dot-notation

Unfortunately, you're not allowed to use dashes in dot notation, so it doesn't change those.
Comment on attachment 8971952 [details]
Bug 1457835 - Enable ESLint for testing/mochitest (manual changes).

https://reviewboard.mozilla.org/r/240712/#review246670

> previous changes did s/reject(/return reject(/, this is doing the opposite, why?

I'm not sure which changes you're referring to. In this case, ESLint is complaining that the return type is inconsistent. When I looked at the idl definition for nsIObserver, it said it returned void. So although `return reject()` is a shorthand, it really should be consistent all the way through, or not do it at all to avoid confusing readers about the actual return value.
ignore that comment about return reject; I was getting cross eyed after so many changes :)
(In reply to Joel Maher ( :jmaher - limited bugzilla access until May 1st) (UTC+2) from comment #9)
> can I also see a try push showing this on all tests for linux and android
> (can be just opt or debug)

Try push triggered via mozreview.
Comment on attachment 8971951 [details]
Bug 1457835 - Enable ESLint for testing/mochitest (automatic changes).

https://reviewboard.mozilla.org/r/240710/#review246950

thanks for updating
Attachment #8971951 - Flags: review?(jmaher) → review+
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/53867132bf19
Enable ESLint for testing/mochitest (automatic changes). r=jmaher
https://hg.mozilla.org/integration/autoland/rev/7da7e1e5be49
Enable ESLint for testing/mochitest (manual changes). r=jmaher
https://hg.mozilla.org/mozilla-central/rev/53867132bf19
https://hg.mozilla.org/mozilla-central/rev/7da7e1e5be49
Status: NEW → RESOLVED
Closed: 3 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.