Closed Bug 984172 Opened 9 years ago Closed 8 years ago

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


(Testing :: General, defect)

Not set


(Not tracked)



(Reporter: markh, Assigned: markh)


(Keywords: dev-doc-needed, Whiteboard: [qa-])


(2 files, 2 obsolete files)

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 - (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
Mark, are you interested in taking this? I think it'd be a *very* valuable addition to Assert.jsm!
Flags: needinfo?(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 :)
Flags: needinfo?(mhammond) → firefox-backlog?
Whiteboard: p=2
Flags: firefox-backlog? → firefox-backlog+
Attached patch WIP (obsolete) — Splinter Review
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?
Flags: needinfo?(mdeboer)
Mark, this is really strange.

So I found the formal spec-y reason of this failure at

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.
Flags: needinfo?(mdeboer)
Clearing out some of my old patch queue.  This seems to do what we want - see "part 2" for actual usage.
Attachment #8440458 - Attachment is obsolete: true
Attachment #8504432 - Flags: feedback?(mdeboer)
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)
Attachment #8504434 - Flags: feedback?(ttaubert)
Comment on attachment 8504432 [details] [diff] [review]

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.
Attachment #8504432 - Flags: feedback?(mdeboer) → feedback+
Attachment #8504434 - Flags: feedback?(ttaubert) → feedback+
Thanks!  This version uses new Promise()
Assignee: nobody → mhammond
Attachment #8504432 - Attachment is obsolete: true
Attachment #8505845 - Flags: review?(mdeboer)
Attachment #8504434 - Flags: review?(ttaubert)
Iteration: --- → 36.1
Points: --- → 2
Whiteboard: p=2 → [qa?]
Attachment #8504434 - Flags: review?(ttaubert) → review+
Comment on attachment 8505845 [details] [diff] [review]

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.'
Attachment #8505845 - Flags: review?(mdeboer) → review+
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Hi Mark, can you mark this bug for QE verification in the whiteboard.
Flags: needinfo?(mhammond)
Flags: needinfo?(mhammond)
Whiteboard: [qa?] → [qa-]
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.