chore(eslint-plugin-internal): [plugin-test-formatting] support random object literal tests #5895
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. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #5895 +/- ##
==========================================
- Coverage 91.32% 91.29% -0.03%
==========================================
Files 365 365
Lines 12212 12251 +39
Branches 3564 3577 +13
==========================================
+ Hits 11152 11185 +33
- Misses 752 758 +6
Partials 308 308
Flags with carried forward coverage won't be shown. Click here to find out more.
|
…m object literal tests
74828bc to
9d928ca
Compare
| import { RuleTester } from '../RuleTester'; | ||
|
|
||
| const ruleTester = new RuleTester({ | ||
| parser: '@typescript-eslint/parser', | ||
| }); | ||
|
|
||
| const grouped: TSESLint.RunTests<MessageIds, Options> = { | ||
| const grouped: RunTests<MessageIds, Options> = { |
There was a problem hiding this comment.
Why don't we just pass this directly to ruleTester.run? There's only one variable here.
| import { RuleTester } from '../../RuleTester'; | ||
|
|
||
| const ruleTester = new RuleTester({ | ||
| parser: '@typescript-eslint/parser', | ||
| }); | ||
|
|
||
| const sortedCiWithoutGrouping: TSESLint.RunTests<MessageIds, Options> = { | ||
| const sortedCiWithoutGrouping: RunTests<MessageIds, Options> = { |
There was a problem hiding this comment.
Similar suggestion: I like the idea behind describing the variables, but it just seems like extra cruft to me. How about passing them directly to ruleTester.run?
There was a problem hiding this comment.
I'm personally not too miffed either way.
I think the separate variables is nice as a way to document and group the tests.
it's much clearer than the alternative:
ruleTester.run('member-ordering', rule, {
valid: [
//
// Sorted case insensitive without grouping
//
'test1',
// ...
//
// Sorted case insensitive with grouping
//
'testn+1',
// ...
],
});I find it really easy to lose the comments in the array when it gets really big.
But the indirection is a bit of a pain overall.
I think either way has trade-offs, so IDK which one we want as the standard in the repo.
There was a problem hiding this comment.
My mild preference would be for less code, but it's not a hill I'm going to die on 😄
| testProp.computed || | ||
| testProp.key.type !== AST_NODE_TYPES.Identifier || | ||
| testProp.key.name !== 'errors' || | ||
| testProp.value.type !== AST_NODE_TYPES.ArrayExpression |
There was a problem hiding this comment.
Nit: this is almost the same as the previous .type === AST_NODE_TYPES.SpreadElement || ... check, but for the 'errors' case. I wonder if there's a good way to dedup? 🤷 definitely not a blocker IMO.
# Conflicts: # packages/eslint-plugin-internal/package.json
Overview
I was looking at some of our tests and noticed they weren't formatted correctly.
Investigating it was because they were declared as variables instead of as part of a test run.
This PR just adds support to the rule for random object literals by doing some fuzzy checks using types.