Add new E2E helper commands to avoid anti patterns

[AdrienClairembault]

Cypress discourages using brittle selectors (see https://docs.cypress.io/guides/references/best-practices#Selecting-Elements).

Targeting the element above by tag, class or id is very volatile and highly subject to change. You may swap out the element, you may refactor CSS and update ID's, or you may add or remove classes that affect the style of the element.

Instead, adding the data-cy attribute to the element gives us a targeted selector that's only used for testing.

The data-cy attribute will not change from CSS style or JS behavioral changes, meaning it's not coupled to the behavior or styling of an element.

Additionally, it makes it clear to everyone that this element is used directly by test code.

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 and findBySelector.

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:

 cy.get(`textarea[name="${subject.attr('name')}"]`).invoke('attr', 'id').then((textarea_id) => {
    cy.window().then((win) => {
        [code]
    }
}

I've found online a nice solution using a custom getByMany command.
This allow the previous exemple to be simplified like this:

cy.window().as('win');
cy
    .get(`textarea[name="${subject.attr('name')}"]`)
    .invoke('attr', 'id')
    .as('textarea_id')
;

cy.getMany(["@win", "@textarea_id"]).then(([win, textarea_id]) => {
    [code]
}

I've implemented this changes into our override of the type command, which become more readable.

Q	A
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#number

[cconard96]

The documentation suggest that any selector used by e2e tests should use a dedicated data attribute such as data-cy.

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.
Text selectors are the best option in my opinion because we are testing user behaviors, and therefore we should mimic what a user does. Users don't see HTML attributes; they see text, shapes and colors.
If we change the UI and the E2E tests fail, then it may mean that there is a behavior change and that is something we should have to acknowledge and change the test if that change was intended.

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 .btn by the text content. If we, for whatever reason, removed .btn that would likely be a UI breaking change. If we change that element from a button to a link but keep the .btn class, then there is no UI change.

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.

[AdrienClairembault]

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 <a> tag into a <button> tag.
As long as the changed label is still visible and that the <button> tag still trigger the same action on click, you shouldn't have to rewrite your tests.

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.

[cconard96]

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.
The link you provided shows their disdain for everything except a data attribute added solely for Cypress. Yet, slightly further down they say:
"Cypress loves the Testing Library project. We use Testing Library internally, and our philosophy aligns closely with Testing Library's ethos and approach to writing tests. We strongly endorse their best practices."

When you look at the Testing Library's selector type priority, they clearly give priority to things the user can actually see and understand.
https://testing-library.com/docs/queries/about/#priority

1. Role - Good because it is used accessibility tools (May not be a good fit for us as a primary selector)
2. Label text - Really good for form fields because that's what the user looks for
3. Placeholder text
4. Text
5. Display value
6. Alt text
7. Title
8. ID - "The user cannot see (or hear) these, so this is only recommended for cases where you can't match by role or text or it doesn't make sense (e.g. the text is dynamic)"

The thing that no user or accessibility tool can see/hear/etc is the lowest priority.

The Cypress recommendations only partially make sense:

1. Using element tags alone [Never] - Makes complete sense
2. Using .btn.btn-large [Never] Coupled to styling - Misguided. Obviously a user is looking for a button to click. Nobody cares if it is a real button or a different element that looks like one.
3. Using #main [Sparingly] Coupled to styling or JS listeners - Makes sense to use IDs in specific cases where you just need to limit the scope of other selections.
4. Using visible text [Depends] text content may change - Not likely that the default text will change. We aren't (at least not yet) testing any language except the default en_GB. There is no good reason given why we should be able to ignore changes made to what the user sees and relies on to interact with the app.
5. Using data-cy attribute [Always] - Arguably should be used as a last resort because nobody sees or cares about it. In every case I can think of, there are far better ways to identify elements we want to interact with.

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.

[AdrienClairembault]

Thank you for the link to testing-library, I agree that their rules are very interesting and might be an upgrade because they force us to take accessibility into account.

From my understanding, if we always use getByLabel for all inputs, then we are certain that things like screen readers are fully supported.

Taking into account that "content based selector" are the least impactful infraction according to cypress:

By using the testing-library, we take this "minor" downside and add the big upside of accessibility checks, leading IMO to an improvement overall.

I suggest that we add the @testing-library/cypress dependency and use their selectors as much as possible (while defaulting to data selector when it is not possible).

[cconard96]

I suggest that we add the @testing-library/cypress dependency and use their selectors as much as possible (while defaulting to data selector when it is not possible).

Agree. There are actually a few Cypress plugins/custom commands I wanted to look at adding as we build out the tests.
Some of the ones that caught my eye:

• https://github.com/component-driven/cypress-axe (Accessibility tests)
• https://github.com/4teamwork/cypress-drag-drop (For when we test sorting/drag-and-drop)
• https://github.com/filiphric/cypress-plugin-steps (Could probably be implemented as a simple command without this library)
• https://github.com/NoriSte/cypress-wait-until (Could be useful when you want to wait for something besides an element)

[AdrienClairembault]

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 cy.findByRole('combobox', {name: "Login source"}) which does not work because the select item has the aria-hidden property set to true.

cy.findByLabelText("Login source") works as I suppose it does not take visibility into account.

However, I don't think this solution is good because we are interacting with the HTML select item, while our actual users will interact with the select2 display.

We probably need to implement a custom select2 command like found here, but it should be looked at in another PR.

To do this properly, cy.findByRole('combobox', {name: "Login source"}) should return select2's combobox, but it does not seems possible with the current DOM:

Unless I am misunderstanding something, shouldn't the Login source label be for select2's combobox, not the native select node ?
I am also confused by the aria-labelledby part on select2's combobox, as it references its selected value, which make no sense to me.

[cconard96]

However, I don't think this solution is good because we are interacting with the HTML select item, while our actual users will interact with the select2 display.
We probably need to implement a custom select2 command like found here, but it should be looked at in another PR.

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.

Unless I am misunderstanding something, shouldn't the Login source label be for select2's combobox, not the native select node ? I am also confused by the aria-labelledby part on select2's combobox, as it references its selected value, which make no sense to me.

Select2 has several known accessibility issues. Their handling, or lack thereof, of labels is one of them.
https://github.com/select2/select2/labels/accessibility

[AdrienClairembault]

Lets see later for select2, should be good for merge now.