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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 21 commits into from Jul 27, 2020
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
118351c
feat(rules): add no-interpolation-inline-snapshot rule
viestat Apr 2, 2020
d96ae67
docs: fix fromat in rule documentation
viestat Apr 2, 2020
b6c50ec
fix: add chamges requested in review
viestat May 4, 2020
ba43b09
Merge branch 'master' of github.com:jest-community/eslint-plugin-jest…
viestat May 4, 2020
d8872c2
test: update number of rules
viestat May 4, 2020
ea5644e
docs: update docs/rules/no-interpolation-inline-snapshot.md
viestat Jun 29, 2020
7dc3bd7
feat: add toThrowErrorMatchingInlineSnapshot
viestat Jun 29, 2020
3579ef1
Merge branch 'master' of github.com:jest-community/eslint-plugin-jest…
viestat Jun 29, 2020
d83c060
docs: update rules table in README
viestat Jun 29, 2020
8c448e9
docs: apply suggestions from code review
viestat Jul 1, 2020
825687e
docs: fix format in README
viestat Jul 1, 2020
96eaf9b
fix: add review suggestions
viestat Jul 1, 2020
a405636
docs: add examples to README
viestat Jul 1, 2020
3dfd3a6
chore: rename rule to no-interpolation-in-snapshots
viestat Jul 1, 2020
ab2c365
chore: remove changes from CHANGELOG.md
viestat Jul 1, 2020
3fcf261
docs: update rule table with new name
viestat Jul 1, 2020
3738c2d
fix: apply suggestions from code review
viestat Jul 6, 2020
f98accf
test: add test to cover missing branch
viestat Jul 6, 2020
e9e2eed
Merge branch 'master' of github.com:jest-community/eslint-plugin-jest…
viestat Jul 27, 2020
032c8bc
docs: regenerate table with the new script
viestat Jul 27, 2020
6029742
fix: edit rule description
viestat Jul 27, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
50 changes: 50 additions & 0 deletions docs/rules/no-interpolation-inline-snapshot.md
@@ -0,0 +1,50 @@
# No string interpolation inside inline snapshots (no-interpolation-inline-snapshot)
viestat marked this conversation as resolved.
Show resolved Hide resolved

Prevents the use of string interpolations within `toMatchInlineSnapshot`.
viestat marked this conversation as resolved.
Show resolved Hide resolved

## Rule Details

Interpolation prevents snapshots from being updated. Instead, properties should
be overloaded with a matcher by passing the optional `propertyMatchers` argument
to `toMatchInlineSnapshot`.
viestat marked this conversation as resolved.
Show resolved Hide resolved

Examples of **incorrect** code for this rule:
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added in a405636


```js
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:

```js
expect(something).toMatchInlineSnapshot();

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

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

## When Not To Use It

Don't use this rule on non-jest test files.
1 change: 1 addition & 0 deletions src/__tests__/__snapshots__/rules.test.ts.snap
Expand Up @@ -24,6 +24,7 @@ Object {
"jest/no-hooks": "error",
"jest/no-identical-title": "error",
"jest/no-if": "error",
"jest/no-interpolation-inline-snapshot": "error",
"jest/no-jasmine-globals": "error",
"jest/no-jest-import": "error",
"jest/no-large-snapshots": "error",
Expand Down
2 changes: 1 addition & 1 deletion src/__tests__/rules.test.ts
Expand Up @@ -3,7 +3,7 @@ import { resolve } from 'path';
import plugin from '../';

const ruleNames = Object.keys(plugin.rules);
const numberOfRules = 41;
const numberOfRules = 42;

describe('rules', () => {
it('should have a corresponding doc for each rule', () => {
Expand Down
66 changes: 66 additions & 0 deletions src/rules/__tests__/no-interpolation-inline-snapshot.test.ts
@@ -0,0 +1,66 @@
import { TSESLint } from '@typescript-eslint/experimental-utils';
import resolveFrom from 'resolve-from';
import rule from '../no-interpolation-inline-snapshot';

const ruleTester = new TSESLint.RuleTester({
parser: resolveFrom(require.resolve('eslint'), 'espree'),
parserOptions: {
ecmaVersion: 2017,
},
});

ruleTester.run('no-interpolation-inline-snapshot', rule, {
valid: [
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added in b6c50ec

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added in 96eaf9b

'expect(something).toMatchInlineSnapshot();',
'expect(something).toMatchInlineSnapshot(`No interpolation`);',
'expect(something).toMatchInlineSnapshot({}, `No interpolation`);',
'expect(something);',
'expect(something).not;',
'expect.toHaveAssertions();',
'myObjectWants.toMatchInlineSnapshot({}, `${interpolated}`);',
'toMatchInlineSnapshot({}, `${interpolated}`);',
],
invalid: [
{
code: 'expect(something).toMatchInlineSnapshot(`${interpolated}`);',
errors: [
{
endColumn: 58,
column: 41,
messageId: 'noInterpolation',
},
],
},
{
code: 'expect(something).not.toMatchInlineSnapshot(`${interpolated}`);',
errors: [
{
endColumn: 62,
column: 45,
messageId: 'noInterpolation',
},
],
},
{
code: 'expect(something).toMatchInlineSnapshot({}, `${interpolated}`);',
errors: [
{
endColumn: 62,
column: 45,
messageId: 'noInterpolation',
},
],
},
{
code:
'expect(something).not.toMatchInlineSnapshot({}, `${interpolated}`);',
errors: [
{
endColumn: 66,
column: 49,
messageId: 'noInterpolation',
},
],
},
],
});
51 changes: 51 additions & 0 deletions src/rules/no-interpolation-inline-snapshot.ts
@@ -0,0 +1,51 @@
import { AST_NODE_TYPES } from '@typescript-eslint/experimental-utils';
import { createRule, isExpectCall, parseExpectCall } from './utils';

export default createRule({
name: __filename,
meta: {
docs: {
category: 'Best Practices',
description: 'Disallow string interpolation inside inline snapshots.',
recommended: false,
},
messages: {
noInterpolation:
'Do not use string interpolation inside of inline snapshots',
},
schema: [],
type: 'suggestion',
},
defaultOptions: [],
create(context) {
return {
CallExpression(node) {
if (!isExpectCall(node)) {
return;
}

const { matcher } = parseExpectCall(node);

if (
G-Rath marked this conversation as resolved.
Show resolved Hide resolved
matcher?.name !== 'toMatchInlineSnapshot' ||
matcher?.arguments === null
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@viestat viestat Jun 29, 2020

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed in 96eaf9b

) {
return;
}

// Check all since the optional 'propertyMatchers' argument might be present
matcher.arguments.forEach(argument => {
if (
argument.type === AST_NODE_TYPES.TemplateLiteral &&
argument.expressions.length > 0
) {
context.report({
messageId: 'noInterpolation',
node: argument,
});
}
});
},
};
},
});