bencodezen / vue-enterprise-boilerplate

An ever-evolving, very opinionated architecture and dev environment for new Vue SPA projects using Vue CLI.
7.78k stars 1.32k forks source link

Cypress component naming best practices #33

Closed jnarowski closed 6 years ago

jnarowski commented 6 years ago

I'd love to contribute to this repo, what do I have to do to become a contributor?

I have a pull request that adjusts some of the cypress tests to align better with their stated best practices. It's something that's worth discussing.

Specifically using the data-cy attribute for selection, instead of relying on DOM selection, ids or css.

chrisvfritz commented 6 years ago

Sorry for the delay in reply. I'd love to have you as a contributor! 🙂 And for what it's worth, I already personally consider you a contributor, as you've definitely made positive contributions to the repo through issues! Many of those discussions have also led directly to code changes.

Ideas for contributions

Improving Cypress tests

I agree that improving the Cypress tests is worth discussing! We may want to open a new issue for this, but to address the one item you mentioned:

Specifically using the data-cy attribute for selection, instead of relying on DOM selection, ids or css.

I think you're referring to selectors like this:

cy.get('[data-testid="login-link"]')
cy.get('[data-testid="input-username"]')

instead of:

cy.contains('a', 'Log in')
cy.get('input[name="username"]')

While Cypress' best practices recommend the data-testid pattern, they do acknowledge that my pattern is also acceptable. 🙂 Specifically, they say it "depends", because the longer story is a bit more complicated - and I realize now, I should document it. 😅

So let's say our app has this requirement:

When logged out, there should be a visible link containing the text "Log in", that when clicked on, will take the user to the login page.

When writing this test, the overarching goal that we're shooting for is that tests only fail when A) something is actually broken or B) the requirements have changed. So selectors like this:

cy.get('a')            // Too general, as there could be many links
cy.get('.login-link')  // Tied to implementation detail of CSS
cy.get('#login-link')  // Tied to implementation detail of JS
cy.contains('Log in')  // Assumes the text only appears in one context

will be problematic, because they all have the potential to break even when the app still works and requirements have not changed. Which brings me to my choice:

cy.contains('a', 'Log in')

This selector describes the feature in a more user-centered way that also serves to drive accessibility. In our requirements, we specify a link. Not just something that looks like a link, but actually is a link using the a element that screen readers and other accessibility software will recognize and look for. That link should also contain specific text, so users of all abilities and contexts can recognize it.

I've actually used the data-testid strategy on most past projects with e2e tests. The problem I've found is that tests become a little too stable. For example, we could replace a with a clickable, styled div and replace Log in with a span that displays an icon. Now the test will still pass, even though the feature no longer meets the requirements:

So I think my strategy actually better serves the end goal. 🙂 There are a few additional notes however:

Let me know if all that make sense. 🙂

jnarowski commented 6 years ago

Nice break down. I totally get where you are coming from.

1) my biggest qualm with integration tests is they break often. There is a fine line between too brittle and too robust.

cy.contains('[role="alert"]', 'validation errors')

What if you have more than one alert? What happens if you need to drill into divs?

I've had too many brittle front end tests that focus on language (that changes) and then tests break. Role could be a good middle ground but what happens when you have a non html5 compliant role?

I still argue that the data-cy="x" gives you full control when dealing with complex e2e tests.

Maybe it's a mixture of your route and the data-cy method and we could show examples of both in the repo?

I see your logic on the login link (making sure it is in fact an "a" tag), but what about when things get more robust in the dom (selecting a row of a list (maybe the second one) in a group of widgets etc.

jnarowski commented 6 years ago

PS: It might be a good idea to add a "how to contribute" section to the wiki or readme.

jnarowski commented 6 years ago

Another thought,

with the current tests: cy.get('input[name="username"]').type('badUsername')

What if you change the input to be named "login" instead of "username". Should that really break your authentication test? My thinking is that the backend logic shouldn't affect the front end, and whether the input is named login or username, it should still pass, as long as the backend is getting what it needs.

Hence the call for something like data-cy='login' that remains unchanged.

The biggest issue I have is being afraid to refactor my app, and thus breaking a TON of e2e tests. What results is less e2e tests in general, which isn't good either.

chrisvfritz commented 6 years ago

What if you have more than one alert?

The beauty of:

cy.contains('[role="alert"]', 'validation errors')

Is that it will only select elements with role="alert" that contain the text "validation errors".

What happens if you need to drill into divs?

Hm, could you provide a specific example?

I've had too many brittle front end tests that focus on language (that changes) and then tests break.

I see the language as part of the requirements, so when the language significantly changes, then so should the spec. The trick is to only select by language that's relevant to those requirements. So in the earlier example, 'validation errors' is used instead of a more complete message like 'There were 2 validation errors.'. This way, if we change There were to There are or remove the period at the end, the test still passes. But, if we change validation errors to submission issues or something like that, that's actually a change in spec, because it's a fundamental change around how we're talking about this thing in our app.

As you define specific terminology for your app, you can even define a list of constants that you share between the source and tests, to enforce consistent language throughout your app. For example:

export const validationError = count => 
  count + ' validation error' + (count === 1 ? '' : 's')
import copy from '@copy'

// ...

cy.contains('[role="alert"]', copy.validationError(2))

Now if you change validation error to submission issue, it won't break any tests and anywhere else in your app where you're using this language will also be updated. 🙂

Role could be a good middle ground but what happens when you have a non html5 compliant role?

The list is pretty extensive, but one advantage of this rule is that if there's not an existing ARIA role or some other way to represent the user interaction accessibly, then you'll know you're building an inherently inaccessible app, which is good to know! 😅

I still argue that the data-cy="x" gives you full control when dealing with complex e2e tests.

They don't give you any more control really, but they are less likely to break when requirements change, which as I've argued, I think is actually a bad thing.

Maybe it's a mixture of your route and the data-cy method and we could show examples of both in the repo?

If you can show me examples of something that can't be achieved with user-centered selectors, I'm open to that. 🙂

I see your logic on the login link (making sure it is in fact an "a" tag), but what about when things get more robust in the dom (selecting a row of a list (maybe the second one) in a group of widgets etc.

The question is always: how would users navigate it? For example, if the user is looking for a widget with the title "Related links", and then underneath that a link titled "Some link title", you might write this:

cy
  .contains('aside[role="listbox"]', 'Related links')
  .contains('li', 'Some link title')

Does that make sense?

PS: It might be a good idea to add a "how to contribute" section to the wiki or readme.

Ha, I agree! Pull request welcome. 🙂

What if you change the input to be named "login" instead of "username". Should that really break your authentication test? My thinking is that the backend logic shouldn't affect the front end, and whether the input is named login or username, it should still pass, as long as the backend is getting what it needs.

The names of fields are important for more than servers though. For example, that change is likely to break features of many assistive technologies, form fillers, and password managers that are looking for names like "username", "user", or "email". So this is part of the requirements.

The biggest issue I have is being afraid to refactor my app, and thus breaking a TON of e2e tests. What results is less e2e tests in general, which isn't good either.

I agree that tests you never write aren't useful, 🙂 but I don't think any of the tests I've provided as examples would break during a refactor. They'd only change if actual requirements changed.

I'd be surprised if you actually tried this strategy and found a lot of pain. Cypress tests are much easier to refactor if you need to, especially since you can run them while you're developing, rather than as an afterthought at the end. This strategy will require a little more time, because it requires a little more thought, but I think that's well worth it because they help promote building accessibly (a requirement for many enterprise apps) and will correctly fail in cases when the data-cy strategy would not (but should).

JayStringer commented 4 years ago

This is a really useful thread, thanks both!

natural411 commented 3 years ago

I tried using [role=""] and cannot get it to find elements. is this not implemented yet? seems like it should work already as it's an attribute on the element.

JayStringer commented 3 years ago

I tried using [role=""] and cannot get it to find elements. is this not implemented yet? seems like it should work already as it's an attribute on the element.

Hey,

You need to specify what the role attribute should match to: cy.contains('[role="breakfast"]', 'waffles')

Will match to an element with a 'role' attribute with the value equal to 'breakfast' that also contains the text 'waffles'.

natural411 commented 3 years ago

Will match to an element with a 'role' attribute with the value equal to 'breakfast' that also contains the text 'waffles'.

thanks for the reply. what about an input box that has no 'text', do we need to put in name or something?

natural411 commented 3 years ago

any idea why

    cy.contains('[role="button"]', 'name')

would work sometimes for buttons and not others? I think i'm missing something.