-
Notifications
You must be signed in to change notification settings - Fork 1.3k
test: refactor use-swr-cache.test.tsx #932
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
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit aa78083:
|
test/use-swr-cache.test.tsx
Outdated
custom cache message | ||
</div> | ||
`) | ||
expect(await screen.findByText('custom cache message')).toBeInTheDocument() |
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.
if await screen.findByText('custom cache message')
does not throw, it means that the text content you are looking for is in the document. You don't need do another assertion here.
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.
@promer94
Thank you for your feedback! That's right.
But I prefer to have an assert function explicitly to indicate this line is an assertion.
What about writing like this?
await expect(screen.findByText('custom cache messagea')).resolves.toBeTruthy()
This is the way dom-testing-lijbrary
is using in the internal tests.
https://github.com/testing-library/dom-testing-library/blob/master/src/__tests__/queries.find.js#L41
(I don't have a strong opinion for this, so it's ok to change the way not using an assert function )
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.
await expect(screen.findByText('custom cache messagea')).resolves.toBeTruthy()
In this way, we are actually testing the findByText API rather than our component.
And here is the example of findby-queries
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've fixed it at d403452
@@ -88,13 +88,13 @@ describe('useSWR - config callbacks', () => { | |||
// props changed, but the onError callback doese not trigger yet. | |||
rerender(<Page text="b" />) | |||
expect(container.firstChild.textContent).toMatchInlineSnapshot(`"Error: 0"`) | |||
expect(container.firstElementChild.getAttribute('title')).toEqual('b') |
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.
This raises an ESLint error and has been fixed automatically by the ESLint plugin.
In #933, this would be replaced with the way that doesn't use container
, so it's a temporary solution.
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.
Thanks!
Thank you! |
I've refactored
use-swr-cache.test.tsx
based on Common mistakes with React Testing Library.I've sometimes experienced that the unit tests of SWR become flaky and are failed, but the failed tests got passed when I re-run the tests.
In addition, the tests are slow because they are often using
setTimeout
with 100ms or 200ms forfetcher
functions. (This is not included in this PR because there is no test for the case, so I'll show you this as another PR)To refactor it, I've installed some npm packages to set up. Please let me know if I should separate it from this PR.
@testing-library/jest-dom
for asserting DOM elementseslint-plugin-jest-dom
andeslint-plugin-testing-library
to avoid common mistakes.testing-library
is a bit complex, so the plugins would be helpful.use-swr-cache.test.tsx
queryByText
that is returned fromrender
withscreen.findByText
toBeInTheDocument
instead oftoMatchInlineSnapshot
because it can assert the test cases by verifying the textContent.If this makes sense, I'll refactor all tests.