Closed Bug 1452706 Opened 2 years ago Closed Last year

[meta] Assert.rejects/throws should not allow an optional "expected" argument

Categories

(Testing :: General, enhancement, P3)

Version 3
enhancement

Tracking

(firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

(Keywords: meta)

Attachments

(4 files)

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.
Assignee: nobody → standard8
Status: NEW → ASSIGNED
Summary: Assert.rejects/throws should not allow an optional "expected" argument → [meta] Assert.rejects/throws should not allow an optional "expected" argument
Depends on: 1461997
Depends on: 1459161
Depends on: 1463499
Depends on: 1463584
Depends on: 1463673
Priority: -- → P3
Depends on: 1465385
Depends on: 1465530
Depends on: 1465762
Depends on: 1466497
Depends on: 1468938
Depends on: 1470396
Depends on: 1470455
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!
Attachment #8989566 - Flags: review?(ystartsev) → review+
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'.
Attachment #8989567 - Flags: review?(mdeboer) → review+
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!!
Attachment #8989568 - Flags: review?(mdeboer) → review+
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
Flags: needinfo?(standard8)
This was a conflict with another bug landing nearby. I've just added a patch to fix that.
Flags: needinfo?(standard8)
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
Attachment #8989997 - Flags: review?(ystartsev) → review+
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
You need to log in before you can comment on or make changes to this bug.