-
Notifications
You must be signed in to change notification settings - Fork 153
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
findBy* retry-ability when multiple elements found #83
Conversation
Only relevant for the findBy* queries, due to it throwing an error if multiple elements are found.
This update changed the failure message for matching multiple elements. Unfortunately I can't see away around it if we are using retry-ability.
// Only catch exceptions for multiple elements found, which we will continue to retry | ||
if (/Found multiple elements/.test(err.message)) { | ||
// Cannot delete after, because consoleProps() is called on the log object before I get the chance | ||
consoleProps.CaughtError = err; |
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.
Could you (conditionally) clear the value in the try block?
@@ -118,12 +123,12 @@ describe('find* dom-testing-library commands', () => { | |||
}) | |||
|
|||
it('findByText finding multiple items should error', () => { | |||
const errorMessage = `Found multiple elements with the text: /^Button Text/i\n\n(If this is intentional, then use the \`*AllBy*\` variant of the query (like \`queryAllByText\`, \`getAllByText\`, or \`findAllByText\`)).` | |||
const errorMessage = `Timed out retrying: Expected to find element: 'findByText(/^Button Text/i)', but never found it.` |
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.
Yeah this is kind of confusing... Let's hold while we figure it out
Hi folks 👋 @Jmaharman, do you still have plans to work on this or should we have someone else pick up where you left off? No pressure, just wondering how to proceed :) |
* Add element selector infomation for debugging (outlines element when you click on command) (fixes testing-library#103) * Add @testing-library/dom errors (from `get*` queries) to failure messages - these are more helpful than the generic `find*('input') does not exist` messages (fixes testing-library#103) * Add retryability to `findBy*` when multiple elements are found (fixes testing-library#83) * Add option to disable logging of all commands * `query*` and `find*` have a consistent code path and error messaging (fixes testing-library#103) * Remove usage of Cypress commands in queries (fixes testing-library#103)
* Add element selector information for debugging (outlines element when you click on command) (fixes testing-library#103) * Add @testing-library/dom errors (from `get*` queries) to failure messages - these are more helpful than the generic `find*('input') does not exist` messages (fixes testing-library#103) * Add retryability to `findBy*` when multiple elements are found (fixes testing-library#83) * Add option to disable logging of all commands * `query*` and `find*` have a consistent code path and error messaging (fixes testing-library#103) * Remove usage of Cypress commands in queries (fixes testing-library#103)
Sorry, I don't have the time to jump into it. If someone else wants to have
a run it at then they are more than welcome to.
…On Tue, 28 Jan 2020 at 18:15, Kent C. Dodds ***@***.***> wrote:
Hi folks 👋 @Jmaharman <https://github.com/Jmaharman>, do you still have
plans to work on this or should we have someone else pick up where you left
off? No pressure, just wondering how to proceed :)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#83?email_source=notifications&email_token=AABUSP7ZGNT5PZRXGRRIONTRABY5TA5CNFSM4ITOQS22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKELGYI#issuecomment-579384161>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABUSP2SWAFO2O7NAJAXHKLRABY5TANCNFSM4ITOQS2Q>
.
|
No problem @Jmaharman, thanks anyway! I think this will actually be handled by #108. |
* feat: add more helpful debugging information to queries * Add element selector information for debugging (outlines element when you click on command) (fixes #103) * Add @testing-library/dom errors (from `get*` queries) to failure messages - these are more helpful than the generic `find*('input') does not exist` messages (fixes #103) * Add retryability to `findBy*` when multiple elements are found (fixes #83) * Add option to disable logging of all commands * `query*` and `find*` have a consistent code path and error messaging (fixes #103) * Remove usage of Cypress commands in queries (fixes #103) * feat: add ability for queries to inherit previous subject * Fixes #109 without breaking change caused by #100 * feat: add parent/child log type detection * chore: implement feedback * docs: update readme to complete my thought process * slightly simplify config fn * Update README.md Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com> Closes #103, Closes #109, Closes #110
🎉 This issue has been resolved in version 5.2.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
What:
Add retry-ability to findBy* when multiple elements are matched.
Why:
findBy* queries would fail early if they match multiple elements, when it could automatically retry.
*How:
This catches the multiple matches exception, and retries until the request times out.
Checklist:
[ ] Documentation
[x] Tests
[x] Ready to be merged
Side Effects
Unfortunately this change comes with side effects, in that the error message when this scenario occurs says that the element cannot be found. See the last commit for the new error message being expected in the test.
I can't see a way around that for now, and as such I'm not sure if we should include this change.
I thought I'd commit this PR so that others could see it, and consider another way of doing it.