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

Matches toBe(null) and toEqual(null) in addition to toBeNull() in prefer-document #153

Merged

Conversation

julienw
Copy link
Contributor

@julienw julienw commented Mar 23, 2021

Fixes #152

Checklist:

  • Documentation
  • Tests
  • Ready to be merged

function usesToHaveLengthZero(matcherNode, matcherArguments) {
return matcherNode.name === "toHaveLength" && matcherArguments[0].value === 0;
}

export const create = (context) => {
const alternativeMatchers = /(toHaveLength|toBeDefined|toBeNull)/;
const alternativeMatchers = /^(toHaveLength|toBeDefined|toBeNull|toBe|toEqual)$/;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without the added ^ and $, the toBe part matches toBeInTheDocument 😱

@julienw
Copy link
Contributor Author

julienw commented Mar 23, 2021

(looking at the failures right now)

@benmonro
Copy link
Member

@allcontributors please add @julienw for code and tests

@allcontributors
Copy link
Contributor

@benmonro

I've put up a pull request to add @julienw! 🎉

@julienw
Copy link
Contributor Author

julienw commented Mar 23, 2021

[lint]   61:3  error  Function 'check' has a complexity of 15. Maximum allowed is 14  complexity

The part I added is so small 😓
Is it worth trying to fix this?

@benmonro
Copy link
Member

@julienw if you can easily fix it that would be great, if not you can eslint disable for now, and please add an issue to resolve it.

many thanks

@julienw
Copy link
Contributor Author

julienw commented Mar 23, 2021

BTW I can't run eslint locally, I get this error:

Error: package.json » ./node_modules/kcd-scripts/eslint.js » /home/julien/travail/git/eslint-plugin-jest-dom/node_modules/kcd-scripts/node_modules/eslint-config-kentcdodds/jest.js:
	Environment key "jest/globals" is unknown

Not sure why :-)

@benmonro
Copy link
Member

ugh, kcd scripts has been a bit problematic for me... one of these days i'll remove it.

@julienw julienw force-pushed the 152-add-more-fixes-to-prefer-document branch from 9f2c4ca to af5de1b Compare March 23, 2021 16:22
@julienw julienw force-pushed the 152-add-more-fixes-to-prefer-document branch from af5de1b to 10299cf Compare March 23, 2021 16:38
@codecov
Copy link

codecov bot commented Mar 23, 2021

Codecov Report

Merging #153 (1a4253b) into master (cdbc37d) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 1a4253b differs from pull request most recent head 10299cf. Consider uploading reports for the commit 10299cf to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##            master      #153   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           15        15           
  Lines          377       381    +4     
  Branches        66        69    +3     
=========================================
+ Hits           377       381    +4     
Impacted Files Coverage Δ
src/rules/prefer-in-document.js 100.00% <100.00%> (ø)

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 cdbc37d...10299cf. Read the comment docs.

@@ -3,6 +3,8 @@
* @author Anton Niklasson
*/

/*eslint complexity: ["error", {"max": 20}]*/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the easiest way to fix the eslint issue. I believe the problem is in other parts of the code than the one I added.

Also 20 is the default in eslint, it looks like some other other preset this project is using set it to 15, so I think this is reasonable to put it back to 20 here.

Tell me what you think!

@julienw
Copy link
Contributor Author

julienw commented Mar 23, 2021

This is now all green :-)

@benmonro benmonro merged commit a6600d3 into testing-library:master Mar 23, 2021
@benmonro
Copy link
Member

Thanks again @julienw BTW, keep an eye out on the smoke test job, it's a pretty sweet tool that runs this plugin against a ton of projects as a way of preemptively detecting possible bugs in the lint code. https://github.com/testing-library/eslint-plugin-jest-dom/actions/workflows/smoke-test.yml. now that your code is in there, if there's any potential use cases that might cause an unhandled exception, we can fix it before it gets reported. :)

@julienw
Copy link
Contributor Author

julienw commented Mar 23, 2021

👍
I'm sure you'll ping me anyway ;-)

BTW it's likely we get new (fixable) failures in existing projects, so this will be in a minor or major release, I guess?

@github-actions
Copy link

🎉 This PR is included in version 3.7.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

match toBe(null) and possibly toEqual(null) in addition to toBeNull
2 participants