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

(Ideally) Fail on Fixed Linter Errors #44

Closed
scott-m-sarsfield opened this issue Jun 8, 2018 · 8 comments
Closed

(Ideally) Fail on Fixed Linter Errors #44

scott-m-sarsfield opened this issue Jun 8, 2018 · 8 comments

Comments

@scott-m-sarsfield
Copy link

My team is using this runner to lint our app. We have the linter called by individual developers and our CI.

When it's an individual developer, we want to auto-fix any linter issues it comes across (we usually do this before merging). We don't mind re-doing the linter to get all green. (We have to do the same with ruby / rubocop.)

For the CI, though, we want it to fail / terminate, so that the imperfect code doesn't propagate through our pipeline.

Thoughts:
(1) If it could just fail whenever a linter issue is autofixed, then there's no problem.
(2) If I could have separate configurations of the runner (fix and not to fix), then that'd also address it.

@scott-m-sarsfield
Copy link
Author

scott-m-sarsfield commented Jun 8, 2018

In

const report = cli.executeOnFiles([testPath]);
if (options.cliOptions && options.cliOptions.fix) {
CLIEngine.outputFixes(report);
}
const end = Date.now();
if (report.errorCount > 0) {
const formatter = cli.getFormatter(options.cliOptions.format);
const errorMessage = formatter(CLIEngine.getErrorResults(report.results));
return fail({
start,
end,
test: { path: testPath, title: 'ESLint', errorMessage },
});
}

a fix, I believe, can be detected with report.results.some( result => result.output )

@ljharb
Copy link
Collaborator

ljharb commented Jun 8, 2018

I'm confused; if you want it to fail on things that are autofixed, can you not disable autofix in CI?

@ljharb
Copy link
Collaborator

ljharb commented Jun 8, 2018

Specifically, you can use two separate jest commands, pointing to separate configs, one for CI and one for local.

@rogeliog
Copy link
Member

rogeliog commented Jun 8, 2018

Thanks for reporting!

Let me make sure that I correctly understood the problem. You want --fix in development, but not in CI right?

You can add a config file for this runner jest-runner-eslint.config.js, and then do the following:

module.exports = {
  cliOptions: {
    fix: !process.env.IS_CI
  }
}

@rogeliog
Copy link
Member

rogeliog commented Jun 8, 2018

As @ljharb mentioned, having two separate configs is also a good option for achieving this.

@scott-m-sarsfield
Copy link
Author

@ljharb To be honest, I haven't had much experience with jest in general.

I've set it up so that I have separate jest configs for unit testing + linting, but I put my jest-runner-eslint configuration in package.json and I'm unclear how I'd configure separate jest-runner-eslint configurations. (Since I thought jest-runner-eslint.config.js and .jest-runner-eslintrc were the only files outside of package.json that could be used for configuration.)

current lint jest config:

{
  "displayName": "lint",
  "rootDir": "../../../",
  "runner": "jest-runner-eslint",
  "testMatch": [
    // ...
  ]
}

However, @rogeliog 's solution seems actionable.

@scott-m-sarsfield
Copy link
Author

Since I'm satisfied that I have a good way forward, I'm going to close the issue.

However, I'm still interested how one would go about having two separate jest-runner-eslint configs.

@rogeliog
Copy link
Member

rogeliog commented Jun 8, 2018

I think once jestjs/jest#4278 happens, it should eliminate the need for the separate config file.

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

No branches or pull requests

3 participants