Skip to content
This repository has been archived by the owner on Aug 11, 2021. It is now read-only.

Add data-cy-* for items under /settings/appearance #111

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

DeepSpaceHarbor
Copy link
Contributor

@DeepSpaceHarbor DeepSpaceHarbor commented Jul 17, 2019

Summary
Some of the components don't have unique attributes. Without them, the e2e tests will end up with weird selectors and code that's hard to understand.

This will add data-cy-* attributes to the elements shown under /settings/appearance

fyi: We already did this with login (#106).
The plan is to the the same with the rest of the app, one pr at a time.

Change log:

  • Added data-cy for elements under settings/appearance
  • Added data-cy-* for notifications.
  • Changed the radio component. Added cypress tag to the fake radio button.
  • Changed the toggle component. Added cypress tag to the fake toggle instead of the input element.

@mmso
Copy link
Member

mmso commented Jul 23, 2019

Changed the radio component. Added cypress tag to the fake radio button.
Changed the toggle component. Added cypress tag to the fake toggle instead of the input element.

I don't like the cypressTag prop that much because I think it becomes too involved in the component. I am fine to add data-cypress-whatever as an attribute that's passed onto the element.

As an alternative because it doesn't work for the cases where the component has a different style for the inputs (since cypress can't click on 0-height elements?), I think selecting it through cypress with label[for=${id}] would be ok even if it's said to be used sparingly on https://docs.cypress.io/guides/references/best-practices.html#Selecting-Elements because in here it's not connected to a specific CSS class or JS listener so it does not have a reason to change.

containers/notifications/Container.js Show resolved Hide resolved
@@ -150,6 +150,7 @@ const LayoutsSection = () => {
stickyLabels={StickyLabels}
loading={loadingStickyLabels}
onToggle={(value) => withLoadingStickyLabels(handleToggleStickyLabels(value))}
cypressTag={{"data-cy-appearance":"sticky-labels"}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we always use data-cy we can pass cypressTag="appearance:sticky-labels"

@dhoko
Copy link
Contributor

dhoko commented Sep 19, 2019

I don't like the cypressTag prop that much

Idem, but we need to find a way to tag the component a prop is a solution, maybe the name is too much "cypress" 😁

think selecting it through cypress with label[for=${id}]

It's better to use something not linked to the app itself/style etc. Having a data attribute is the best solution as we won't remove it by mistake because of what it means. We can't forget about it.

using id/classNames will create an issue one day, it always. That's why people always try to scope classNames when using JS or CSS.

@DeepSpaceHarbor
Copy link
Contributor Author

Yeah, cypressTag is a bit weird. I'm open to suggestions for better name(s).

@mmso
Copy link
Member

mmso commented Sep 19, 2019

For me the issue is Cypress. Why can it not press inputs with 0 height? The browser can, so it should be able too.

@DeepSpaceHarbor
Copy link
Contributor Author

Because, you want to interact with the same elements as the real user.
The real user wont be able to click that 0 height element, it makes sense for the test to avoid that as well.

@mmso
Copy link
Member

mmso commented Sep 19, 2019

The user is also interacting with the real input. Otherwise a change would never be triggered. In this case the label just defers the click to the input. So it does not matter if it happens on the input or on the label.

@mmso
Copy link
Member

mmso commented Sep 19, 2019

Why do you not always use data-cy? For this special case, I would be ok to destructure data-cy and put that on the label.

@DeepSpaceHarbor
Copy link
Contributor Author

Closing. The focus has shifted to manual testing.
Will review again if needed when we start test automation.

@DeepSpaceHarbor DeepSpaceHarbor deleted the test/appereance branch December 18, 2019 19:31
@mmso mmso restored the test/appereance branch December 20, 2019 22:37
@mmso
Copy link
Member

mmso commented Dec 20, 2019

Ok that's unfortunate. I feel like we were on the right track here. I will re-open this and take it over when I have more time.

@mmso mmso reopened this Dec 20, 2019
@mmso mmso self-assigned this Dec 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants