Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a toThrowWithMessage matcher #170

Merged
merged 11 commits into from
Sep 24, 2018
Merged

Conversation

natealcedo
Copy link
Contributor

What

This pull request adds in a new matcher for asserting that a callback function throws an error of a given type and given error message.

Why

This feature has been requested in core but after some discussion, it was decided that implementing it here would be the better solution.

This PR fixes the following issues

  1. make throwingMatcher test n arguments (currently zero or one) jestjs/jest#7000
  2. .toThrow() to match both error constructor and message? jestjs/jest#3659
  3. toThrow(constructor, message) #157

Notes

I've gone down the road of testing the matcher both by extending expect with this matcher and by invoking the matcher itself. Since this repository doesn't have some of the util tools that the Jest repo has, namely runJest, I am not able to test the failing scenarios since I can't capture whatever is logged to stdErr to assert that the error message is indeed correct. As such, I've just invoked the matcher itself and asserted whether it passes or not and used snapshot testing for asserting error messages.

Also, the goal for me in this matcher was to stick as close to the default toThrowErrormatcher. As such, this matcher also accepts both a string and regex for matching the error message.

The code is abit verbose right now but I'm more than happy to have a discussion on how to amend it moving forward.

Housekeeping

  • Unit tests
  • Documentation is up to date
  • No additional lint warnings
  • Add yourself to contributors list (yarn contributor)
  • Typescript definitions are added/updated where relevant

@codecov-io
Copy link

codecov-io commented Sep 22, 2018

Codecov Report

Merging #170 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #170   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files         103    105    +2     
  Lines         505    536   +31     
  Branches       86     95    +9     
=====================================
+ Hits          505    536   +31
Impacted Files Coverage Δ
src/matchers/toThrowWithMessage/predicate.js 100% <100%> (ø)
src/matchers/toThrowWithMessage/index.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 53a2c60...aa807a6. Read the comment docs.

README.md Outdated
@@ -398,6 +399,30 @@ test('passes when value is a function', () => {
});
```

#### .toThrowWithMessage(type, message)

Use `.toThrowWithMessage` when checking if a callback function throws an error with a given error type and given error message.
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps it's worth mentioning that the message can be either a String or RegExp?

Copy link
Contributor Author

@natealcedo natealcedo Sep 24, 2018

Choose a reason for hiding this comment

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

This sounds like a great idea! I've done the update.

if (!callback || typeof callback !== 'function') {
return {
pass: false,
message: () => `
Copy link
Member

Choose a reason for hiding this comment

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

The spacing of these validation errors is tabbed in too much for the printouts (see the snapshots)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I've pushed in a change for this. Do take a look.

@mattphillips
Copy link
Member

@natealcedo this is looking good just a few small comments :)

@mattphillips
Copy link
Member

Thanks @natealcedo 👍

@mattphillips mattphillips merged commit 5e43791 into jest-community:master Sep 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants