Closed
Bug 984172
Opened 10 years ago
Closed 10 years ago
Add an Assert.jsm helper to verify a promise is rejected
Categories
(Testing :: General, defect)
Testing
General
Tracking
(Not tracked)
People
(Reporter: markh, Assigned: markh)
Details
(Keywords: dev-doc-needed, Whiteboard: [qa-])
Attachments
(2 files, 2 obsolete files)
10.87 KB,
patch
|
ttaubert
:
review+
ttaubert
:
feedback+
|
Details | Diff | Splinter Review |
4.33 KB,
patch
|
mikedeboer
:
review+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
Mark, are you interested in taking this? I think it'd be a *very* valuable addition to Assert.jsm!
Updated•10 years ago
|
Flags: needinfo?(mhammond)
Assignee | ||
Comment 2•10 years ago
|
||
(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
Updated•10 years ago
|
Flags: firefox-backlog? → firefox-backlog+
Assignee | ||
Comment 3•10 years ago
|
||
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)
Comment 4•10 years ago
|
||
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)
Assignee | ||
Comment 5•10 years ago
|
||
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)
Assignee | ||
Comment 6•10 years ago
|
||
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 7•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8504434 -
Flags: feedback?(ttaubert) → feedback+
Assignee | ||
Comment 8•10 years ago
|
||
Thanks! This version uses new Promise()
Assignee: nobody → mhammond
Attachment #8504432 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8505845 -
Flags: review?(mdeboer)
Assignee | ||
Updated•10 years ago
|
Attachment #8504434 -
Flags: review?(ttaubert)
Updated•10 years ago
|
Iteration: --- → 36.1
Points: --- → 2
Whiteboard: p=2 → [qa?]
Updated•10 years ago
|
Attachment #8504434 -
Flags: review?(ttaubert) → review+
Comment 9•10 years ago
|
||
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+
Assignee | ||
Comment 10•10 years ago
|
||
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•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7f47fcab5b5c https://hg.mozilla.org/mozilla-central/rev/b2a0cc22e9a6
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment 12•10 years ago
|
||
Hi Mark, can you mark this bug for QE verification in the whiteboard.
Flags: needinfo?(mhammond)
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(mhammond)
Whiteboard: [qa?] → [qa-]
Updated•9 years ago
|
Keywords: dev-doc-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•