-
Notifications
You must be signed in to change notification settings - Fork 468
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
Frontend: Unit tests /components directory, convert to svg icons #8716
Frontend: Unit tests /components directory, convert to svg icons #8716
Conversation
5364946
to
218f716
Compare
…l and platform cell tests
218f716
to
466e04c
Compare
nice. we should be able to close some of these issues here when we merge this in. #8773 |
🔥 |
|
||
// Click eye icon | ||
const eyeIcon = screen.findByTestId("eye-icon"); | ||
await user.click(await eyeIcon); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you need the await in await eyeIcon
?
would this work?
const eyeIcon = screen.getByTestId("eye-icon");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I was wondering why it wasn't working, merp, the diff between find by test id and get by test id.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very nice PR. nice work 👍🏽
|
||
describe("Issue cell", () => { | ||
it("renders icon, total issues, and failing policies tooltip", async () => { | ||
const { user } = renderWithSetup( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The custom renderer will always set up the user events for testing. what do you think about doing this as an alternative?
const render = customRender()
const { user } = render(...)
tbh I kinda prefer just using renderWithSetup when you only need user events setup. what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Rachel, read this later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ghernandez345 looks a lot cleaner, only issue I see is when we need { user } and render is defined already so we can't redefine it so it looks wonky (e.g. RevealButton.tests.tsx)
I went ahead and made this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where do you mean? I dont see where that is the case
@ghernandez345 @RachelElysia Would y'all please make sure any tests covered here are closed in the related epic? Feel free to bring them onto the release board if you want. Since they don't need QA, it's not strictly necessary. |
Unit tests added:
Other
import {v4 as uuid } from "uuid";
because it was throwing an error with the typescript compiler when running a test on the file. Replaced all 4 instances with with loadashuniqueId
SVG icons added moved to icon PR so this PR can focus soley on unit tests
Checklist for submitter
If some of the following don't apply, delete the relevant line.