Closed Bug 1357625 Opened 5 years ago Closed 5 years ago

Assert.rejects should resolve with the expected rejection

Categories

(Testing :: General, enhancement)

Version 3
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: markh, Assigned: markh)

Details

Attachments

(1 file)

Back in bug 984172 we added Assert.rejects, but the promise returned by that function always resolves with undefined. It would be helpful if it resolved with the expected exception, so code could do something like:

> let err = await Assert.rejects(someFunction());
> equal(err.something, "something");

To do more test-specific checking of the exception.

Mike, you reviewed bug 984172, so I'll be asking you for review - please feel free to redirect if you desire.
Comment on attachment 8859425 [details]
Bug 1357625 - Have Assert.rejects resolve with the expected exception.

https://reviewboard.mozilla.org/r/131448/#review134254

Cool! Thanks for this :)
Attachment #8859425 - Flags: review?(mdeboer) → review+
Pushed by mhammond@skippinet.com.au:
https://hg.mozilla.org/integration/autoland/rev/cc96b54811f6
Have Assert.rejects resolve with the expected exception. r=mikedeboer
Backed out for failing xpcshell's test_PromiseUtils.js:

https://hg.mozilla.org/integration/autoland/rev/64e79a1b9c76e94e1bd0a56ab8d1497f2afad6bc

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=cc96b54811f67c5c092574bfbc070b8cd04180b0&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=92698658&repo=autoland

[task 2017-04-19T14:19:47.853456Z] 14:19:47     INFO -  TEST-PASS | toolkit/modules/tests/xpcshell/test_PromiseUtils.js | test_reject_resolved_promise - [test_reject_resolved_promise : 1] Rejection with a rejected promise uses the passed promise itself as the reason of rejection
[task 2017-04-19T14:19:47.857327Z] 14:19:47     INFO -  Unexpected exception Error: This one rejects at /home/worker/workspace/build/tests/xpcshell/tests/toolkit/modules/tests/xpcshell/test_PromiseUtils.js:102
[task 2017-04-19T14:19:47.859270Z] 14:19:47     INFO -  test_reject_resolved_promise/p<@/home/worker/workspace/build/tests/xpcshell/tests/toolkit/modules/tests/xpcshell/test_PromiseUtils.js:102:51
[task 2017-04-19T14:19:47.861202Z] 14:19:47     INFO -  test_reject_resolved_promise@/home/worker/workspace/build/tests/xpcshell/tests/toolkit/modules/tests/xpcshell/test_PromiseUtils.js:102:11
[task 2017-04-19T14:19:47.864177Z] 14:19:47     INFO -  _run_next_test@/home/worker/workspace/build/tests/xpcshell/head.js:1554:9
[task 2017-04-19T14:19:47.865994Z] 14:19:47     INFO -  run@/home/worker/workspace/build/tests/xpcshell/head.js:703:9
[task 2017-04-19T14:19:47.867799Z] 14:19:47     INFO -  _do_main@/home/worker/workspace/build/tests/xpcshell/head.js:211:5
[task 2017-04-19T14:19:47.873049Z] 14:19:47     INFO -  _execute_test@/home/worker/workspace/build/tests/xpcshell/head.js:546:5
[task 2017-04-19T14:19:47.874791Z] 14:19:47     INFO -  @-e:1:1
[task 2017-04-19T14:19:47.876491Z] 14:19:47     INFO -  exiting test
[task 2017-04-19T14:19:47.878527Z] 14:19:47  WARNING -  TEST-UNEXPECTED-FAIL | toolkit/modules/tests/xpcshell/test_PromiseUtils.js | test_reject_resolved_promise - [test_reject_resolved_promise : 240] Unable to find a rejection expected by expectUncaughtRejection. - 1 == 0
[task 2017-04-19T14:19:47.880488Z] 14:19:47     INFO -  resource://testing-common/PromiseTestUtils.jsm:assertNoMoreExpectedRejections:240
[task 2017-04-19T14:19:47.882277Z] 14:19:47     INFO -  /home/worker/workspace/build/tests/xpcshell/head.js:_execute_test:635
[task 2017-04-19T14:19:47.884004Z] 14:19:47     INFO -  -e:null:1
[task 2017-04-19T14:19:47.885845Z] 14:19:47     INFO -  exiting test
[task 2017-04-19T14:19:47.889459Z] 14:19:47     INFO -  PID 11975 | JavaScript error: , line 0: uncaught exception: 2147500036
[task 2017-04-19T14:19:47.891173Z] 14:19:47     INFO -  <<<<<<<
Flags: needinfo?(markh)
Comment on attachment 8859425 [details]
Bug 1357625 - Have Assert.rejects resolve with the expected exception.

https://reviewboard.mozilla.org/r/131448/#review134662

::: testing/modules/Assert.jsm:395
(Diff revision 2)
>          if (expected && !expectedException(err, expected)) {
>            reject(err);
>            return;
>          }
>          this.report(false, err, expected, message);
> -        resolve();
> +        // We resolve with the expected exception, although if that is itself

test_PromiseUtils demonstrated an edge-case in my patch - when the exception is an already rejected promise, rejects() would then resolve with that rejected promise, causing Assert.rejects to fail.

I "fixed" this by ensuring that the value Assert.rejects with is never a promise, but always the resolution/rejection value instead.

This also means test_PromiseUtils had to change as the rejected promise is no longer unhandled - it's handled by the result of Assert.rejects.

I asked Paolo for review here as he "owns" the unresolved promise warnings. I *think* this change makes sense, but if either of you disagree with this change I'm happy to close this bug as WONTFIX and have the individual tests which want this change to do something locally.
Assignee: nobody → markh
Flags: needinfo?(markh)
Comment on attachment 8859425 [details]
Bug 1357625 - Have Assert.rejects resolve with the expected exception.

> let err = await Assert.rejects(someFunction());
> equal(err.something, "something");

Hm, no, that gives us two different patterns to do the same thing.

I think we should encourage the existing pattern instead:

await Assert.rejects(someFunction(),
                     reason => reason.something == "something");

...which mirrors what "Assert.throws" does. You can always capture the reason to a local variable if you have to use it later.

Improving the "rejects" implementation is something we could still do though. The function always rejects if the called function rejects but the reason doesn't match. This isn't always correct.

The correct behavior is the one we currently have if the called function doesn't reject. This behavior depends on whether "report" throws an exception, which I believe is dependent on whether assertions are fatal, that is true for xpcshell, and false for browser tests.

We should probably have a behavior dependent on whether assertions are fatal, in all circumstances. It may be easier to just rewrite "Assert.rejects" as an async function, then calling "report" would do the right thing.
Attachment #8859425 - Flags: review?(paolo.mozmail) → review-
(In reply to :Paolo Amadini from comment #7)
> Hm, no, that gives us two different patterns to do the same thing.
> 
> I think we should encourage the existing pattern instead:
> 
> await Assert.rejects(someFunction(),
>                      reason => reason.something == "something");

huh - I missed that feature existed! I'll do that, thanks!
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.