Whilst enabling ESLint on more code recently, and as a result of other items, I'm finding various issues with the use of Assert.rejects and Assert.throws: - Assert.rejects with missing await (which can cause the wrong exception to be caught). - rejects/throws are called without the `expected` parameter and don't actually test what they intended to (e.g. undefined item, instead of a throw). - rejects/throws are called with string arguments for the `expected` parameter (which isn't correct, as it is seen as the message). - Fixing a rejects test actually demonstrated the rejection messages weren't as good as the developers thought they were. Given all the above, I think we should start requiring the second, `expected` parameter to Assert.rejects/throws, as well as the await for Assert.rejects. For the current files we cover via ESLint there are: 21 missing await for Assert.rejects 64 with only one argument for Assert.rejects/throws 76 with a non-expected second argument for Assert.reject/throws, e.g. either they've just supplied the message (as the current API allows), or they've done the wrong thing - manual inspection spotted multiple cases of trying to match a string but it wasn't a regexp. Seeing as these are slightly more difficult to fix as they generally need investigation, I think we should implement an ESLint rule initially, then later we can change Assert.jsm so that the second argument is enforced.

I'm currently working towards this. The plan is: 1a) Create an ESLint rule for detecting these issues. 1b) Enable the rule throughout the tree, whitelisting or a directory bases where we have failures. 2) Work to get the rule fully enabled across the tree. 3) Replace the rule with an assertion in Assert.jsm and fix any remaining instances. The ESLint rule is a cheaper way to allow the work to be done incrementally (I suspect some co-ordination is going to be required as to the right thing to check for in some cases), and prevent too many new instances coming up. I suspect the missing await warrants a second rule, which will stay enabled all the time.

Try push is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd05600a66c47b2065d5156d3329b878662914ce

Comment on attachment 8989566 [details] Bug 1452706 - Add the 'expected' arguments to throws/rejects for devtools/shared/sourcemap. https://reviewboard.mozilla.org/r/254594/#review261556 This looks good to me, thanks for adding those!

Comment on attachment 8989567 [details] Bug 1452706 - Make the 'expected' argument to Assert.throws/rejects required rather than optional. https://reviewboard.mozilla.org/r/254596/#review261604 ::: testing/modules/Assert.jsm:330 (Diff revision 1) > */ > proto.notStrictEqual = function notStrictEqual(actual, expected, message) { > this.report(actual === expected, actual, expected, message, "!=="); > }; > > +function checkExpected(instance, funcName, expected) { Perhaps `checkExpectedArgument` would be a wee bit clearer? Albeit not much :) A comment above it that briefly explains its purpose would help regardless! ::: testing/modules/Assert.jsm:336 (Diff revision 1) > + if (!expected) { > + instance.ok(false, `Error: The 'expected' argument was not supplied to Assert.${funcName}()`); > + } > + > + if (!instanceOf(expected, "RegExp") && > + !(typeof expected === "function") && ```js typeof expected != "function" && typeof expected != "object") { ``` ::: testing/modules/Assert.jsm:377 (Diff revision 1) > * ``` > * > * @param block > * (function) Function block to evaluate and catch eventual thrown errors > * @param expected (optional) > - * (mixed) This parameter can be either a RegExp, a function, or a string. The > + * (mixed) This parameter can be either a RegExp, a function. The nit: 'RegExp or a function'.

Comment on attachment 8989568 [details] Bug 1452706 - Remove the now redundant ESLint rule require-expected-throws-or-rejects. https://reviewboard.mozilla.org/r/254598/#review261606 rs=me :) Thanks Mark!!

Pushed by mbanner@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/626c790eb6f3 Add the 'expected' arguments to throws/rejects for devtools/shared/sourcemap. r=yulia https://hg.mozilla.org/integration/autoland/rev/4fbd34b0807a Make the 'expected' argument to Assert.throws/rejects required rather than optional. r=mikedeboer https://hg.mozilla.org/integration/autoland/rev/6abc5dc6baaf Remove the now redundant ESLint rule require-expected-throws-or-rejects. r=mikedeboer

Backed out 3 changesets (bug 1452706) for xpcshell failures at devtools/server/tests/unit/test_format_command.js Log: https://treeherder.mozilla.org/logviewer.html#?job_id=186471056&repo=autoland&lineNumber=2041 TEST-START | browser/extensions/formautofill/test/unit/test_sync.js [task 2018-07-04T17:42:29.097Z] INFO - TEST-PASS | browser/extensions/formautofill/test/unit/test_sync.js | took 14033ms [task 2018-07-04T17:42:29.105Z] INFO - TEST-START | devtools/server/tests/unit/test_format_command.js [task 2018-07-04T17:42:29.875Z] WARNING - TEST-UNEXPECTED-FAIL | devtools/server/tests/unit/test_format_command.js | xpcshell return code: 0 [task 2018-07-04T17:42:29.876Z] INFO - TEST-INFO took 765ms [task 2018-07-04T17:42:29.877Z] INFO - >>>>>>> [task 2018-07-04T17:42:29.878Z] INFO - (xpcshell/head.js) | test MAIN run_test pending (1) [task 2018-07-04T17:42:29.880Z] INFO - TEST-PASS | devtools/server/tests/unit/test_format_command.js | run_test/< - [run_test/< : 90] "help()" == "help()" [task 2018-07-04T17:42:29.881Z] INFO - TEST-PASS | devtools/server/tests/unit/test_format_command.js | run_test/< - [run_test/< : 90] "screenshot({\\"fullscreen\\":true})" == "screenshot({\\"fullscreen\\":true})" [task 2018-07-04T17:42:29.882Z] INFO - TEST-PASS | devtools/server/tests/unit/test_format_command.js | run_test/< - [run_test/< : 90] "screenshot({\\"fullscreen\\":true})" == "screenshot({\\"fullscreen\\":true})" [task 2018-07-04T17:42:29.883Z] INFO - TEST-PASS | devtools/server/tests/unit/test_format_command.js | run_test/< - [run_test/< : 90] "screenshot()" == "screenshot()" [task 2018-07-04T17:42:29.884Z] INFO - TEST-PASS | devtools/server/tests/unit/test_format_command.js | run_test/< - [run_test/< : 90] "screenshot({\\"dpr\\":0.5,\\"fullpage\\":true,\\"chrome\\":true})" == "screenshot({\\"dpr\\":0.5,\\"fullpage\\":true,\\"chrome\\":true})" [task 2018-07-04T17:42:29.885Z] INFO - TEST-PASS | devtools/server/tests/unit/test_format_command.js | run_test/< - [run_test/< : 90] "screenshot({\\"filename\\":\\"filename\\"})" == "screenshot({\\"filename\\":\\"filename\\"})" [task 2018-07-04T17:42:29.886Z] INFO - TEST-PASS | devtools/server/tests/unit/test_format_command.js | run_test/< - [run_test/< : 90] "screenshot({\\"filename\\":\\"filename\\"})" == "screenshot({\\"filename\\":\\"filename\\"})" [task 2018-07-04T17:42:29.889Z] INFO - TEST-PASS | devtools/server/tests/unit/test_format_command.js | run_test/< - [run_test/< : 90] "screenshot({\\"name\\":[\\"filename\\",\\"filename\\",\\"filename\\"]})" == "screenshot({\\"name\\":[\\"filename\\",\\"filename\\",\\"filename\\"]})" [task 2018-07-04T17:42:29.889Z] INFO - TEST-PASS | devtools/server/tests/unit/test_format_command.js | run_test/< - [run_test/< : 90] "screenshot({\\"filename\\":\\"filename1\\"})" == "screenshot({\\"filename\\":\\"filename1\\"})" [task 2018-07-04T17:42:29.890Z] INFO - TEST-PASS | devtools/server/tests/unit/test_format_command.js | run_test/< - [run_test/< : 90] "screenshot({\\"chrome\\":true})" == "screenshot({\\"chrome\\":true})" [task 2018-07-04T17:42:29.891Z] INFO - TEST-PASS | devtools/server/tests/unit/test_format_command.js | run_test/< - [run_test/< : 90] "screenshot({\\"filename\\":\\"file name with spaces\\"})" == "screenshot({\\"filename\\":\\"file name with spaces\\"})" [task 2018-07-04T17:42:29.891Z] INFO - TEST-PASS | devtools/server/tests/unit/test_format_command.js | run_test/< - [run_test/< : 90] "screenshot({\\"filename\\":\\"filename1\\",\\"name\\":\\"filename2\\"})" == "screenshot({\\"filename\\":\\"filename1\\",\\"name\\":\\"filename2\\"})" [task 2018-07-04T17:42:29.892Z] INFO - TEST-PASS | devtools/server/tests/unit/test_format_command.js | run_test/< - [run_test/< : 90] "screenshot({\\"name\\":\\"filename1\\",\\"filename\\":\\"filename2\\"})" == "screenshot({\\"name\\":\\"filename1\\",\\"filename\\":\\"filename2\\"})" [task 2018-07-04T17:42:29.892Z] INFO - TEST-PASS | devtools/server/tests/unit/test_format_command.js | run_test/< - [run_test/< : 90] "screenshot({\\"filename\\":\\"fo\\\\\\\\\\\\\\"o bar\\"})" == "screenshot({\\"filename\\":\\"fo\\\\\\\\\\\\\\"o bar\\"})" [task 2018-07-04T17:42:29.893Z] INFO - TEST-PASS | devtools/server/tests/unit/test_format_command.js | run_test/< - [run_test/< : 90] "screenshot({\\"filename\\":\\"foo b\\\\\\\\\\\\\\"ar\\"})" == "screenshot({\\"filename\\":\\"foo b\\\\\\\\\\\\\\"ar\\"})" [task 2018-07-04T17:42:29.893Z] WARNING - TEST-UNEXPECTED-FAIL | devtools/server/tests/unit/test_format_command.js | run_test/< - [run_test/< : 94] Error: The 'expected' argument to Assert.throws() must be a RegExp, function or an object - false == true [task 2018-07-04T17:42:29.894Z] INFO - /builds/worker/workspace/build/tests/xpcshell/tests/devtools/server/tests/unit/test_format_command.js:run_test/<:94 [task 2018-07-04T17:42:29.895Z] INFO - /builds/worker/workspace/build/tests/xpcshell/tests/devtools/server/tests/unit/test_format_command.js:run_test:93 [task 2018-07-04T17:42:29.895Z] INFO - /builds/worker/workspace/build/tests/xpcshell/head.js:_execute_test:526 [task 2018-07-04T17:42:29.896Z] INFO - -e:null:1 [task 2018-07-04T17:42:29.897Z] INFO - exiting test [task 2018-07-04T17:42:29.897Z] INFO - <<<<<<< Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=6abc5dc6baaffcb18d235cb9474a69a4313d37d1 Backout: https://hg.mozilla.org/integration/autoland/rev/1fa4c7ae50bf91288d4b8eca72ae21c96324da28

This was a conflict with another bug landing nearby. I've just added a patch to fix that.

Comment on attachment 8989997 [details] Bug 1452706 - Fix the expected arguments to be RegExps in test_format_command.js. https://reviewboard.mozilla.org/r/255018/#review261844 Looks great, thanks for catching that

Comment on attachment 8989997 [details] Bug 1452706 - Fix the expected arguments to be RegExps in test_format_command.js. https://reviewboard.mozilla.org/r/255018/#review261846

Pushed by mbanner@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/208e3e0cc450 Add the 'expected' arguments to throws/rejects for devtools/shared/sourcemap. r=yulia https://hg.mozilla.org/integration/autoland/rev/d6d7246a5ebe Make the 'expected' argument to Assert.throws/rejects required rather than optional. r=mikedeboer https://hg.mozilla.org/integration/autoland/rev/102075df8629 Remove the now redundant ESLint rule require-expected-throws-or-rejects. r=mikedeboer https://hg.mozilla.org/integration/autoland/rev/3a69ff8923a7 Fix the expected arguments to be RegExps in test_format_command.js. r=yulia

https://hg.mozilla.org/mozilla-central/rev/208e3e0cc450 https://hg.mozilla.org/mozilla-central/rev/d6d7246a5ebe https://hg.mozilla.org/mozilla-central/rev/102075df8629 https://hg.mozilla.org/mozilla-central/rev/3a69ff8923a7