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

findBy* retry-ability when multiple elements found #83

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions cypress/fixtures/test-app/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,13 @@ <h2>*AllByText</h2>
<button onclick="this.innerText = 'Jackie Kicked'">Jackie Chan 1</button>
<button onclick="this.innerText = 'Jackie Kicked'">Jackie Chan 2</button>
</section>
<section>
<h2>Remove element from DOM delayed</h2>
<ul>
<li>List Item</li>
<li>List Item <button onclick="li = this.parentElement; setTimeout(function() { li.remove(); }, 100)">Remove Parent</button></li>
</ul>
</section>
<section>
<h2>*ByText on another page</h2>
<a onclick='setTimeout(function() { window.location = "/next-page.html"; }, 100);'>Next Page</a>
Expand Down
9 changes: 7 additions & 2 deletions cypress/integration/find.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,11 @@ describe('find* dom-testing-library commands', () => {
cy.findByText('Non-existing Button Text', {timeout: 100}).should('not.exist')
})

it('findByText retry if multiple elements found', () => {
cy.findByText('Remove Parent').click()
cy.findByText('List Item', {timeout: 200}).should('exist')
})

it('findByText within', () => {
cy.get('#nested').within(() => {
cy.findByText('Button Text 2').click()
Expand Down Expand Up @@ -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.`
Copy link
Contributor

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

cy.on('fail', err => {
expect(err.message).to.eq(errorMessage)
})

cy.findByText(/^Button Text/i)
cy.findByText(/^Button Text/i, { timeout: 100 })
})
})

Expand Down
28 changes: 16 additions & 12 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,21 @@ function createCommand(queryName, implementationName) {
.window({log: false})
.then((thenArgs) => {
const getValue = () => {
const value = commandImpl(thenArgs.document);
const result = Cypress.$(value);
let result;
try {
const value = commandImpl(thenArgs.document);
result = Cypress.$(value);
}
catch (err) {
// 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;
Copy link
Contributor

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?

result = Cypress.$();
} else {
throw err; // Throw everything else
}
}

// Overriding the selector of the jquery object because it's displayed in the long message of .should('exist') failure message
// Hopefully it makes it clearer, because I find the normal response of "Expected to find element '', but never found it" confusing
Expand All @@ -103,16 +116,7 @@ function createCommand(queryName, implementationName) {
return getValue();
}

return resolveValue()
.then(subject => {

// Remove the error that occurred because it is irrelevant now
if (consoleProps.error) {
delete consoleProps.error;
}

return subject;
})
return resolveValue();
})
},
}
Expand Down