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

queryBy* queryAllBy* #5

Merged
merged 3 commits into from
May 5, 2018
Merged

Conversation

lgandecki
Copy link
Collaborator

@lgandecki lgandecki commented May 5, 2018

What:

Allows things like:

it('queryByText', () => {
  cy.queryByText('Button Text').should('exist')
  cy.queryByText('Non-existing Button Text').should('not.exist')
})

Why:

It's a lack of a feature comparing to the DOM-testing-library, and it makes it harder to seamlessly switch between react/cypress without being able to check for elements NONexistence.

How:

A bit of a hack, but I think it's better to show a message like that:

screen shot 2018-05-05 at 13 49 51

(expected null to not exist) , then not allow for checking for nonexistence whatsoever. Especially since it's just below a log saying "querybysomething value", so it's very easy to understand what's going on.

If on the other side, the thing we are querying for exists, everything works 100% as expected, with a proper error message.

I've also added a default timeout, as without it cypress by default shows ugly error messages (about function timing out, instead of the dom-testing-library errors). It was useful for debugging, but if you prefer not to have this change, I'm obviously happy to remove it

Checklist:

Not filling those yet, as, again, I would like to hear your thoughts first.

  • Documentation
  • Tests
  • Ready to be merged
  • Added myself to contributors table

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

I'm in favor 👍 let's get the docs updated too.

const baseCommandImpl = doc =>
waitForElement(() => queryImpl(doc, ...args), {
container: doc,
timeout: 3000,
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not related directly to the subject of this PR, but as you're adding hardcoded timeout: 3000 I would suggest adding a way to pass these defaults to the command, e.g. use the argument that follows query arguments, something like args[queryImpl.length] (hacky generic way, queryImpl arity plus one) as an options object of structure like { timeout: 3000, waitForElementOptions: { ... } } (timeout as a top-level option because it may be used in multiple lower-level calls, not just in waitForElement call).

P.S. Cypress default timeout is 4000, not 3000.

Copy link
Member

Choose a reason for hiding this comment

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

If we do the same timeout as Cypress, we run the risk of Cypress timing out before us. Could we make the Cypress timeout 5000ms for this command to deal with that issue?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I did this in getVisuallyBelow command (maintained in proprietary closed source for now, easier to test there):

    chain = chain
      .then({
        timeout: waitForElementOptionsWithDefaults.timeout + 100,
        log: false,
      }, thenHandler);

Copy link
Member

Choose a reason for hiding this comment

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

Looks perfect

Copy link
Collaborator

Choose a reason for hiding this comment

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

The timeout option to cy builtins overrides the default Cypress timeout. I was saying that we should default to the same number in our options.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cypress.Commands.add('getVisuallyBelow', { prevSubject: 'element' }, (
  subject,
  matcher,
  {
    log,
    timeout = 4000,
    selector = '*',
    verticalDistThreshold = 0,
    horizontalDistThreshold = -12,
    waitForElementOptions = {},
  } = {}
) => {

@lgandecki
Copy link
Collaborator Author

Ok. I've updated the docs, as for tests - would you want to have all the query* ones there as well to be extra safe? It's hard to imagine when some of them would work and others wouldn't, but happy to do so for consistency.

As for the timeout - those are good points. I think I'd rather have the cypress timeout set for 5000 ms for consistency with jest, but how about merging this in first, and then maybe @sompylasar you would like to take a stab at that, seems like you have some good ideas :-)

@kentcdodds kentcdodds merged commit 04c5850 into testing-library:master May 5, 2018
@kentcdodds
Copy link
Member

I figured it would be easier to merge this now and fix things than delay. Let's get the timeout updates done soon. Also it'd be cool to expose our command registration utility as well as you mentioned elsewhere @lgandecki 👍

@kentcdodds
Copy link
Member

Thanks so much for your help! I've added you as a collaborator on the project. Please make sure that you review the other/MAINTAINING.md and CONTRIBUTING.md files (specifically the bit about the commit messages and the git hooks) and familiarize yourself with the code of conduct (we're using the contributor covenant). You might also want to watch the repo to be notified when someone files an issue/PR. Please continue to make PRs as you feel the need (you can make your branches directly on the repo rather than your fork if you want). Thanks! And welcome to the team :)

@kentcdodds
Copy link
Member

🎉 This PR is included in version 1.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@lgandecki
Copy link
Collaborator Author

Awesome! Thank you :-) This was super fast - you took away my excuse to slack a bit this rewrite I'm doing.. ;)

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

Successfully merging this pull request may close these issues.

3 participants