Skip to content

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

Merged
merged 6 commits into from
Mar 3, 2021

Conversation

koba04
Copy link
Collaborator

@koba04 koba04 commented Jan 26, 2021

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 for fetcher 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.

  • Using @testing-library/jest-dom for asserting DOM elements
  • Using eslint-plugin-jest-dom and eslint-plugin-testing-library to avoid common mistakes.
    • The API of testing-library is a bit complex, so the plugins would be helpful.
  • Refactoring use-swr-cache.test.tsx
    • I've replaced queryByText that is returned from render with screen.findByText
    • I've used toBeInTheDocument instead of toMatchInlineSnapshot because it can assert the test cases by verifying the textContent.

If this makes sense, I'll refactor all tests.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jan 26, 2021

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:

Sandbox Source
SWR-Basic Configuration
SWR-States Configuration
SWR-Infinite Configuration

custom cache message
</div>
`)
expect(await screen.findByText('custom cache message')).toBeInTheDocument()
Copy link
Collaborator

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.

Copy link
Collaborator Author

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 )

Copy link
Collaborator

@promer94 promer94 Jan 27, 2021

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

Copy link
Collaborator Author

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')
Copy link
Collaborator Author

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.

Copy link
Member

@shuding shuding left a comment

Choose a reason for hiding this comment

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

Thanks!

@shuding shuding merged commit 4f6f61b into vercel:master Mar 3, 2021
@koba04 koba04 deleted the refactor-use-swr-cache-test branch March 3, 2021 15:25
@koba04
Copy link
Collaborator Author

koba04 commented Mar 3, 2021

Thank you!

This was referenced Mar 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants