-
Notifications
You must be signed in to change notification settings - Fork 466
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: Enhance ByText
error with selector
#1159
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 7054c06:
|
Codecov Report
@@ Coverage Diff @@
## main #1159 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 24 24
Lines 996 998 +2
Branches 327 326 -1
=========================================
+ Hits 996 998 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. You've checked that docs were added but I don't see a PR to the docs page (https://github.com/testing-library/testing-library-docs/pulls?q=is%3Apr+is%3Aopen+sort%3Aupdated-desc+author%3Araplemie)?
I checked the docs checkmark because it was "handled" as N/A, not added. The error messages are not documented in the docs, so it didnt seem to require a docs change, but since the PR been sleeping for a month I thought that maybe this was automated that it had to be checked for it to process any further. I unchecked the box, but is there a section where error messages are documented so I can follow the style ? Or do we keep the documentation as is if there is no precedent in documenting the error messages text that are returning by a query ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I only glanced at this, saw documentation checked and thought it needed docs but docs were forgotten.
But this doesn't add selector
to ByText
. It just adds it to the error message. Thank you for clarifying 👍🏻
selector
ByText
error with selector
@all-contributors add @raplemie for code |
I've put up a pull request to add @raplemie! 🎉 |
🎉 This PR is included in version 8.19.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Thanks for merging! |
What: fixes #1158
Why: The selector option behavior might not be well known by everyone reading the tests, and current test error output does not mention that additional requirement of the element being searched (which might be shown in the prettyDOM, similar reasonning to #450)
How: By adding a conditional to the error message genereated which have access to the used selector, and display it in the case it is not
undefined
(function called directly) and different from"*"
(default selector forgetByText
)Checklist:
docs site N/A