Unify end-to-end test framework data QA selector to align with feature specs for easibility

added to epic &11

changed title from Unify our data QA selector to Unify end-to-end test framework data QA selector to align with feature specs to easibility

mentioned in epic &11

Some considerations while solving the problem of unifying the selectors between feature specs and end-to-end specs:

The feature tests section in the Testing best practices documentation encourages using Capybara’s semantic methods and avoiding querying by IDs, classes, or attributes.

Example of what the testing guide for feature tests considers good and bad:

Good: click_button _('Sign in')

Bad: find('[data-testid="sign-in"]').click

It also describes the reasons for this notably: "It is less brittle, as it avoids querying by IDs, classes, and attributes, which are not visible to the user." and "It is more readable, as it uses more natural language." It describes using data-testid as acceptable when scoping to elements that do not have a label.

The end-to-end tests use data: { qa_selector: 'sign_in_button' } format on the view files which can then be used in the test with click_element :sign_in_button. The format the e2e tests use is equally readable. However, it involves adding a selector which the Capybara’s semantic methods do not.

If we go with using the Capybara’s semantic methods in e2e tests, we would run into the problem of breaking the e2e tests when the UI changes. The e2e tests do not run on every MR. The feature tests on the other hand run more often and so the changes in UI breaking the feature tests are detected and fixed in the same MR.

Currently, this problem is solved for e2e tests by the qa:selectors sanity job which runs on all code changes and detects if there are any changes with the data-qa-selectors used by the e2e specs. If we go with the Capybara’s semantic methods, we will also need to update the Test::Sanity::Selectors code to look for changes to the element's text.

Also, the e2e test framework wraps Capybara's methods with additional logic for battling flakiness, logging and validating required elements. We will still need that additional logic which means we will still not be using the Capybara's semantic methods directly. A potential solution to this problem could be overriding the capybara methods and exposing the same signatures.

Also, a quick note on the size of the effort. This will be a huge task since it will be touching almost every file in the qa/ directory.

@ddavison I would like to know your thoughts on this. Thanks.

I created an issue long ago similar to this.

Provide QA element selection mechanisms to regu... (#183)

In the GitLab source code, we are already used to using data-qa-selector whereas backend and frontend use data-testid.

First, we should unify these selectors. They are duplicated attributes and we have a lot of opportunity for re-use. We can unify them by retiring one of them, then providing a nice mechanism that runs an "and" on the selectors. My suggestion would be to keep data-qa-selector because we already have the ability for extensible data-qa-* attributes for dynamic selection.

Second, instead of something like click_button _('Sign in'), we allow that, but also allow the use of click_button :sign_in. click_button can run a translation in the background of [data-qa-selector='sign_in'],[data-testid='sign_in']

Third, we provide a way to fabricate resources by using a FactoryBot & ActiveResource create(:project, name: 'test') invocation rather than Resource::Project.fabricate_via_api! { |p| p.name = 'test' }

Thanks @ddavison. Switching to data-qa-* selectors in feature tests makes sense . I have updated the proposal based on your suggestions.

I think the third point should have an issue of its own.

One thing to consider when we unify these selectors -

Other projects like CDot is also using data-testid. We could either play catch up, or make sure that this standard is applies across all projects.

@sliaquat This seems like it would be a lot of work for questionable rewards. And if we start using data-qa-selector in feature specs we run the risk of reinforcing the idea that they're the Quality department's responsibility. data-testid is more neutral.

And the description says:

Developers are used to writing capybara at the feature level. They get confused with our QA framework.

I think selectors are only a small part of that confusion, but that small part has a lot to do with us preventing them from using data-qa-selector in feature specs even though they see it in the UI code. I don't think it will be less confusing if we say they should now only use data-qa-selector.

What if we made it possible to use either data-testid or data-qa-selector? That way most engineers could continue to use data-testid, and it doesn't matter if data-qa-selector is used once in a while. There might still be some uncertainty about which to use (and why there are two options), but that's less of a problem if they both work, and we could explain that in documentation.

And if we start using data-qa-selector in feature specs we run the risk of reinforcing the idea that they're the Quality department's responsibility.

This is true.

We still want the flexibility of the data-qa-*. What about using data-test-*

data-test-selector
data-test-name
data-test-whatever

What if we made it possible to use either data-testid or data-qa-selector

We could do this, but we'll run the potential risk of having two applied to the same attribute. We could make a cop to enforce it, but it might be better just to consolidate.

[data-qa-selector='sign_in'],[data-testid='sign_in']

Do we have any buy-in from the greater engineering department? I feel that this would be a change sprung upon folks currently writing feature specs working with testid

Also the testid fields tend to use hypen - seperated variable names, selectors tend to use _ underscores, which would be used?

          data-testid="subscription-details"
          data-qa-selector="subscription_details"

According to the Frontend testing common practices docs ...

A data-testid attribute (recommended by maintainers of @vue/test-utils) optionally combined with shallowMountExtended or mountExtended

I'm seeing countless instances of "shallowMountExtended" and "mountExtended". I'm starting to wonder if this is a non-starter to go towards data-qa-*, since Vue test-utils might expect the data-testid attribute

I'm seeing countless instances of "shallowMountExtended" and "mountExtended". I'm starting to wonder if this is a non-starter to go towards data-qa-*, since Vue test-utils might expect the data-testid attribute

@ddavison Well spotted. Yeh, looks like we won't be able to retire data-testid, at least not in the frontend tests.

There is currently unification on frontend and feature tests as they both use data-testid. Frontend will likely continue to use data-testid after any changes proposed in this issue are implemented. On the feature tests side, if we go with allowing two formats as suggested by @mlapierre above (but using data-test-* instead of data-qa-*) , is it going to reduce confusion or add to it?

This is what we will end up with:

Level	selector
Frontend	`data-testid`
Feature	`data-testid` and `data-test-*`
End-to-End	`data-test-*`

Do we have any buy-in from the greater engineering department? I feel that this would be a change sprung upon folks currently writing feature specs working with testid

@willmeek Thanks. Yes. I think it makes sense to get feedback from a few folks from the greater engineering dept once we the proposal ironed out.

Seems that if we must keep data-testid in frontend tests, then if we want to reduce confusion and complexity we should switch to data-testid everywhere.

Changing all the QA selectors and associated code is probably less work than changing all the selectors in frontend and feature specs. And there's fewer of us who would have to adapt to the change.

We could adapt gradually by allowing either but document that data-qa-selector is deprecated, then work on changing them all to data-testid, then add a cop to enforce data-testid.

WDYT?

Also the testid fields tend to use hypen - seperated variable names, selectors tend to use _ underscores, which would be used?

Either? Treat it like ' vs. " and allow either but prefer consistency?

Also note that data-testid can come from generic components from https://gitlab.com/gitlab-org/gitlab-ui. Switching to data-qa-* would be even less feasible given it spans outside of just gitlab project.

I had to do a lot of work with selectors for the new sidebar, and having to define additional selectors in tandem with already existing data-testid created a lot of unnecessary work so I agree with Mark, we should support it in E2E tests.

It looks like the proposal looks like

~~1. Update feature tests framework to use data-qa-* attributes~~

Update documentation to encourage using data-testid in E2E tests
Phase out data-qa-selector and replace it with data-testid attributes
Add a rubocop rule to disallow data-qa-selector

@willmeek Almost. I added Add support for using data-testid attribute in e2e test framework to the top of the list. Thanks

Thanks Sanad!

changed title from Unify end-to-end test framework data QA selector to align with feature specs to easibility to Unify end-to-end test framework data QA selector to align with feature specs for easibility

added #1739 (closed) as child task

added #1740 (closed) as child task

added #1741 (closed) as child task

added #1742 as child task

changed the description

I can pick this up this quarter if there are no objections

assigned to @willmeek

mentioned in issue #1725 (closed)

mentioned in issue gitlab-org/gitlab#411445 (closed)

changed the description

Unify end-to-end test framework data QA selector to align with feature specs for easibility

Problem

Proposal

Pushed out to a future iteration

Designs

Child items ...

Activity

Unify end-to-end test framework data QA selector to align with feature specs for easibility

Problem

Proposal

Pushed out to a future iteration

Relates to

Activity