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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: support all variations of describe, it, & test #792

Merged
merged 11 commits into from Apr 5, 2021
Merged

Conversation

G-Rath
Copy link
Collaborator

@G-Rath G-Rath commented Mar 14, 2021

We've had a bit of flurry of activity in the last few weeks which has resulted in some of our technical debt coming to the surface, so this is (hopefully) the start of me addressing some of that.

There are two main goals with this refactor:

  • we're consistent by only matching if the given node is a call i.e it'll result in jest doing "something"
    • This is primarily important for .each, because of.each() - while that looks like a call (and technically is), it's not the "right" kind of call we're wanting to match with these methods.
  • we're matching all possible variations both in terms of properties and accessor style (dot property & square brackets)

Because in a perfect world I'd like this to be the last time I have to think twice about this when using these methods, I've implemented a set of tests using an inline eslint rule to ensure we're matching every possible variation of calls and not-calls for these methods.

The end result was lowercase-name losing a point of cover due to not having a test that covers when the names are not strings, and a few "valid" cases for valid-describe failing as they're now actually supported (馃帀).

Because I'd like to land this as a refactor commit by itself for now, I've renamed the methods so that I can keep the old isDescribe around for valid-describe to use - I'll be refactoring it once this is landed to support the new guards.

It's also technically a more accurate name anyway 馃し


Because of the interlocking nature of things, I've picked this as a starting point and deliberately ignored other possible refactors as I think it'll be premature to do them in one lump go - this is why I'm not doing any deduplication on the methods (since they now actually share pretty much the exact same implementation) nor touched the types or other util functions.

My goal is to have this as a stable starting point that I can build other refactors & fixes up around :)

@G-Rath G-Rath requested a review from SimenB March 14, 2021 02:05
@G-Rath
Copy link
Collaborator Author

G-Rath commented Mar 14, 2021

(Have made this a draft PR for now, but tbh it's ready to go)

Comment on lines 707 to 709
return node.callee.tag.object.type === AST_NODE_TYPES.MemberExpression
? isTestCaseName(node.callee.tag.object.object)
: isTestCaseName(node.callee.tag.object);
Copy link
Collaborator Author

@G-Rath G-Rath Mar 14, 2021

Choose a reason for hiding this comment

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

I'm sure there's a way to restructure this so we don't have to duplicate the condition in two different branches, but can't figure it out.

I'm also sure that the same restructure will be usable in no-focused-tests (as it has pretty much the exact same conditional structure)

@G-Rath
Copy link
Collaborator Author

G-Rath commented Mar 14, 2021

Just realised I should have tests covering not supported methods, i.e test.items()(), describe.every``(), etc

(as I expected: it fails on describe.every``())

@G-Rath G-Rath force-pushed the refactor-utils branch 2 times, most recently from 9911ff8 to 96fd19a Compare March 14, 2021 03:00
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

great stuff 馃憤

// if we're an `each()`, ensure we're being called (i.e `.each()()`)
if (
node.callee.type !== AST_NODE_TYPES.TaggedTemplateExpression &&
node.parent?.type !== AST_NODE_TYPES.CallExpression &&
Copy link
Member

Choose a reason for hiding this comment

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

there's no issue with ? potentially making this undefined at the check true?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It shouldnt no - parent currently never can actually be undefined anyway, it's just marked as such because of TypeScript reasons.

There's typescript-eslint/typescript-eslint#1417 which tracks wanting to improve the typings for parent.

@@ -0,0 +1,424 @@
import { TSESLint } from '@typescript-eslint/experimental-utils';
Copy link
Member

Choose a reason for hiding this comment

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

great idea to have this test 馃憤 My only "complaint" is that it will potentially not help us discover dead code, but probably worth it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we should still be able to find deadcode, just not in these methods. I plan to split the utils up into their own files under utils/* at some point, which'll also help.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

iirc jest doesn't support any kind of "don't collect coverage from files/tests" ay?

Thinking about it, we should be able to rig CI to run this test specifically in its own job, and skip it when running the coverage-collecting tests

I had a think about this, and think we should be able to do something in CI to cover that?

@G-Rath
Copy link
Collaborator Author

G-Rath commented Mar 16, 2021

I think I might make this a fix instead of a refactor, and extend the scope a bit to include adjusting valid-describe.

There's already a few rules this technically fixes their support for .each implicitly, but I've just confirmed explicitly that this fixes consistent-test-it.

@SimenB
Copy link
Member

SimenB commented Mar 16, 2021

There's already a few rules this technically fixes their support for .each implicitly, but I've just confirmed explicitly that this fixes consistent-test-it.

adding a few tests to the other rules to verify would be sweet

@G-Rath
Copy link
Collaborator Author

G-Rath commented Mar 21, 2021

#798 made me realise this doesn't address a big inconsistency that's not highlighted by the tests: the CallExpression that's matched for .each()() is not the same as the one matches for all the other cases, including .each``().

i.e with a rule that has a create like this:

create: context => ({
  CallExpression(node) {
    if (isTestCaseCall(node)) {
      console.log(node.arguments.length, getNodeName(node));
    }
  },
}),

with this source code:

it('is true, () => {});
it.only('is true', () => {});
it.each()('is true', () => {});
it.each``('is true', () => {});

You would get this output:

2 it
2 it.only
0 it.each
2 it.each

This is because .each()() actually is two call expressions, and our handling works from the "outer" CallExpression, which is .each().

This is a really subtle difference that I really want to get rid of because it's something I don't want to have to think about - I think the better behaviour would be to match .each()() as the CallExpression we want - that should make it simpler in most situations when working with .each as it should mean you don't have to do const args = isEachCall(node) ? node.parent.args : node.args.

I think it should be straightforward to implement this behaviour, but will look to do that in a follow PR as I think this is nice by its own, and I could also be wrong 馃し

@SimenB
Copy link
Member

SimenB commented Mar 22, 2021

I think it should be straightforward to implement this behaviour, but will look to do that in a follow PR as I think this is nice by its own, and I could also be wrong 馃し

馃憤

@G-Rath G-Rath marked this pull request as ready for review April 5, 2021 01:18
@G-Rath
Copy link
Collaborator Author

G-Rath commented Apr 5, 2021

@SimenB I think this is good to go - I've been through most of the rules to find which ones are improved by this refactor, and added a couple of test cases to each that fail with the old guards.

I've also refactored the utils test to have less duplication and be more unit-y - we can always make new tests if we come up with specific cases to test.

Part of the final refactor is also because I've written a patch for including testing getNodeName, as well as having implemented this change, which I'll add in follow up PRs.

I'll merge this in a few minutes since I'd like to move on with more refactoring, but happy to discuss and revert/make changes if you've got any review comments :)

@G-Rath G-Rath changed the title refactor(utils): support matching all variations of describe & test fix: support all variations of describe, it, & test Apr 5, 2021
@G-Rath G-Rath merged commit 0968b55 into main Apr 5, 2021
@G-Rath G-Rath deleted the refactor-utils branch April 5, 2021 01:42
github-actions bot pushed a commit that referenced this pull request Apr 5, 2021
## [24.3.4](v24.3.3...v24.3.4) (2021-04-05)

### Bug Fixes

* support all variations of `describe`, `it`, & `test` ([#792](#792)) ([0968b55](0968b55))
@github-actions
Copy link

github-actions bot commented Apr 5, 2021

馃帀 This PR is included in version 24.3.4 馃帀

The release is available on:

Your semantic-release bot 馃摝馃殌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug each support Relates to supporting the `each` method released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants