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 an option to avoid fixing no-skip-test #324

Closed

Conversation

edbrannin
Copy link
Contributor

My team's projects run eslint --fix by deafult, and some of our editors apply lint-fixes on save by default.

This has led to some issues with this rule (with I 100% wish to keep using):

  • People adding // eslint-disable-next-line ava/no-skip-test to get around their editors' auto-fix, then accidentally committing the whole file without fixing it
  • Adding this rule to an existing project can strip test.skip() in a bunch of places that deserve manual inspection

This option also lets projects opt-in to working like the eslint docs recommend:

Best practices for fixes:

  1. Avoid any fixes that could change the runtime behavior of code and cause it to stop working.

If this is accepted, I'd like to do the same thing for no-only-test for the same reasons.

@edbrannin
Copy link
Contributor Author

Integration test failure:

[17:34:00] Integration tests [failed]

 got (eslint --format json --config /home/runner/work/eslint-plugin-ava/eslint-plugin-ava/test/integration/eslint-config-ava-tester/index.js /tmp/1919053f89b82a9ed6d360e4babe2161 --parser @typescript-eslint/parser --ext .ts)
Command failed with exit code 1: npx eslint --format json --config /home/runner/work/eslint-plugin-ava/eslint-plugin-ava/test/integration/eslint-config-ava-tester/index.js /tmp/1919053f89b82a9ed6d360e4babe2161 --parser @typescript-eslint/parser --ext .ts
Cannot destructure property 'type' of 'node' as it is undefined.
Occurred while linting /tmp/1919053f89b82a9ed6d360e4babe2161/test/cache.ts:363
Cannot destructure property 'type' of 'node' as it is undefined.
Occurred while linting /tmp/1919053f89b82a9ed6d360e4babe2161/test/cache.ts:363

 got (eslint --format json --config /home/runner/work/eslint-plugin-ava/eslint-plugin-ava/test/integration/eslint-config-ava-tester/index.js /tmp/1919053f89b82a9ed6d360e4babe2161 --parser @typescript-eslint/parser --ext .ts --fix-dry-run)
Command failed with exit code 1: npx eslint --format json --config /home/runner/work/eslint-plugin-ava/eslint-plugin-ava/test/integration/eslint-config-ava-tester/index.js /tmp/1919053f89b82a9ed6d360e4babe2161 --parser @typescript-eslint/parser --ext .ts --fix-dry-run
Cannot destructure property 'type' of 'node' as it is undefined.
Occurred while linting /tmp/1919053f89b82a9ed6d360e4babe2161/test/cache.ts:363
Cannot destructure property 'type' of 'node' as it is undefined.
Occurred while linting /tmp/1919053f89b82a9ed6d360e4babe2161/test/cache.ts:363

I'm at a bit of a loss here. The only type I see in my PR is in the schema, but I based that on no-ignored-test-files so I doubt that's where the problem is.

Maybe I'll try using npm link to see if it works in my projects...

@edbrannin
Copy link
Contributor Author

I tried this on my code with npm link

  • Configuring with { fix: false } had the intended effect :)
  • npx eslint --fix works fine there, so I'm no closer to understanding why the integration tests failed.
  • Actually, this project was Javascript, and it looks like the test failure was on typescript? I don't have much experience there.

If you're ok with this change in general, I'd appreciate some help figuring out what went wrong in the integration tests.

@sindresorhus
Copy link
Member

See: #281 (comment) (Sorry, I should have moved that comment to a separate clear issue)

@edbrannin
Copy link
Contributor Author

@sindresorhus No worries, that's what I get for raising a PR instead of an issue.

I'm a bit concerned about unit tests breakage, but at least the implementation should be a quick fix from what's there now. I may give that a shot tomorrow.

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.

2 participants