DSD-1748: Adding interaction tests for RadioGroup #1597
Conversation
EdwinGuzman
requested review from
bigfishdesign13,
jackiequach,
oliviawongnyc and
7emansell
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| await userEvent.click(screen.getByLabelText("Radio 3")); | ||
| await userEvent.click(screen.getByLabelText("Radio 2")); | ||
| await userEvent.click(screen.getByLabelText("Radio 5")); | ||
| await userEvent.keyboard("{arrowdown}"); | ||
| await userEvent.keyboard("{arrowleft}"); | ||
| await userEvent.keyboard("{arrowup}"); | ||
| await userEvent.keyboard("{arrowright}"); |
There was a problem hiding this comment.
This code executes events, but shouldn't there also be tests that verify the outcome of the events?
There was a problem hiding this comment.
I thought the same. I was a bit confused but the Storybook doc examples don't always have expectations https://storybook.js.org/docs/writing-tests/interaction-testing
The actions and interactions are the tests. When I did expectations, such as expect(screen.getByRole("radiogroup")).toBeInTheDocument(); above, they don't show up in the addon panel. So we can add more expectations but what is shown to users are the interactions.
There was a problem hiding this comment.
What happens if you add an expectation and it fails?
There was a problem hiding this comment.
The error will be shown. It's not very intuitive but it's an implicit success, so you're not told anything until something throws an error.
There was a problem hiding this comment.
Do you think this will ever replace the unit tests?
There was a problem hiding this comment.
I ask because I feel an expectation should be added whenever an userEvent is executed. We do that with the unit tests. Should we also do that here as well. We can add all the click events that we want, but if nothing changes after the event and we don't explicitly test to verify that it has changed, does the fact that we executed a click event mean anything?
There was a problem hiding this comment.
I think adding interactions and expectations is required for unit tests since it's testing the component in a fundamental and technical manner. In these tests, it's enough that the interactions work since that's what users do on sites and it's also how Storybook write the example tests as well.
There's also this idea of implicit assertions (it's on the site from the creator of React Testing Library). He posted this recently https://x.com/kentcdodds/status/1765501275003580800 and it's very interesting because it's not explicitly asserting anything, just that the page loads and that's enough.
That's something I'd like to also be considered here where if the keyboard and mouse interactions work, then we don't really need explicit expectations.
Regardless though, I'd like to keep how we think about unit tests and these interaction tests as separate types of tests.
There was a problem hiding this comment.
@EdwinGuzman Looking back at the article about implicit assertions, none of the examples talk about testing user events. They are all related to values and formats, but not to an event that may cause something to happen or change. With the interective tests, we are triggering events, but can there always be an implicit assertion that something on screen or in the code has changed simply because an event was triggered?
There was a problem hiding this comment.
Right, I'd say yes and no and generally depends on the component we are testing and what we are testing for. If we want to test tabbing and moving arrows around, we visually see that new values get selected so it's working. It doesn't hurt to add expectations though.
I can add some expectations but because of how the addons display the tests, it won't appear in the list of interactions.
There was a problem hiding this comment.
I can add some expectations but because of how the addons display the tests, it won't appear in the list of interactions.
I think that is ok. They won't verify success, but they will throw errors if there is a problem.
|
Ready for another review @7emansell @jackiequach @bigfishdesign13 |
| await userEvent.keyboard("{arrowup}"); | ||
| await userEvent.keyboard("{arrowright}"); | ||
| await userEvent.keyboard("{arrowdown}"); | ||
| expect(screen.getByLabelText("Radio 2")).toBeChecked(); |
There was a problem hiding this comment.
Odd, I now see this in the UI.
bigfishdesign13
added
Ship It
Fixes JIRA ticket DSD-1748
This PR does the following:
RadioGroupcomponent. It checks for both clicking on radio inputs and using up/down and left/right keys presses to select new values.How has this been tested?
Storybook
Accessibility concerns or updates
Checklist:
Front End Review: