Closed Bug 984172 Opened 6 years ago Closed 5 years ago

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

Categories

(Testing :: General, defect)

defect
Not set
Points:
2

Tracking

(Not tracked)

RESOLVED FIXED
mozilla36
Iteration:
36.1

People

(Reporter: markh, Assigned: markh)

Details

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

Attachments

(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 - 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
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 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.
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]
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.
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
Status: NEW → ASSIGNED
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]
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.'
Attachment #8505845 - Flags: review?(mdeboer) → review+
https://hg.mozilla.org/mozilla-central/rev/7f47fcab5b5c
https://hg.mozilla.org/mozilla-central/rev/b2a0cc22e9a6
Status: ASSIGNED → RESOLVED
Closed: 5 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.