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

feat: check for any Node in toHaveTextContent #358

Merged
merged 2 commits into from
Apr 22, 2021

Conversation

julienw
Copy link
Contributor

@julienw julienw commented Apr 22, 2021

Fixes #306

I based it off #307 but changed a lot of things, that's why I didn't keep the initial author information.

What, Why: Please have a look at #306
How: I added a checkNode function in utils so that it can be reused elsewhere. I followed the style of the other functions and also the comment in #307 about checking that we have a window before using it.

Checklist:

  • Documentation
  • Tests
  • Updated Type Definitions <-- Not sure where to check them?
  • Ready to be merged

@julienw julienw changed the base branch from check-for-any-node to main April 22, 2021 10:24
@codecov
Copy link

codecov bot commented Apr 22, 2021

Codecov Report

Merging #358 (1a38f75) into main (7d1c742) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #358   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           23        23           
  Lines          545       554    +9     
  Branches       198       199    +1     
=========================================
+ Hits           545       554    +9     
Impacted Files Coverage Δ
src/to-have-text-content.js 100.00% <100.00%> (ø)
src/utils.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 7d1c742...1a38f75. Read the comment docs.

@gnapse gnapse self-requested a review April 22, 2021 11:08
}
}

function checkHasWindow(htmlElement, ErrorClass, ...args) {
Copy link
Member

Choose a reason for hiding this comment

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

I think you could've kept the signature of this one intact as we only call it internally in this module, only once, and always to check for the HTML element check, and not the Node check.

Not a big deal or a blocking change request. Can stay as is. Maybe I'm missing something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See line 63, I actually call it for the node check ;-)

Copy link
Member

Choose a reason for hiding this comment

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

yes, I know.

My point was that that was the only call to it. But never mind, it's ok as is. It was a bit of a nit-pick, and maybe a wrong one at that anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yeah I see what you mean :-) yeah, maybe it's a bit over-designed :-)

@gnapse
Copy link
Member

gnapse commented Apr 22, 2021

Updated Type Definitions <-- Not sure where to check them?

This will need to be handled separately in https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/testing-library__jest-dom

Not meaning to say you should handle it. Maybe someone does it first, if they feel the pain of wanting this new feature and needed TypeScript support. Personally I find it a hassle to not have the types co-located.

README.md Outdated Show resolved Hide resolved
@gnapse gnapse changed the title fix: check for any Node in toHaveTextContent feat: check for any Node in toHaveTextContent Apr 22, 2021
@julienw
Copy link
Contributor Author

julienw commented Apr 22, 2021

This will need to be handled separately in https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/testing-library__jest-dom

I see, looking at this I believe there's no change needed. But I'm not using TS (we're using Flow in our project) so I may miss something.

For Flow, what we put in expect is typed as any so everything passes.

@gnapse gnapse merged commit fa0d91d into testing-library:main Apr 22, 2021
@julienw
Copy link
Contributor Author

julienw commented Apr 22, 2021

Thanks for the quick review and merge!

@gnapse
Copy link
Member

gnapse commented Apr 22, 2021

Thank you too for your contribution!

@gnapse
Copy link
Member

gnapse commented Apr 22, 2021

@all-contributors add @julienw for code, test

@allcontributors
Copy link
Contributor

@gnapse

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

@github-actions
Copy link

🎉 This PR is included in version 5.12.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@nickserv
Copy link
Member

nickserv commented Apr 25, 2021

The type declarations live in @types/testing-library__jest-dom. However as far as I understand, the type of the expected value isn't actually checked by TypeScript, so I don't think you need to update them in this case.

@gnapse
Copy link
Member

gnapse commented May 31, 2021

@RandinAk what's the purpose of your last comment? It seems to be mostly a quote of this PR's description, with no extra info added.

@gnapse
Copy link
Member

gnapse commented Jun 7, 2021

@all-contributors add @julienw for code, test

@allcontributors
Copy link
Contributor

@gnapse

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

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.

toHaveTextContent should support any Node
3 participants