Assert: Add `assert.rejects`. #1238

rwjblue · 2017-12-17T23:25:42Z

platinumazure

This looks like a great start! Are there any tests that show assert.rejects causing a test failure if the promise is fulfilled? I only gave this a cursory view as my plane landed, so apologies if I missed something.

rwjblue · 2017-12-18T02:21:00Z

@platinumazure - Thanks for reviewing! You are absolutely right, I was actually somewhat confused about how that worked as I'm used to failing assertions failing CI (which is to be avoided), but a testing framework has to creatively work around that to ensure that its failures are correct too. I did a bit of digging after your comment to see how this was handled for things like assert.throws or assert.ok, and figured out how it works (inverting assert.pushResult's result value).

tldr; I just pushed an update with tests around when assert.rejects fails.

platinumazure · 2017-12-18T03:30:27Z

I've taken another quick look and it looks promising. I'll give it a full review this week. Naturally, I'll also want @trentmwillis to take a look as well. Thanks for your hard work!

rwjblue · 2017-12-18T12:12:33Z

looks promising.

Hehe. Thank you, got a good chuckle out of this pun this morning. 😝

Krinkle · 2017-12-18T16:04:54Z

I assume we don't want to support expected as string because it creates ambiguity with the optional message parameter. I don't have a specific suggestion here, but I do think it warrants better documentation than currently provided.

As-is, the documentation tells me the following:

expected is a comparable for the rejection value.
Function and RegExp type values are special cases, other types are compared directly to actual (such as an Error instance).

However, current code supports object as the only "regular" comparable value type. Any other value (except string) silently falls back to result = false, that's definitely a problem.

I understand string is a problem because of ambiguity with the optional message parameter. I also understand that we (I think?) discourage throwing things that aren't objects. But in that case we shouldn't showcase it in the documentation. The first usage example is currently Promise.rejects("error").

Two minor suggestions, if we indeed want to discourage throwing non-objects:

Remove non-object rejection example from documentation.
Make sure that assert.rejects explicitly fails when expected is of unsupported type (with unit tests to confirm). The missing else case in the current code suggests that at least number and boolean types would result in silent or less-obvious failure.

Krinkle · 2017-12-18T16:05:16Z

docs/assert/rejects.md

+  };
+
+  assert.rejects(
+    Promise.reject("error"),


rwjblue · 2017-12-18T20:46:06Z

Thanks for the review @Krinkle! I just pushed some changes which (assuming I understood your concerns properly) should address your bullet points.

Specifically:

cleaned up the documentation a bit to make it clearer what each expectedMatcher type actually did
refactored the implementation around function's for the expected value so that we can properly be aware of the difference between "invalid expected type" vs "matcher function returned non-true"

Let me know what you think!

trentmwillis

Overall, looks good to me. Just some minor things.

@Krinkle, when you have a moment, can you review again to verify your concerns were addressed? (Looks to me like they were.)

trentmwillis · 2018-01-03T02:39:14Z

docs/assert/rejects.md

+| name               | description                          |
+|--------------------|--------------------------------------|
+| `promise` (thenable) | promise to test for rejection      |
+| `expectedMatcher`  | Rejection value matcher              |


This name is inconsistent with the function signature given. I'd prefer we use expectedMatcher throughout as it makes more sense semantically.

trentmwillis · 2018-01-03T02:41:55Z

docs/assert/rejects.md

+specific set of circumstances, use `assert.rejects()` for testing and
+comparison.
+
+The `expected` argument can be:


expected -> expectedMatcher

trentmwillis · 2018-01-03T02:43:53Z

docs/assert/rejects.md

+```js
+QUnit.test( "rejects", function( assert ) {
+
+  assert.rejects(new Error("some error description"));


This is an invalid example, right?

trentmwillis · 2018-01-03T02:44:30Z

docs/assert/rejects.md

+    "`rejectionValue.toString()` contains `description`"
+  );
+
+	// Using a custom error like object


Nit: inconsistent indentation.

trentmwillis · 2018-01-03T02:46:30Z

src/assert.js

+				message = expected;
+				expected = undefined;
+			} else {
+				message = "rejects does not accept a string value for the expected argument.\n" +


Can we put assert.rejects() does not accept a string value...? I think it is just a bit clearer/more helpful.

trentmwillis · 2018-01-07T01:10:18Z

@rwjblue, no pressure, but do you think you'll update this soon? I'd like to see it in the next release if possible but it can always be in the one after as well 🙂

rwjblue · 2018-01-07T16:34:32Z

@trentmwillis - Thanks for the review! I cleaned up the docs and fixed the error message, I believe that addresses all of the outstanding review comments.

trentmwillis · 2018-01-07T23:12:36Z

Thanks a bunch!

rwjblue force-pushed the add-assert-rejects branch 4 times, most recently from fe1567c to 7568663 Compare December 17, 2017 23:44

rwjblue mentioned this pull request Dec 17, 2017

Add assert method for rejection of promises #1204

Closed

platinumazure reviewed Dec 18, 2017

View reviewed changes

rwjblue force-pushed the add-assert-rejects branch from 7568663 to ac61264 Compare December 18, 2017 02:18

Krinkle requested changes Dec 18, 2017

View reviewed changes

docs/assert/rejects.md Outdated

};

assert.rejects(

Promise.reject("error"),

Copy link

Member

Krinkle Dec 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(*)

rwjblue force-pushed the add-assert-rejects branch from ac61264 to af87708 Compare December 18, 2017 20:42

trentmwillis requested changes Jan 3, 2018

View reviewed changes

rwjblue added 2 commits January 7, 2018 11:31

Assert: Add assert.rejects.

2aeebb3

Docs: Add assert.rejects API documentation.

dc451e6

rwjblue force-pushed the add-assert-rejects branch from af87708 to dc451e6 Compare January 7, 2018 16:33

Krinkle approved these changes Jan 7, 2018

View reviewed changes

trentmwillis approved these changes Jan 7, 2018

View reviewed changes

trentmwillis merged commit 5db78e2 into qunitjs:master Jan 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Assert: Add `assert.rejects`. #1238

Assert: Add `assert.rejects`. #1238

rwjblue commented Dec 17, 2017

platinumazure left a comment

rwjblue commented Dec 18, 2017

platinumazure commented Dec 18, 2017

rwjblue commented Dec 18, 2017

Krinkle commented Dec 18, 2017

Krinkle Dec 18, 2017

rwjblue commented Dec 18, 2017

trentmwillis left a comment

trentmwillis Jan 3, 2018

trentmwillis Jan 3, 2018

trentmwillis Jan 3, 2018

trentmwillis Jan 3, 2018

trentmwillis Jan 3, 2018

trentmwillis commented Jan 7, 2018

rwjblue commented Jan 7, 2018

trentmwillis commented Jan 7, 2018

Assert: Add assert.rejects. #1238

Assert: Add assert.rejects. #1238

Conversation

rwjblue commented Dec 17, 2017

platinumazure left a comment

Choose a reason for hiding this comment

rwjblue commented Dec 18, 2017

platinumazure commented Dec 18, 2017

rwjblue commented Dec 18, 2017

Krinkle commented Dec 18, 2017

Krinkle Dec 18, 2017

Choose a reason for hiding this comment

rwjblue commented Dec 18, 2017

trentmwillis left a comment

Choose a reason for hiding this comment

trentmwillis Jan 3, 2018

Choose a reason for hiding this comment

trentmwillis Jan 3, 2018

Choose a reason for hiding this comment

trentmwillis Jan 3, 2018

Choose a reason for hiding this comment

trentmwillis Jan 3, 2018

Choose a reason for hiding this comment

trentmwillis Jan 3, 2018

Choose a reason for hiding this comment

trentmwillis commented Jan 7, 2018

rwjblue commented Jan 7, 2018

trentmwillis commented Jan 7, 2018

Assert: Add `assert.rejects`. #1238

Assert: Add `assert.rejects`. #1238