feat(rules): add .concurrent support#498
feat(rules): add .concurrent support#498leonardovillela wants to merge 2 commits intojest-community:masterfrom
Conversation
G-Rath
left a comment
G-Rath
left a comment
There was a problem hiding this comment.
Thanks for this!
I've left details on how to remove all the type casts, which are my main blocker on merging this PR.
I also think isConcurrentExpression can be improved, but am happy to do that myself as I need to break isTestCase & friends up once this is merged anyway (both things you're welcome to attempt yourself, but don't feel you have to if you'd like to move on to other things 😄)
src/rules/no-focused-tests.ts
Outdated
| isSupportedAccessor(callee.property, 'only'); | ||
| isSupportedAccessor( | ||
| isConcurrentExpression(callee) | ||
| ? (callee.parent as TSESTree.MemberExpression).property |
There was a problem hiding this comment.
This cast can be eliminated by making isConcurrentExpression a proper type guard.
src/rules/no-focused-tests.ts
Outdated
| getNodeName(expression) && | ||
| new RegExp( | ||
| `(${validTestCaseNames.join('|')})\.${TestCaseProperty.concurrent}`, | ||
| ).test(getNodeName(expression) as string); |
There was a problem hiding this comment.
This cast can be eliminated by assigning the result of getNodeName to a variable, and then checking that variable, instead of calling getNodeName twice.
src/rules/no-focused-tests.ts
Outdated
|
|
||
| const isConcurrentExpression = (expression: TSESTree.Node) => | ||
| getNodeName(expression) && | ||
| new RegExp( |
There was a problem hiding this comment.
I'm not a big fan of this, as it's hard to understand what the logic will be at runtime - instead, it would be better to use standard conditional checks against the AST structure, like we do with isDescribeEach & co.
| node.callee.property.type === AST_NODE_TYPES.Identifier && | ||
| TestCaseProperty.hasOwnProperty(node.callee.property.name)) | ||
| TestCaseProperty.hasOwnProperty(node.callee.property.name)) || | ||
| isConcurrentTestCase(node) |
There was a problem hiding this comment.
I think it would be better to bring the checks required from isConcurrentTestCase into here, to save on duplicating conditional checks as it should just be a matter of adding a third layer.
Once this PR is merged, I'm going to swing by this method to see about breaking it up a bit and making it more readable.
|
First of all, thanks for the review @G-Rath. I'll do the requested changes today or tomorrow. About the refactor i can try to do this, but in other PR to not block this one, if you get me some guidance ou even open an issue describing your ideias would be nice. But if you prefer to yourself do this refactor and not open an issue describing it, feel free. |
|
@leonardovillela No problem - The refactor definitely doesn't need to be done anytime soon, nor block. It's probably better at this point for me to do it, as I've got to re-review my code anyway to get a handle on the exact changes that need to be done, and it ties in with some other maintenance work I need to do :) I think for now let's focus on getting those other comments addressed, and then we can decide where to go after my review post-changes. |
* feat(rules): add .concurrent support * refactor(no-focused): remove unnecessary type cast * chore(utils): inline `isConcurrentTestCase` * chore(no-focused-tests): use static checks instead of dynamic RegExp * chore(no-focused-tests): add test case for if `concurrent` is called * chore(utils): remove body from arrow functions Co-authored-by: Leonardo Villela <leonardo.villela37@gmail.com>
|
🎉 This issue has been resolved in version 23.3.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
@G-Rath thanks. I wish to contribute more in the future. |
|
@G-Rath do you have a recommend issue that to me take? |
2 participants
Refers to #432
Key points
tstyping, i miss a TSTree definition for.concurrentcall expression. Mainly in two files above.xitdon't support use of.concurrent, so i don't change the rules for it.xtestdon't support use of.concurrent, so i don't change the rules for it..tododon't support use of.concurrent, so i don't change the rules for it.