-
Notifications
You must be signed in to change notification settings - Fork 243
ts-migration/consistent-test-it #327
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
ts-migration/consistent-test-it #327
Conversation
src/rules/consistent-test-it.ts
Outdated
| }, | ||
| defaultOptions: [ | ||
| { fn: 'test', withinDescribe: 'it' } as { | ||
| fn?: string; |
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.
can we use the enum instead of string?
| const nodeName = getNodeName(node.callee); | ||
|
|
||
| if (!nodeName) { | ||
| return; |
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.
new, but seeing as tests pass I guess it doesn't matter.
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.
It's required by typescript - getNodeName could always return null, but not always checked:
export const getNodeName = node => {
function joinNames(a, b) {
return a && b ? `${a}.${b}` : null;
}
switch (node && node.type) {
case 'Identifier':
return node.name;
case 'MemberExpression':
return joinNames(getNodeName(node.object), getNodeName(node.property));
}
return null;
};
Ah, the joys of complex ASTs 😂
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.
seeing as tests pass I guess it doesn't matter.
I believe this is b/c of the isDescribe - the conditions for which getNodeName would return null are mutually exclusive w/ isDescribe, which is why this has never been a problem.
I mean, that branch must be being hit b/c coverage isn't complaining, so it can happen.
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.
Just so it's written down somewhere, this is the code that can be used to make getNodeName to return null:
[1,2,3].forEach(() => { test("foo") })
It's b/c [1,2,3] is an ArrayExpression, which causes getNodeName to bail out.
I needed to know how to do this to implement a test for another rule to get coverage other the line :)
| }, | ||
| defaultOptions: [ | ||
| { fn: 'test', withinDescribe: 'it' } as { | ||
| fn?: 'it' | 'test'; |
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.
do we need as const on the schema?
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.
might not need the type info here then. Maybe not though, not sure how enum in schema works
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.
Not as far as I'm aware - the schema is json schema, so it's not actually used by TS (it'd be cool if it was, but it's far too complex)
No description provided.