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

G-Rath · 2021-03-14T02:05:17Z

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 · 2021-03-14T02:06:54Z

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

G-Rath · 2021-03-14T02:10:26Z

src/rules/utils.ts

+    return node.callee.tag.object.type === AST_NODE_TYPES.MemberExpression
+      ? isTestCaseName(node.callee.tag.object.object)
+      : isTestCaseName(node.callee.tag.object);


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 · 2021-03-14T02:13:34Z

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``())

SimenB

great stuff 👍

SimenB · 2021-03-14T08:13:09Z

src/rules/utils.ts

+    // 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 &&


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

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.

SimenB · 2021-03-14T08:14:39Z

src/rules/__tests__/utils.test.ts

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


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

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.

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 · 2021-03-16T20:05:54Z

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 · 2021-03-16T20:57:38Z

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 · 2021-03-21T01:55:05Z

#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 · 2021-03-22T07:20:20Z

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 · 2021-04-05T01:34:52Z

@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 :)

## [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 · 2021-04-05T01:46:12Z

🎉 This PR is included in version 24.3.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

G-Rath requested a review from SimenB March 14, 2021 02:05

G-Rath commented Mar 14, 2021

View reviewed changes

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

SimenB approved these changes Mar 14, 2021

View reviewed changes

G-Rath force-pushed the refactor-utils branch from 96fd19a to c46f28e Compare March 16, 2021 20:08

G-Rath mentioned this pull request Mar 21, 2021

fix(prefer-expect-assertions): support .each #798

Merged

G-Rath force-pushed the refactor-utils branch 2 times, most recently from f0d9460 to 9ce4e24 Compare April 3, 2021 21:10

This was referenced Apr 4, 2021

no-conditional-expect does not handle .each()() #809

Closed

no-if does not handle .each()() #810

Closed

G-Rath added bug each support Relates to supporting the `each` method labels Apr 4, 2021

G-Rath mentioned this pull request Apr 4, 2021

no-test-return-statement does not handle .each()() #811

Closed

G-Rath force-pushed the refactor-utils branch from 9ce4e24 to 223e6a2 Compare April 4, 2021 22:39

G-Rath added 9 commits April 5, 2021 10:45

refactor(utils): support matching all variations of describe & test

16b507a

test(consistent-test-it): add case for describe.only.each

fe43744

test(require-top-level-describe): add cases for modifiers and each

915e3ff

test(no-conditional-expect): add cases for .each with template literal

4ba6271

test(no-export): add cases for .each

3444b5b

test(no-if): add cases for .each calls

8984a1d

test(no-test-return-statement): add cases for .each

e113082

test(prefer-hooks-on-top): add cases for .each

484de05

test(valid-title): add cases for .each

5803d4b

G-Rath force-pushed the refactor-utils branch from 223e6a2 to 5803d4b Compare April 4, 2021 22:46

test(utils): deduplicate code

6211dbb

G-Rath marked this pull request as ready for review April 5, 2021 01:18

test(utils): refactor to deduplicate code

4ced9af

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 merged commit 0968b55 into main Apr 5, 2021

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

chore(release): 24.3.4 [skip ci]

3b8c36c

## [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 bot added the released label Apr 5, 2021

G-Rath mentioned this pull request Apr 5, 2021

fix: improve handling of .each calls and with tagged literals #814

Merged

Obi-Dann mentioned this pull request Apr 10, 2021

Test Upgrade all dependencies AurorNZ/report-updated-dependencies#7

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

G-Rath commented Mar 14, 2021

G-Rath commented Mar 14, 2021

G-Rath Mar 14, 2021 •

edited

Loading

G-Rath commented Mar 14, 2021 •

edited

Loading

SimenB left a comment

SimenB Mar 14, 2021

G-Rath Mar 16, 2021

SimenB Mar 14, 2021

G-Rath Mar 16, 2021

G-Rath Apr 5, 2021

G-Rath commented Mar 16, 2021

SimenB commented Mar 16, 2021

G-Rath commented Mar 21, 2021

SimenB commented Mar 22, 2021

G-Rath commented Apr 5, 2021

github-actions bot commented Apr 5, 2021

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

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

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

Conversation

G-Rath commented Mar 14, 2021

G-Rath commented Mar 14, 2021

G-Rath Mar 14, 2021 • edited Loading

Choose a reason for hiding this comment

G-Rath commented Mar 14, 2021 • edited Loading

SimenB left a comment

Choose a reason for hiding this comment

SimenB Mar 14, 2021

Choose a reason for hiding this comment

G-Rath Mar 16, 2021

Choose a reason for hiding this comment

SimenB Mar 14, 2021

Choose a reason for hiding this comment

G-Rath Mar 16, 2021

Choose a reason for hiding this comment

G-Rath Apr 5, 2021

Choose a reason for hiding this comment

G-Rath commented Mar 16, 2021

SimenB commented Mar 16, 2021

G-Rath commented Mar 21, 2021

SimenB commented Mar 22, 2021

G-Rath commented Apr 5, 2021

github-actions bot commented Apr 5, 2021

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

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

G-Rath Mar 14, 2021 •

edited

Loading

G-Rath commented Mar 14, 2021 •

edited

Loading