-
Notifications
You must be signed in to change notification settings - Fork 7
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
chore: test setup + accordion tests #712
Conversation
Doesn't seem like we're using Playwright.
@@ -15,7 +15,6 @@ module.exports = { | |||
"plugin:jsx-a11y/recommended", | |||
"airbnb-typescript", | |||
"plugin:sonarjs/recommended", | |||
"plugin:jest-playwright/recommended", |
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.
As far as I can tell, there aren't any playwright tests.
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.
Are we adding any today?
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.
It's not mentioned in the spreadsheet.
Also, Playwright itself isn't in this repo. This is just an eslint plugin. Easy enough for someone to add if they do add Playwright 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.
That sounds good! With Chromatics, maybe there's no immediate need to use Playwright. But if we do, we can add back later 👍
@@ -1,7 +1,7 @@ | |||
module.exports = { | |||
moduleDirectories: ["node_modules", "<rootDir>"], | |||
preset: "ts-jest", | |||
setupFiles: ["jest-canvas-mock", "../../intersectionObserverMock.js"], | |||
setupFilesAfterEnv: ["./jest.setup.ts"], |
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.
Removed the unused jest-canvas-mock dep.
Also moved the intersectionObserver mock and the testing-library/jest-dom setup into a setup file (which makes the typescript integration easier).
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.
Oh great idea thanks for doing this!
|
||
// Returns a component that already contain all decorators from story level, meta level and global level. | ||
const Test = composeStory(TestStory, Meta); | ||
const { Test } = composeStories(stories); |
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.
Since we already have an import of the whole stories file, we can remove an import by using composeStories
instead of composeStory
.
|
||
it("renders accordion component", () => { | ||
render(<Test {...Test.args} />); | ||
render(<Test />); |
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.
No need to pass in the story's args. Using composeStory
/ composeStories
already does that.
@@ -0,0 +1,2 @@ | |||
import "@testing-library/jest-dom"; | |||
import "../../intersectionObserverMock"; |
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.
Looks like data-viz doesn't have any tests, so we could remove jest and its setup from it if we wanted to simplify things a bit.
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.
I think @masoudmanson will start adding tests in December for data-viz Heatmap, so let's keep it 💪 Thank you!
expect(accordionHeader).toHaveAttribute("aria-expanded", "false"); | ||
|
||
await waitFor(() => { | ||
expect(screen.getByText(/Lorem ipsum/)).not.toBeVisible(); |
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.
Why does this line need the waitFor when the lines above do not need it?
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.
It seems like going from expanded back to collapsed is async for some reason, and takes a couple ms to happen.
Not sure why it's different than the collapsed -> expanded case.
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.
Looks good! Left a couple questions.
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.
LGTM! Thanks for creating the first SDS Hackday PR, @ahuth 🔥 🏆
Only one tiny non-blocking nit!
@@ -1,7 +1,7 @@ | |||
module.exports = { | |||
moduleDirectories: ["node_modules", "<rootDir>"], | |||
preset: "ts-jest", | |||
setupFiles: ["jest-canvas-mock", "../../intersectionObserverMock.js"], | |||
setupFilesAfterEnv: ["./jest.setup.ts"], |
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.
Oh great idea thanks for doing this!
|
||
// Starts closed | ||
const accordionHeader = screen.getByRole("button"); | ||
// eslint-disable-next-line sonarjs/no-duplicate-string |
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.
nit: should we extract duplicate string as a constant 🙏 ?
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.
Sure, will do
// Starts closed | ||
const accordionHeader = screen.getByRole("button"); | ||
// eslint-disable-next-line sonarjs/no-duplicate-string | ||
expect(accordionHeader).toHaveAttribute("aria-expanded", "false"); | ||
expect(screen.getByText(/Lorem ipsum/)).not.toBeVisible(); | ||
|
||
// It opens when clicked. | ||
fireEvent.click(accordionHeader); | ||
expect(accordionHeader).toHaveAttribute("aria-expanded", "true"); | ||
expect(screen.getByText(/Lorem ipsum/)).toBeVisible(); | ||
|
||
// And closes when clicked again. | ||
fireEvent.click(accordionHeader); | ||
expect(accordionHeader).toHaveAttribute("aria-expanded", "false"); | ||
|
||
await waitFor(() => { | ||
expect(screen.getByText(/Lorem ipsum/)).not.toBeVisible(); | ||
}); |
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.
so much better 🔥 Thank you!
@@ -0,0 +1,2 @@ | |||
import "@testing-library/jest-dom"; | |||
import "../../intersectionObserverMock"; |
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.
I think @masoudmanson will start adding tests in December for data-viz Heatmap, so let's keep it 💪 Thank you!
Thanks 🙏 |
🚨 SDS Hackday PR! 🚨
This PR does a couple things:
jest-canvas-mock
andeslint-plugin-jest-playwright
deps@testing-library/jest-dom
so we can use its matchersI'm planning on going through more of the tests, but wanted to stop here and get the test setup and DOm matchers in.