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

Feature: Add support for .rejects.toThrowWithMessage #315

Merged
merged 2 commits into from
Oct 11, 2021
Merged

Feature: Add support for .rejects.toThrowWithMessage #315

merged 2 commits into from
Oct 11, 2021

Conversation

GerkinDev
Copy link
Contributor

@GerkinDev GerkinDev commented Apr 27, 2021

What

This feature adds suport for expect(promise).rejects.toThrowWithMessage, which previously throwed because passed tested value was not a function. This test is now conditionnaly skipped if the matcher is called from rejects.

Why

Closes #219

Notes

This change required an update to jest@^24.0.0 to use features mentioned in the corresponding changelog, and in particular this PR.

This update of jest required an update of babel-jest@^24.0.0, which is incopatible with babel-core@^6.26.0. Thus, babel-core & babel-cli were removed and replaced with @babel/core@^7.0.0 & @babel/cli@^7.0.0, and use of @babel/preset-env@^7.0.0. All changes in babel/build-related deps are a direct result of those chained required updates.

This PR is directly installable as a dependency in projects with:

npm install --save-dev git+https://github.com/GerkinDev/jest-extended.git#build/rejects-toThrowWithMessage

Housekeeping

  • Unit tests
  • Documentation is up to date
  • No additional lint warnings
  • Typescript definitions are added/updated where relevant

@imjordanxd
Copy link

@GerkinDev any updates on this? I can see you have conflicts.

@GerkinDev
Copy link
Contributor Author

Looks like the original maintainers did some changes 2 weeks ago in 4849c57 creating the conflict.

Problem is, this PR has a counterpart PR https://github.com/keeganwitt/jest-extended/pull/57 in a forked repo, and I don't know which will be updated or not. Both repositories have recent changes by @keeganwitt and are not in sync. Thus, I would have to create 2 separate PRs with 2 separate git history to match each repo, and I won't do that.

@keeganwitt can you please do something ? Rebase one on the other, or do a merge of some sort, or tell me which repo is the most likely to integrate this PR ? Conflicts are purely on version changes, mine having updated babel@>=7.0.0 while jest-community/jest-extended@master is still on babel@6.26.0. But this PR also uses jest@>=24.0.0 (on master since 4849c57).
So yeah please tell me what I should do, thanks 🍻

@keeganwitt
Copy link
Collaborator

I made a fork and started merging MRs into it because I wasn't sure the status of this repo and whether it was still maintained and would get stuff merged. Now that I've found it's in fact not abandoned, my fork should be unnecessary.

I'll close the MRs in my fork, to help avoid the confusion (and once my MRs here are merged, delete my fork to clean up those references).

@GerkinDev
Copy link
Contributor Author

@keeganwitt do you know who is in charge of this repo and active then ? I'll resolve conflict asap once I know it wont have time to have other conflicts again. Time is precious

@keeganwitt
Copy link
Collaborator

@keeganwitt do you know who is in charge of this repo and active then?

I don't think specific people have said "I'll be handling this work" at this time, and I don't want to speak for others. But there are people with access (I was originally concerned no one besides Matt had access). Sorry that I don't have a more specific answer.

@GerkinDev
Copy link
Contributor Author

Okay then, so I'm waiting here with my hand raised hoping someone will ping me soon

@rickhanlonii
Copy link
Contributor

@GerkinDev just to clarify, this project is in jest-community, so all of the Jest contributors help maintain it.

README.md Outdated
- [.toBeAfter(date)](#tobeafterdate)
- [.toBeBefore(date)](#tobebeforedate)
- Further proposals in [#117](https://github.com/jest-community/jest-extended/issues/117) PRs welcome
- [~~Date~~](#date)
Copy link
Contributor

Choose a reason for hiding this comment

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

This formatting doesn't appear to be correct

Screen Shot 2021-06-28 at 11 08 42 AM

Copy link
Contributor Author

@GerkinDev GerkinDev Jun 28, 2021

Choose a reason for hiding this comment

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

If I remember correctly, this was auto generated by tooling, not by hand. I'm not sure what I should do to fix formatting.

README.md Outdated
@@ -435,7 +437,7 @@ test('passes when value is a function', () => {
Use `.toThrowWithMessage` when checking if a callback function throws an error with a given error type and given error message. Message can either be a `String` or a `RegExp`.

```js
test('throws an error of type TypeError with message "hello world"', () => {
test('throws an error of type TypeError with message "hello world"', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make this a separate test instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied in a05d5ce

package.json Outdated
@@ -35,23 +35,22 @@
"license": "MIT",
"repository": "jest-community/jest-extended",
"devDependencies": {
"@babel/cli": "^7.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm hesitant to change the babel setup in this PR because it's unrelated. Can you revert and handle separately? @keeganwitt has been working on the babel upgrade in a separate stream of work.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's #327 for reference, which is blocked pending #328.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentionned in the PR description, this bump is a direct dependency of the update of babel-jest@>=24.0.0, required because jest@>=24.0.0 exposes context properties allowing specific behavior depending on the async context. I doubt this PR could be done reliabily and being compatible with jest@^23.0.0 & jest@^24.0.0.

package.json Outdated
"eslint": "^4.9.0",
"eslint-plugin-import": "^2.8.0",
"eslint-plugin-jest": "^21.2.0",
"husky": "^0.14.3",
"import-all.macro": "^2.0.3",
"jest": "^23.0.1",
"jest": "^24.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a breaking change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's a dev dependency, and master is already bumped to 24.

@keeganwitt
Copy link
Collaborator

keeganwitt commented Jun 28, 2021

@GerkinDev just to clarify, this project is in jest-community, so all of the Jest contributors help maintain it.

Yes, sorry if that was unclear in my response. No specific person(s) has taken primary responsibility, but it continues to be collectively maintained by the team. There just isn't any specific people to @.

@GerkinDev
Copy link
Contributor Author

Up ?

@corbetchri
Copy link

Hey team - any idea when this will go through? Would love to use this change. Thank you

@GerkinDev
Copy link
Contributor Author

Up. More and more conflicts....

@GerkinDev
Copy link
Contributor Author

Rebased on latest main.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

thanks!

could you remove the changes in package.json and the lockfile? They should be needed (and are updated in main) 🙂

@GerkinDev
Copy link
Contributor Author

@SimenB here you are ! Rebased and pruned from useless chore !

@codecov
Copy link

codecov bot commented Oct 11, 2021

Codecov Report

Merging #315 (2497be4) into main (ee70268) will increase coverage by 100.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           main      #315        +/-   ##
===========================================
+ Coverage      0   100.00%   +100.00%     
===========================================
  Files         0       117       +117     
  Lines         0       681       +681     
  Branches      0       108       +108     
===========================================
+ Hits          0       681       +681     
Impacted Files Coverage Δ
src/matchers/toThrowWithMessage/index.js 100.00% <100.00%> (ø)
src/matchers/toBeFalse/predicate.js 100.00% <0.00%> (ø)
src/matchers/toContainAllValues/index.js 100.00% <0.00%> (ø)
src/matchers/toHaveBeenCalledAfter/predicate.js 100.00% <0.00%> (ø)
src/matchers/toBeHexadecimal/index.js 100.00% <0.00%> (ø)
src/matchers/toBeDate/predicate.js 100.00% <0.00%> (ø)
src/matchers/toBeFunction/index.js 100.00% <0.00%> (ø)
src/matchers/toBeEmpty/predicate.js 100.00% <0.00%> (ø)
src/matchers/toBeNaN/predicate.js 100.00% <0.00%> (ø)
src/matchers/toContainAnyKeys/predicate.js 100.00% <0.00%> (ø)
... and 108 more

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 ee70268...2497be4. Read the comment docs.

@SimenB SimenB merged commit cbdbe11 into jest-community:main Oct 11, 2021
@SimenB
Copy link
Member

SimenB commented Oct 11, 2021

Thank you, and thanks for your patience here!

@GerkinDev GerkinDev deleted the feature/rejects-toThrowWithMessage branch October 11, 2021 14:11
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.

toThrowWithMessage does not work with promise
6 participants