Skip to content
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

Enforce best practices for testing #2357

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

lindapaiste
Copy link
Collaborator

I'm not the biggest expert when it comes to testing components, so I looked up some guidelines and found that there's a lot of things which can be improved in existing unit tests.

See:

Changes:

  • Installed eslint plugins for react testing library and jest-dom which will let us know when we're doing something wrong.
  • Removed unnecessary act(() => {}) around render, fireEvent, etc. Which was actually every act in the code!
  • Use specific assertions like toHaveValue(), toBeDisabled(), toBeChecked(), etc.
  • Replace .mock.calls[0][0]).toBe() with toHaveBeenCalledWith().
  • Use getBy* when accessing elements that should be present, and queryBy* only with .not.toBeInTheDocument();
  • Fixed the buggy expectFileNameToBe checker which would always pass, as it had no expect condition and used a queryBy (which allows non-existance). It now uses an await find.

Not Fixed:

  • I fixed some example code snippets in contributor_docs/testing.md, but this entire document could use a review.
  • I put an // eslint-disable-next-line testing-library/no-render-in-setup in the client/index.integration.test.jsx file rather than fixing it.
  • I turned off the rule testing-library/render-result-naming-convention because it doesn't like the way that we write const props = renderComponent(); in the Toolbar and FileNode tests.
  • I have not switching to using the @testing-library/user-event library, as it caused errors.

I have verified that this pull request:

  • has no linting errors (npm run lint)
  • has no test errors (npm run test)
  • is from a uniquely-named feature branch and is up to date with the develop branch.
  • is descriptively named and links to an issue number, i.e. Fixes #123

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant