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

Confusing behaviour when chaining .get() #23008

Closed
alyssaruth opened this issue Jul 29, 2022 · 3 comments
Closed

Confusing behaviour when chaining .get() #23008

alyssaruth opened this issue Jul 29, 2022 · 3 comments
Labels
E2E Issue related to end-to-end testing type: enhancement Requested enhancement of existing feature

Comments

@alyssaruth
Copy link

alyssaruth commented Jul 29, 2022

Current behavior

Currently, it is possible to write code like the following:

cy.get('form').get('input')

What it looks like this code will do is to first find the form component (by searching the global DOM), and then search for an input component within whatever it finds. However, the code is actually equivalent to cy.get('input'), because get always searches from the global document regardless of how it is chained. The correct thing to write, if the nested search is what's desired, is cy.get('form').find('input').

To me, this is an unnecessary and easy-to-fall-into trap (I fell victim to it yesterday and was debugging for ~30 minutes before I realised my mistake). The behaviour is documented, but I think folks can be forgiven for not going immediately to the docs for a method with as benign a name as get.

Desired behavior

I think there should be something within Cypress to warn a user that chaining get like this is likely to be a mistake. I can think of several possible solutions:

  • Make .get() throw an error if it is chained off a previous selector (i.e. anything other than cy.get()). I'm not sure if this is possible, but find throws an error if it's used at the top-level (meaning cy.find() is impossible to write by mistake, which is good!). So I'm hoping the reverse is possible for get.
  • Make it only possible to use .get() from the top-level via the type-system. Again, I'm not sure if that's possible in a world where everything seems to be a Chainable<X>, but if it were it would be a nice solution.
  • Rename .get() to something more descriptive - perhaps .findGlobal() ? The main reason why I think this is a problem (and something you've had to document in the first place) is because there is no obvious semantic difference between the words find and get which would imply this difference in behaviour. I'm more likely to scratch my head if I see cy.findGlobal('form').findGlobal('input') - it would be much less surprising for the result to be that all inputs are being returned from the DOM.

If you go with the third option, you could first introduce findGlobal as an alias, deprecating get before eventually removing it. And with the alias available, users would be free to use a tool like ESLint to ban .get in their repositories.

Test code to reproduce

cy.,get('X').get('Y)

Cypress Version

10.3.0

Other

No response

@mjhenkes mjhenkes self-assigned this Jul 29, 2022
@cypress-bot cypress-bot bot added the stage: investigating Someone from Cypress is looking into this label Jul 29, 2022
@mjhenkes mjhenkes added type: enhancement Requested enhancement of existing feature E2E-core labels Jul 29, 2022
@cypress-bot cypress-bot bot added stage: routed to e2e-core and removed stage: investigating Someone from Cypress is looking into this stage: routed to e2e-core labels Jul 29, 2022
@mschile mschile added triage and removed triage labels Aug 18, 2022
@mjhenkes mjhenkes removed their assignment Oct 5, 2022
@nagash77 nagash77 added E2E Issue related to end-to-end testing and removed E2E-core labels Nov 8, 2022
@jennifer-shehane
Copy link
Member

We considered this in this issue. #508 but ultimately decided against implementing it. It’s not a bad idea, it’s value is just arguably not worth the amount of effort it will take to build it and the amount of pain users would have upgrading. It’d break a lot of people.

@alyssaruth
Copy link
Author

it’s value is just arguably not worth the amount of effort it will take to build it and the amount of pain users would have upgrading. It’d break a lot of people.

I think that's a fair assessment of some of the approaches, but one thing that might still be worth considering (IMO) would be my third suggestion - introduce a findGlobal alias and add a deprecation tag on cy.get(). This is a small amount of work and doesn't break anyone, until you ultimately remove the deprecated method (which could be on a timeline of your choosing, so many major versions away).

@alyssaruth
Copy link
Author

In case it helps anyone else hitting this particular paper cut, this is an ESLint rule we've added that goes some way towards mitigating:

{
    "rules": {
        "no-restricted-syntax": [
            "error",
            {
                "selector": "CallExpression[callee.property.name='get'][callee.object.type='CallExpression'][callee.object.callee.object.name='cy']",
                "message": "Do not chain .get off other cypress commands. Use .find if you need nesting, otherwise start a fresh chain with cy.get()
            },
        ]
    }
}

This will, for example, flag this as bad - which is what we were after:

cy.get('.parent').get('.child')

It'll only catch it at that level of nesting, though, so you could still evade it if you were deeper in a chain with something like the below. But it's better than nothing, and in our world I think the above case was the most likely one to trip people up.

cy.get('.parent').find('.child'').get('.grandchild')

casewalker added a commit to casewalker/files-site that referenced this issue Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E2E Issue related to end-to-end testing type: enhancement Requested enhancement of existing feature
Projects
None yet
Development

No branches or pull requests

5 participants