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

ESLint checks (and the upcoming Flow integration) is missing in tests #1169

Closed
rricard opened this issue Dec 5, 2016 · 7 comments
Closed

Comments

@rricard
Copy link
Contributor

rricard commented Dec 5, 2016

Followup to this comment thread: #1152 (comment)

Description

When running the start or test lint issues or flow issues (with #1152) are not reported if they are not within a test file (not required by webpack, only seen by jest).

Expected behavior

I see three acceptable UX here:

  1. The start script should explore test files as well and run the eslint loader (+ flow plugin) on it
  2. The test script should run eslint (+ flow) each time jest triggers a change from its watch
  3. Add an independent script that will watch all files at the same time

The first one would be the best, the last one would be ok, but let's try to avoid it!

Actual behavior

Test files are completely ignored by static checks.

Environment

Run these commands in the project folder and fill in their results:

  1. npm ls react-scripts (if you haven’t ejected): 0.8.1
  2. node -v: 6.7.0
  3. npm -v: 3.10.3
  4. yarn --version: 0.18.0

Then, specify:

  1. Operating system: macOS Sierra 10.12.2 Beta (16C41b)
  2. Browser and version: Google Chrome 54.0.2840.98 (64-bit)

Reproducible Demo

https://github.com/rricard/cra-failing-test-checks

@thisconnect
Copy link

Can/will npm test lint without flow too?

@gaearon
Copy link
Contributor

gaearon commented May 21, 2017

npm run build runs the linter, is this not enough for your use case?

@thisconnect
Copy link

@gaearon My CI complained about some warnings, somehow I could not reproduce them locally. :/

I've added a lint script in package.json that just runs eslint, works for me now.

"scripts": {
  "lint": "eslint src"
}

see thisconnect/desktop@02d3e2f
(This is a test repo that uses and instantiated create-react-app + electron as in PR #1718)

@gaearon
Copy link
Contributor

gaearon commented May 21, 2017

My CI complained about some warnings, somehow I could not reproduce them locally. :/

The build script didn’t use to output warnings. It does now, if you update to react-scripts@1.0.0 and higher.

@thisconnect
Copy link

Thanks @gaearon !!

@gaearon
Copy link
Contributor

gaearon commented Sep 8, 2017

This is interesting: https://github.com/rogeliog/jest-runner-eslint

@gaearon
Copy link
Contributor

gaearon commented Jan 8, 2018

Meh. It doesn't seem that important, and above all test runs must be fast. Happy to look at PRs implementing this but we won't be doing it ourselves.

@gaearon gaearon closed this as completed Jan 8, 2018
@lock lock bot locked and limited conversation to collaborators Jan 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants