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

Convert tests to use RuleTester instead of --report-unused-disable-directives #31

Merged
merged 4 commits into from Aug 30, 2021

Conversation

jtbandes
Copy link
Member

@jtbandes jtbandes commented Aug 30, 2021

Public-Facing Changes

Description
The RuleTester provides the ability to assert on specific error messages and fixes that rules produce. It's also more in line with how the upstream projects (eslint and typescript-eslint) test their rules, which will smooth the process if we decide to upstream these rules.

The tests for (and changes to) no-meaningless-void-operator are changes I made based on feedback on my typescript-eslint PR: typescript-eslint/typescript-eslint#3641

Imported jest.config.js from template-typescript.

…rectives

The RuleTester provides the ability to assert on specific error messages and fixes that rules produce. It's also more in line with how the upstream projects (eslint and typescript-eslint) test their rules, which will smooth the process if we decide to upstream these rules.

The tests for (and changes to) no-meaningless-void-operator are changes I made based on feedback on my typescript-eslint PR: typescript-eslint/typescript-eslint#3641
@jtbandes jtbandes requested a review from amacneil August 30, 2021 18:57
@@ -1,5 +1,6 @@
// Use --report-unused-disable-directives to validate errors
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed this file to .example.ts instead of .test.ts so it doesn't get run under jest, only linted.

package.json Show resolved Hide resolved
Comment on lines +3 to +4
// eslint-disable-next-line @typescript-eslint/no-var-requires
const rule = require("./no-boolean-parameters") as TSESLint.RuleModule<
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using import I get errors because TS can't find a corresponding declaration file. In the future, we should convert the rules themselves to TypeScript and then we wouldn't need a separate declaration file. (I recall hearing they are currently .js because it allows us to easily use this package to lint itself... might need to look for alternate solutions there)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could just require a compile step before running the tests - shouldn't be too hard to set up.

other reasons they are in js aren't super valid:

  • I think one original rule came from the studio repo where we didn't want to require a compile step
  • when writing the triple equals rule I was just lazy because most example code was in JS

@amacneil
Copy link
Contributor

Cool, didn't know about RuleTester

@jtbandes jtbandes merged commit 6d943de into main Aug 30, 2021
@jtbandes jtbandes deleted the jacob/ruletester branch August 30, 2021 19:37
jtbandes added a commit that referenced this pull request Aug 30, 2021
…in `@foxglove/typescript` config (#32)

**Public-Facing Changes**
None (restoring existing default behavior of configs provided by this plugin)

**Description**
Update docs and default configs based on changes in #31
@jtbandes jtbandes mentioned this pull request Aug 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants