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

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?

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

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