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
Add new E2E helper commands to avoid anti patterns #17005
Add new E2E helper commands to avoid anti patterns #17005
Conversation
Co-authored-by: Cédric Anne <cedric.anne@gmail.com>
I'd argue that rewriting our UI code to add data attributes everywhere specifically as targets for Cypress is exactly what should be used sparingly. If you are filling a form, you should find the relevant field based on the label. If a field doesn't have a label, you should have to think if that was intended or not and if that is really the best UI implementation for a user. If you are getting a button, you should find a Certain elements where there is no static label and no good way to mimic the actual user behavior, like the user menu dropdown, it then makes sense to fall back to using an ID, class, or data attribute. Users learn what that dropdown looks like and where it is positioned. We could in theory add a test to ensure the user menu dropdown is in the top right of the page, but that may not be a good thing to check that every time the tests want to use it. |
I think what the documentation is trying to say is that the tests should focus on the features of the application, rather than their implementation. For exemple, changing a label shouldn't break the tests, same for converting a I think it would be best to follow cypress developers recommandations regarding bad and good practice as they are probably more experienced than us on this part. |
Which recommendation though? Their own documentation contradicts itself. When you look at the Testing Library's selector type priority, they clearly give priority to things the user can actually see and understand.
The thing that no user or accessibility tool can see/hear/etc is the lowest priority. The Cypress recommendations only partially make sense:
Imagine if some PHP testing framework decided to say that calling your functions from test code by name was a bad practice because you may want to rename them later and instead there should be a specific PHP Attribute added to every function with its own label. You interact with the code through the function names, so it makes sense to test by calling those functions by name. You interact with the app's UI through what you see, so it makes sense to test it like that too. |
Thank you for the link to From my understanding, if we always use Taking into account that "content based selector" are the least impactful infraction according to cypress: By using the I suggest that we add the |
Agree. There are actually a few Cypress plugins/custom commands I wanted to look at adding as we build out the tests.
|
Done, the integration with the login page went smoothly, there is only one issue with the Auth login select. I tried to access it with
However, I don't think this solution is good because we are interacting with the HTML We probably need to implement a custom To do this properly, Unless I am misunderstanding something, shouldn't the |
I think something like select2 can be tested through the "interactive" method of clicking the dropdown and then the desired option once and then just use the programmatic way of changing the value through the base select for every other case. I've already added logic for opening a Select2 dropdown (to validate AJAX loading), so maybe it won't be too difficult to add the part for clicking an option too.
Select2 has several known accessibility issues. Their handling, or lack thereof, of labels is one of them. |
Lets see later for select2, should be good for merge now. |
Cypress discourages using brittle selectors (see https://docs.cypress.io/guides/references/best-practices#Selecting-Elements).
The documentation suggest that any selector used by e2e tests should use a dedicated data attribute such as
data-cy
.Their code exemple also suggest setting up helper commands to ease this selector usage, which I've implemented as
getBySelector
andfindBySelector
.I've modified the
login
command to use the new functions as an exemple.After this PR is merged, we can look into updating all the others commands and tests.
I've also noticed that some of our commands were already hard to read because of heavily nested code.
This is due to the nature of cypress, as its promises are not real JS promises and thus can't be streamlined using the
async/await
API.This mean getting multiple
thenables
values create a lot of indentation, for exemple:I've found online a nice solution using a custom
getByMany
command.This allow the previous exemple to be simplified like this:
I've implemented this changes into our override of the
type
command, which become more readable.