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

Add ESLint plugins for Jest, Jest DOM, and Testing Library #8155

Closed
wants to merge 6 commits into from

Conversation

nickserv
Copy link
Contributor

@nickserv nickserv commented Dec 11, 2019

Rationale

I was excited to see Testing Library get added to the default template (cra-template), but I noticed the officially recommended ESLint plugins were missing from eslint-config-react-app. They both help to make matcher usage more consistent and fix some bugs where improper usage can causing failing or false positive test results. I also added the Jest plugin which has similar benefits for ordinary Jest usage (with or without testing library packages).

Changes

Added recommend ESLint configs for Jest, Jest DOM, and React Testing Library.

Note on react-scripts

The webpack config used in react-scripts currently only runs ESLint via eslint-loader from the app's entry point. Unfortunately this means that test files will be excluded from the built-in ESLint runner. To make these config additions more useful in react-scripts I attempted to set up jest-runner-eslint, but I'm getting the following error when adding the multiple projects configuration:

Error: Jest: Cannot use configuration as an object without a file path.

Source: https://github.com/nickmccurdy/create-react-app/blob/92075f408baddacde5863fcf9bff6ca9ded1299e/packages/react-scripts/scripts/utils/createJestConfig.js#L68-L80

Help with this error would be appreciated, as it would be a lot more useful to have these ESLint plugins working in react-scripts. Currently they're still useful when using ESLint outside react-scripts, such as through an ESLint extension or the eslint CLI outside of a CRA app.

Testing

  • Ran eslint manually (outside of react-scripts) and confirmed that linter rules are enabled.
  • Created an example app with yarn create-react-app my-app (note: this doesn't support linting test files yet, it's just a smoke test to make sure the ESLint config isn't broken).

@ianschmitz
Copy link
Contributor

Thanks for the PR. Can you clarify the thinking with adding this even though we don't run ESLint on the test files? I'm not sure what value this brings to CRA without that.

@nickserv
Copy link
Contributor Author

nickserv commented Dec 13, 2019

I have code in progress to do that, but it throws this error when I follow the jest-runner-eslint usage, and help would be appreciated: Error: Jest: Cannot use configuration as an object without a file path. (source: nickserv@92075f4)

I removed this commit from this PR to fix the broken Jest config so that CRA will still work without the test rules, and eslint-config-react-app will work outside CRA with the test rules.

@ianschmitz ianschmitz self-assigned this Dec 13, 2019
@stale
Copy link

stale bot commented Jan 12, 2020

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Jan 12, 2020
@nickserv
Copy link
Contributor Author

Still should be valid as far as I’m aware, but I’d be happy to take another look if someone can help with this error

@Belco90
Copy link

Belco90 commented Feb 4, 2020

I almost open the same PR for setting up some testing-library eslint plugins, glad to have found this issue! I hope it gets merged eventually.

@nickserv nickserv requested a review from amyrlam as a code owner February 5, 2020 19:29
@nickserv
Copy link
Contributor Author

nickserv commented Feb 6, 2020

I've updated the dependencies for this pull request. I'm still having the Jest config error with the latest version of jest-runner-eslint, so I'd appreciate help figuring that out (unless we decide to merge before it's fixed).

@Belco90
Copy link

Belco90 commented Feb 6, 2020

I'll try to help you, but I'm afraid I'm not an expert in this matter.

@Belco90
Copy link

Belco90 commented Feb 10, 2020

@nickmccurdy Sorry I didn't have spare time to check this, but @thomlom (the other amazing collaborator of eslint-plugin-testing-library) mentioned recently this in a chat:

If you use jest-runner-eslint to lint your files, you have to add the files to testMatch too

Just sharing here in case it helps you in some way, as we couldn't check the issue with jest-runner-eslint.

@nickserv
Copy link
Contributor Author

I appreciate the effort, but I'm already using testMatch and it's still erroring unfortunately

@nickserv nickserv force-pushed the testing-library-linting branch from e92aee2 to 35664ef Compare April 23, 2020 18:17
@nickserv
Copy link
Contributor Author

I've rebased both branches to fix some formatting issues when merging in recent changes. They're now up to date, but the testing-library-linting-integration branch still has the Jest error mentioned above.

@NMinhNguyen
Copy link
Contributor

Thanks for the PR. Can you clarify the thinking with adding this even though we don't run ESLint on the test files? I'm not sure what value this brings to CRA without that.

@ianschmitz I'd say one big benefit is that VS Code (and any other IDE) would provide linting in your IDE.

@mrmckeb
Copy link
Contributor

mrmckeb commented Jun 21, 2020

@kentcdodds recommends these, and I've been using them at work and the team find them helpful. That being said, they're also very easy for users to add in.

@kentcdodds
Copy link
Contributor

They are pretty easy for users to add, but the people who benefit from them most are the people who don't know they exist.

@mrmckeb
Copy link
Contributor

mrmckeb commented Jun 21, 2020

I think we should get this one in personally, as there is value for beginners, and advanced users can opt out if they wish (by replacing the ESLint config).

It looks like this is related: #8963.

@NMinhNguyen
Copy link
Contributor

I think we should get this one in personally

Agreed.

@nickserv
Copy link
Contributor Author

Merge conflicts fixed, ready to merge

@nickserv
Copy link
Contributor Author

nickserv commented Jul 1, 2020

Closing in favor of #8963 which also supports ESLint 7

@nickserv nickserv closed this Jul 1, 2020
@nickserv nickserv deleted the testing-library-linting branch June 23, 2022 17:32
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.

7 participants