Skip to content
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

test(switch): Redo switch tests in react-testing-library #386

Merged
merged 30 commits into from
Jan 28, 2020

Conversation

lychyi
Copy link
Contributor

@lychyi lychyi commented Dec 27, 2019

Summary

Alright, lots of changes here:

  • Migrated to react testing library
  • Rearranged and added a bunch of tests (both API and styles)
  • Extended Jest matchers for jest-emotion in setupTests instead of locally in PageHeader.spec.tsx
  • Added functional tests in Cypress
  • Added a visual state table for ChromaticQA

Doing these tests revealed a few changes that we'll need to make (tracked in #387):

  • I don't think we need to specify tabIndex={0}, this is the default already.
  • We should map a static member Switch.ErrorType.Alert to ErrorType from canvas-kit-react-common. It's kind of a pain to import that also just to use Switch.
  • Per some conversations that @NicholasBoll had with a11y in the past, we should probably make the role attribute a configurable prop. Instead of checkbox it could be something like switch.
  • We should probably rename the elements SwitchBackground and SwitchCircle to something more semantically correct like SwitchTrack and SwitchThumb.

@cypress
Copy link

cypress bot commented Dec 27, 2019



Test summary

84 0 0 0


Run details

Project canvas-kit
Status Passed
Commit d25189a
Started Jan 28, 2020 12:37 AM
Ended Jan 28, 2020 12:39 AM
Duration 01:57 💡
OS Linux Ubuntu Linux - 16.04
Browser Electron 61

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@lychyi lychyi added this to In progress in Current Sprint (7/20 - 8/9) via automation Jan 2, 2020
Remove conditional testing for check/uncheck
modules/form-field/react/stories/stories_Switch.tsx Outdated Show resolved Hide resolved
getSwitch().click();
});
it('should be checked', () => {
getSwitch().should('be.checked');
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this duplication of RTL/enzyme tests?

Copy link
Member

Choose a reason for hiding this comment

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

Cypress tests are about specifications from a user's perspective. This spec reads in full "Non-disabled Switch when the switch is clicked should be checked". Linking the given of the story being rendered, the when of the switch being clicked, and the then of the side effect of that action where the switch is now checked.

I might argue that "checked" is an implementation detail of a switch being "on", but there is a bit of controversy around how a "switch" should be labelled/read by screen readers from an accessibility standpoint. I don't remember the final say the role attribute of a Switch which effects how it will be read by a screen reader.

I personally avoid actions in unit-level tests including clicks. Most of the time firing a click event is enough for the semantic action we'd call "click", but some components will expect other browser events in a specific order and some events aren't as simple as a "click". I leave that all for Cypress which has wiring for all that. In Cypress, a "click" includes other events normally associated with "click" like "mouseover". I've seen unit tests that try to recreate all that. Yuck.

I wouldn't worry about possible duplication in testing levels. Each serve a specific purpose.

cypress/integration/Switch.spec.ts Outdated Show resolved Hide resolved
cypress/integration/Switch.spec.ts Show resolved Hide resolved
modules/form-field/react/stories/stories_Switch.tsx Outdated Show resolved Hide resolved
modules/switch/react/spec/Switch.spec.tsx Outdated Show resolved Hide resolved
modules/switch/react/spec/Switch.spec.tsx Outdated Show resolved Hide resolved
modules/switch/react/spec/Switch.spec.tsx Outdated Show resolved Hide resolved
modules/switch/react/spec/Switch.spec.tsx Outdated Show resolved Hide resolved
@NicholasBoll
Copy link
Member

Dang, TravisCI is slow! 10s isn't enough to verify a Storybook doc is loaded.

Specify checkbox as an input in test desc
Copy link
Contributor

@anicholls anicholls left a comment

Choose a reason for hiding this comment

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

Very close 👍

modules/form-field/react/stories/stories_Switch.tsx Outdated Show resolved Hide resolved
modules/form-field/react/stories/stories_Switch.tsx Outdated Show resolved Hide resolved
modules/switch/react/spec/Switch.spec.tsx Outdated Show resolved Hide resolved
@lychyi lychyi merged commit fd9544f into Workday:master Jan 28, 2020
Current Sprint (7/20 - 8/9) automation moved this from In progress (PRs) to Done Jan 28, 2020
@lychyi lychyi deleted the switch-tests branch January 28, 2020 00:41
@jpante jpante added this to the Component Testing Coverage milestone Apr 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants