-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
feat: add new package rule-tester
#6777
Conversation
Thanks for the PR, @bradzacher! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
3b8c1ac
to
6aa9edc
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## v6 #6777 +/- ##
==========================================
+ Coverage 87.51% 87.61% +0.10%
==========================================
Files 376 375 -1
Lines 12939 12923 -16
Branches 3820 3821 +1
==========================================
- Hits 11323 11322 -1
+ Misses 1231 1216 -15
Partials 385 385
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whew, what a PR! This is great - looking forward to having native dependency contraints!
|
||
import CodeBlock from '@theme/CodeBlock'; | ||
|
||
# `@typescript-eslint/rule-tester` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Docs] Maybe we should explicitly call this out as beta/wip/similar, so folks don't see this and worry they should immediately switch to it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per our chats on discord - @armano2 and I think we should push to make this a breaking change replacement in v6 - people should switch to it cos it's better in many ways and it should mostly be a 1:1 replacement.
Also switching to the package means people get the only: boolean
test prop for free, regardless of their ESLint version.
// we intentionally import from eslint here because we need to use the same class | ||
// that ESLint uses, not our custom override typed version | ||
import { SourceCode } from 'eslint'; | ||
import merge from 'lodash.merge'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Question] Should this be further forked to do the same lodash import style as our other files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about that - but ESLint has this exact import for its rule tester.
So I figured that we'd just use the exact same code to avoid a duplicate install
[nodeSelector: string]: RuleFunction | undefined; | ||
} | ||
// Interface to merge into for anyone that wants to add more selectors | ||
interface RuleListenerExtension {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Docs] This feels like something that should be mentioned in the docs page?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was just me adding some future-proofing in case anyone was doing something weird.
Actually don't need it - but I think it's fine to leave it undocumented for the moment.
@@ -1,24 +1,2 @@ | |||
// Note - @types/json-schema@7.0.4 added some function declarations to the type package | |||
// If we do export *, then it will also export these function declarations. | |||
// This will cause typescript to not scrub the require from the build, breaking anyone who doesn't have it as a dependency |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Question] Is this comment no longer necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah because in the olden days we did export * from 'json-schema'
and we'd export the types for functions that don't actually exist.
But now a days we can do export type *
and it'll not include the runtime bits!
Though #6963 would just be better than making this change at all TBH
rule-tester
PR Checklist
References:
RuleTester
#5750Overview
The changes I implemented in #5750 were pretty complex to build because I was working around how the base
RuleTester
class did things. I ran into a lot of weirdness due to things like the class expecting specific properties on objects.In looking to implement #6499 - I don't want to hit the same issues, so I think it's best if we just draw a line in the sand and fork it so that we can build our enhancements directly into the tooling.
This is also good because it means that we can provide a new baseline for testing and don't need to worry about what the installed ESLint version supports - which will make our eventual dependency testing easier.
This PR:
RuleTester
afterAll
as additional static "test framework providers" to the class.skip
which runs the test usingit.skip
.afterAll
.tsx
files nowBefore our hands were tied and I had to lean on setting
only
on every test to filter out the skipped tests - now with the newskip
functionality we only need setskip
on these tests which is a much, much nicer implementation!skip
which runs the test case usingit.skip
.itSkip
anddescribeSkip
as additional static "test framework providers" to the class.defaultFilenames
constructor option to allow configuring the default filenames used for type-aware testing.Documentation:
https://deploy-preview-6777--typescript-eslint.netlify.app/architecture/rule-tester
TODO:
RuleTester
#5750migrate codebase to this ruletesterimplement snapshot testing Enhancement: snapshot testing for lint rule failures #6499