984172 - Add an Assert.jsm helper to verify a promise is rejected

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

Assert.jsm has a useful Assert.throws method, but it's not obvious how this could be (ab)used to test a promise is rejected.  Without this, people may roll their own pattern which is ineffective - eg, see bug 983913.

In that bug we introduced a (local) function Assert_rejects - http://hg.mozilla.org/mozilla-central/file/c95bf0629658/services/sync/tests/unit/test_browserid_identity.js#l24 (although note that function will soon be moved to services/sync/modules-testing/fxa_utils.js)

It's not clear that implementation is optimal (eg, it doesn't throw/reject with an AssertionError on failure, which could be argued functions in Assert.jsm should), but it's enough to get discussion started.

CCing people I could see involved in the landing of Assert.jsm

Comment 1

[Mike de Boer [:mikedeboer]]

Mark, are you interested in taking this? I think it'd be a *very* valuable addition to Assert.jsm!

Comment 2

[Mark Hammond [:markh] [:mhammond]]

(In reply to Mike de Boer [:mikedeboer] from comment #1)
> Mark, are you interested in taking this? I think it'd be a *very* valuable
> addition to Assert.jsm!

Sure - so long as it meets the backlog criteria :)

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Attachment: WIP

This is a simple WIP patch, but I'm a little confused by assert.jsm's semantics for error checking.  With this patch I see:

 0:03.73 Full message: TypeError: invalid 'instanceof' operand (new Error("oh no", "o:/src/mozilla-git/gecko-dev/obj-release/_tests/xpcshell/testing/modules/tests/xpcshell/test_assert.js", 309))

which confuses me - it seems the Assert.jsm code that is checking the exception matches if failing with an invalid instanceof.  I get the same result with plain strings as well as error objects.

Mike, any clues?

Comment 4

[Mike de Boer [:mikedeboer]]

Mark, this is really strange.

So I found the formal spec-y reason of this failure at http://stackoverflow.com/questions/6021245/what-does-this-instanceof-error-message-mean

IOW: `expected`, the rightmost value (but possibly `actual` as well), is missing the internal [[HasInstance]] method. I don't know how or why this would happen, but I'm suspecting Promise.jsm.

Comment 5

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-984172-part-1-add-Assert.rejects.-r-mikedeboer.patch

Clearing out some of my old patch queue.  This seems to do what we want - see "part 2" for actual usage.

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0002-Bug-984172-part-2-replace-Assert_rejects-with-Assert.patch

This replaces existing Assert_rejects calls with Assert.rejects.  Putting the finger on ttaubert as he reviewed the addition of Assert_rejects in the first place.  (This is ready for review, however I haven't requested review on the required "part 1" yet, so I'm making this a feedback request for now)

Comment 7

[Mike de Boer [:mikedeboer]]

Comment on attachment 8504432 [details] [diff] [review]
0001-Bug-984172-part-1-add-Assert.rejects.-r-mikedeboer.patch

Review of attachment 8504432 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for working on this, Mark, and for the a-ok test coverage.

Looking forward to the final patch!

::: testing/modules/Assert.jsm
@@ +462,5 @@
> +  if (typeof expected === "string") {
> +    message = expected;
> +    expected = null;
> +  }
> +  let deferred = Promise.defer();

I was recommended earlier to use the newer `return new Promise(onResolve, onReject);` pattern instead of defers by Paolo. You probably want to do that here too.

Comment 8

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-984172-part-1-add-Assert.rejects.-r-mikedeboer.patch

Thanks!  This version uses new Promise()

Comment 9

[Mike de Boer [:mikedeboer]]

Comment on attachment 8505845 [details] [diff] [review]
0001-Bug-984172-part-1-add-Assert.rejects.-r-mikedeboer.patch

Review of attachment 8505845 [details] [diff] [review]:
-----------------------------------------------------------------

Great! Thanks for working on this.

::: testing/modules/tests/xpcshell/test_assert.js
@@ +337,5 @@
> +  yield checkRejectsFails(new Error("something else"), /oh no/);
> +
> +  // Check simple string messages.
> +  yield assert.rejects(Promise.reject("oh no"), /oh no/, "rejected");
> +  // wrong message

nit: 'Wrong message.'

Comment 10

[Mark Hammond [:markh] [:mhammond]]

Thanks!  Pushed with nit addressed.

https://hg.mozilla.org/integration/fx-team/rev/7f47fcab5b5c
https://hg.mozilla.org/integration/fx-team/rev/b2a0cc22e9a6

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7f47fcab5b5c
https://hg.mozilla.org/mozilla-central/rev/b2a0cc22e9a6

Comment 12

[Marco Mucci [:MarcoM]]

Hi Mark, can you mark this bug for QE verification in the whiteboard.