feat: create `no-interpolation-in-snapshots` rule #553

viestat · 2020-04-02T15:16:12Z

Motivation

This rule disallows the use of string interpolations within toMatchInlineSnapshot since interpolation prevents snapshots from being updated.

Instead, properties should be overloaded with a matcher by passing the optional propertyMatchers argument to toMatchInlineSnapshot.

Examples of incorrect code for this rule:

expect(something).toMatchInlineSnapshot(
  `Object {
    property: ${interpolated}
  }`
);

expect(something).toMatchInlineSnapshot({ other: expect.any(Number) }, 
  `Object {
    other: Any<Number>,
    property: ${interpolated}
  }`
);

Examples of correct code for this rule:

expect(something).toMatchInlineSnapshot();

expect(something).toMatchInlineSnapshot(
  `Object {
    property: 1
  }`
);

expect(something).toMatchInlineSnapshot({ property: expect.any(Date) }, 
  `Object {
    property: Any<Date>
  }`
);

G-Rath

I've given this a brief look over and requested a few changes :)

src/rules/no-interpolation-inline-snapshot.ts

G-Rath · 2020-04-13T03:31:27Z

src/rules/__tests__/no-interpolation-inline-snapshot.test.ts

+});
+
+ruleTester.run('no-interpolation-inline-snapshot', rule, {
+  valid: [


Could you add some more tests for cases like:

when expect is called without a matcher or modifier

when expect is called with a dangling modifier (i.e no matcher)

when a property is accessed on expect (i.e expect.toHaveAssertions()

when a method called toMatchInlineSnapshot is called with interpolation, both by itself and on an object that looks suspiciously similar to expect

added in b6c50ec

Awesome, thanks!

If you could check one more in that has two or more interpolations, just for completeness, that would be cool but not blocking.

added in 96eaf9b

… into no-interpolation-inline-snapshot

viestat · 2020-05-04T12:01:25Z

Thanks for the feedback!

G-Rath

Sorry for the delay; left a few comments.

This rule should also check the toThrowErrorMatchingInlineSnapshot matcher as well :)

Also don't forget to regenerate the README rules table (you can do this by running yarn run tools:generate-rules-table)

docs/rules/no-interpolation-inline-snapshot.md

G-Rath · 2020-06-24T21:26:08Z

src/rules/no-interpolation-inline-snapshot.ts

+
+        if (
+          matcher?.name !== 'toMatchInlineSnapshot' ||
+          matcher?.arguments === null


I think you might as well just use optional chaining on the forEach below, since matcher?.name !== will also check that matcher is defined

Thank you for your review!
I am not sure I understand what you mean here.

Optional chaining is cool, but if you use it multiple times in a row on the same object, that's usually a smell that you should just check for existence.

When using optional chaining in a condition, it has the same guarding as doing "obj && obj.prop ", meaning it does a "is defined" check.

For example this:

if(node.parent && node.parent.type === AST_NODE_TYPES.AssignmentExpression) { const { right } = node.parent; ... }

Can be simplified to just:

if(node.parent?.type === AST_NODE_TYPES.AssignmentExpression) { const { right } = node.parent; ... }

So originally what I was saying when your code was just matcher?.name === 'value' was that you could drop the matcher.arguments === null in favor of just later doing matcher.arguments?.forEach.

However, now that the code has changed to include multiple compares to name, for which I'd recommend using include, and so in turn just do a matcher && as matcher?.name returns type string | undefined which is not accepted by string[].includes (as that expects just string).

tl;dr

I would replace the multiple matcher?.name ===s with just matcher && ['toMatchInlineSnapshot', 'toThrowErrorMatchingInlineSnapshot'].includes(matcher.name)

I would replace matcher.arguments === null with matcher.arguments?.forEach

changed in 96eaf9b

Co-authored-by: Gareth Jones <[email protected]>

… into no-interpolation-inline-snapshot

G-Rath

This is looking really good! I'm planning on including this rule in the default ruleset in our next major, as it's always an error.

Aside from the few changes I've requested my only other nit is I think no-interpolation-in-snapshots might be a bit more clear as a name and it's technically true for both types of snapshots (and it's a little shorter which is nicer too).

G-Rath · 2020-06-29T20:43:34Z

src/rules/no-interpolation-inline-snapshot.ts

+
+        if (
+          matcher?.name !== 'toMatchInlineSnapshot' ||
+          matcher?.arguments === null


Optional chaining is cool, but if you use it multiple times in a row on the same object, that's usually a smell that you should just check for existence.

When using optional chaining in a condition, it has the same guarding as doing "obj && obj.prop ", meaning it does a "is defined" check.

For example this:

if(node.parent && node.parent.type === AST_NODE_TYPES.AssignmentExpression) { const { right } = node.parent; ... }

Can be simplified to just:

if(node.parent?.type === AST_NODE_TYPES.AssignmentExpression) { const { right } = node.parent; ... }

So originally what I was saying when your code was just matcher?.name === 'value' was that you could drop the matcher.arguments === null in favor of just later doing matcher.arguments?.forEach.

However, now that the code has changed to include multiple compares to name, for which I'd recommend using include, and so in turn just do a matcher && as matcher?.name returns type string | undefined which is not accepted by string[].includes (as that expects just string).

tl;dr

I would replace the multiple matcher?.name ===s with just matcher && ['toMatchInlineSnapshot', 'toThrowErrorMatchingInlineSnapshot'].includes(matcher.name)

I would replace matcher.arguments === null with matcher.arguments?.forEach

G-Rath · 2020-06-29T20:46:29Z

src/rules/__tests__/no-interpolation-inline-snapshot.test.ts

+});
+
+ruleTester.run('no-interpolation-inline-snapshot', rule, {
+  valid: [


Awesome, thanks!

If you could check one more in that has two or more interpolations, just for completeness, that would be cool but not blocking.

G-Rath · 2020-06-29T20:50:06Z

CHANGELOG.md

@@ -1,96 +1,111 @@
 ## [23.17.1](https://github.com/jest-community/eslint-plugin-jest/compare/v23.17.0...v23.17.1) (2020-06-23)


This file is automatically generated, so should not be formatted with prettier as it'll get overridden the next time a release happens :)

This was edited by husky commit tasks when I merged the latest changes with master 😓
Removed them with ab2c365

G-Rath

A couple of doc nits :)

docs/rules/no-interpolation-inline-snapshot.md

G-Rath · 2020-06-29T21:03:46Z

docs/rules/no-interpolation-inline-snapshot.md

+be overloaded with a matcher by passing the optional `propertyMatchers` argument
+to `toMatchInlineSnapshot`.
+
+Examples of **incorrect** code for this rule:


Could you chuck a in/correct example for toThrowErrorMatchingInlineSnapshot as well?

added in a405636

Co-authored-by: Gareth Jones <[email protected]>

src/rules/no-interpolation-in-snapshots.ts

docs/rules/no-interpolation-in-snapshots.md

Co-authored-by: Gareth Jones <[email protected]> Co-authored-by: Simen Bekkhus <[email protected]>

G-Rath

This is looking good - the only thing left is that the description needs to be synced up so that it says "Disallow string interpolation inside snapshots" everywhere.

I've refactored our generate-rules-table tool in #624 to handle this based on the description property in the actual rules, so feel free to wait until that's been merged, or pull the script in locally and run it :)

… into no-interpolation-inline-snapshot

G-Rath

✨

# [23.19.0](v23.18.2...v23.19.0) (2020-07-27) ### Features * create `no-interpolation-in-snapshots` rule ([#553](#553)) ([8d2c17c](8d2c17c))

github-actions · 2020-07-27T19:59:01Z

🎉 This PR is included in version 23.19.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

viestat added 2 commits April 2, 2020 17:12

feat(rules): add no-interpolation-inline-snapshot rule

118351c

docs: fix fromat in rule documentation

d96ae67

G-Rath requested changes Apr 13, 2020

View reviewed changes

viestat added 3 commits May 4, 2020 13:59

fix: add chamges requested in review

b6c50ec

Merge branch 'master' of github.com:jest-community/eslint-plugin-jest…

ba43b09

… into no-interpolation-inline-snapshot

test: update number of rules

d8872c2

viestat requested a review from G-Rath May 4, 2020 12:00

G-Rath requested changes Jun 24, 2020

View reviewed changes

viestat and others added 4 commits June 29, 2020 11:06

docs: update docs/rules/no-interpolation-inline-snapshot.md

ea5644e

Co-authored-by: Gareth Jones <[email protected]>

feat: add toThrowErrorMatchingInlineSnapshot

7dc3bd7

Merge branch 'master' of github.com:jest-community/eslint-plugin-jest…

3579ef1

… into no-interpolation-inline-snapshot

docs: update rules table in README

d83c060

viestat force-pushed the no-interpolation-inline-snapshot branch from 0ba9265 to d83c060 Compare June 29, 2020 12:04

viestat requested a review from G-Rath June 29, 2020 12:14

G-Rath requested changes Jun 29, 2020

View reviewed changes

docs: apply suggestions from code review

8c448e9

Co-authored-by: Gareth Jones <[email protected]>

viestat force-pushed the no-interpolation-inline-snapshot branch from 6ae26e8 to 8c448e9 Compare July 1, 2020 09:09

viestat added 6 commits July 1, 2020 11:12

docs: fix format in README

825687e

fix: add review suggestions

96eaf9b

docs: add examples to README

a405636

chore: rename rule to no-interpolation-in-snapshots

3dfd3a6

chore: remove changes from CHANGELOG.md

ab2c365

docs: update rule table with new name

3fcf261

viestat requested a review from G-Rath July 1, 2020 09:55

SimenB approved these changes Jul 4, 2020

View reviewed changes

src/rules/no-interpolation-in-snapshots.ts Outdated Show resolved Hide resolved

src/rules/no-interpolation-in-snapshots.ts Outdated Show resolved Hide resolved

src/rules/no-interpolation-in-snapshots.ts Outdated Show resolved Hide resolved

G-Rath approved these changes Jul 5, 2020

View reviewed changes

G-Rath reviewed Jul 5, 2020

View reviewed changes

docs/rules/no-interpolation-in-snapshots.md Outdated Show resolved Hide resolved

G-Rath changed the title ~~feat(rules): add no-interpolation-inline-snapshot rule~~ feat: create no-interpolation-in-snapshots rule Jul 5, 2020

viestat and others added 2 commits July 6, 2020 10:58

fix: apply suggestions from code review

3738c2d

Co-authored-by: Gareth Jones <[email protected]> Co-authored-by: Simen Bekkhus <[email protected]>

test: add test to cover missing branch

f98accf

G-Rath requested changes Jul 15, 2020

View reviewed changes

Merge branch 'master' of github.com:jest-community/eslint-plugin-jest…

e9e2eed

… into no-interpolation-inline-snapshot

viestat force-pushed the no-interpolation-inline-snapshot branch from 002af65 to e9e2eed Compare July 27, 2020 08:20

viestat added 2 commits July 27, 2020 10:22

docs: regenerate table with the new script

032c8bc

fix: edit rule description

6029742

viestat force-pushed the no-interpolation-inline-snapshot branch from a795d72 to 6029742 Compare July 27, 2020 08:40

SimenB requested a review from G-Rath July 27, 2020 09:25

G-Rath approved these changes Jul 27, 2020

View reviewed changes

G-Rath merged commit 8d2c17c into jest-community:master Jul 27, 2020

github-actions bot pushed a commit that referenced this pull request Jul 27, 2020

chore(release): 23.19.0 [skip ci]

9e2d4ff

# [23.19.0](v23.18.2...v23.19.0) (2020-07-27) ### Features * create `no-interpolation-in-snapshots` rule ([#553](#553)) ([8d2c17c](8d2c17c))

github-actions bot added the released label Jul 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: create `no-interpolation-in-snapshots` rule #553

feat: create `no-interpolation-in-snapshots` rule #553

viestat commented Apr 2, 2020

G-Rath left a comment

G-Rath Apr 13, 2020

viestat May 4, 2020

G-Rath Jun 29, 2020

viestat Jul 1, 2020

viestat commented May 4, 2020

G-Rath left a comment •

edited

Loading

G-Rath Jun 24, 2020

viestat Jun 29, 2020 •

edited

Loading

G-Rath Jun 29, 2020

viestat Jul 1, 2020

G-Rath left a comment

G-Rath Jun 29, 2020

G-Rath Jun 29, 2020

G-Rath Jun 29, 2020

viestat Jul 1, 2020

G-Rath left a comment

G-Rath Jun 29, 2020

viestat Jul 1, 2020

G-Rath left a comment

G-Rath left a comment

github-actions bot commented Jul 27, 2020

		@@ -1,96 +1,111 @@
		## [23.17.1](https://github.com/jest-community/eslint-plugin-jest/compare/v23.17.0...v23.17.1) (2020-06-23)

feat: create no-interpolation-in-snapshots rule #553

feat: create no-interpolation-in-snapshots rule #553

Conversation

viestat commented Apr 2, 2020

Motivation

G-Rath left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viestat commented May 4, 2020

G-Rath left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viestat Jun 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

G-Rath left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

G-Rath left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

G-Rath left a comment

Choose a reason for hiding this comment

G-Rath left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 27, 2020

feat: create `no-interpolation-in-snapshots` rule #553

feat: create `no-interpolation-in-snapshots` rule #553

G-Rath left a comment •

edited

Loading

viestat Jun 29, 2020 •

edited

Loading