-
Notifications
You must be signed in to change notification settings - Fork 243
Adds no-large-snapshots rule #24
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
Changes from all commits
631b7b5
f4ccc37
3990217
8267cfa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,111 @@ | ||
| # disallow large snapshots (no-large-snapshots) | ||
|
|
||
| When using Jest's snapshot capability one should be mindful of the size of | ||
| created snapshots. As a best practice snapshots should be limited in size in | ||
| order to be more manageable and reviewable. A stored snapshot is only as good as | ||
| its review and as such keeping it short, sweet, and readable is important to | ||
| allow for thorough reviews. | ||
|
|
||
| ## Usage | ||
|
|
||
| Because Jest snapshots are written with back-ticks (\` \`) which are only valid | ||
| with | ||
| [ES2015 onwards](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals) | ||
| you should set `parserOptions` in your config to at least allow ES2015 in order | ||
| to use this rule: | ||
|
|
||
| ```js | ||
| parserOptions: { | ||
| ecmaVersion: 2015, | ||
| }, | ||
| ``` | ||
|
|
||
| ## Rule Details | ||
|
|
||
| This rule looks at all Jest snapshot files (files with `.snap` extension) and | ||
| validates that each stored snapshot within those files does not exceed 50 lines | ||
| (by default, this is configurable as explained in `Options` section below). | ||
|
|
||
| Example of **incorrect** code for this rule: | ||
|
|
||
| ```js | ||
| exports[`a large snapshot 1`] = ` | ||
| line 1 | ||
| line 2 | ||
| line 3 | ||
| line 4 | ||
| line 5 | ||
| line 6 | ||
| line 7 | ||
| line 8 | ||
| line 9 | ||
| line 10 | ||
| line 11 | ||
| line 12 | ||
| line 13 | ||
| line 14 | ||
| line 15 | ||
| line 16 | ||
| line 17 | ||
| line 18 | ||
| line 19 | ||
| line 20 | ||
| line 21 | ||
| line 22 | ||
| line 23 | ||
| line 24 | ||
| line 25 | ||
| line 26 | ||
| line 27 | ||
| line 28 | ||
| line 29 | ||
| line 30 | ||
| line 31 | ||
| line 32 | ||
| line 33 | ||
| line 34 | ||
| line 35 | ||
| line 36 | ||
| line 37 | ||
| line 38 | ||
| line 39 | ||
| line 40 | ||
| line 41 | ||
| line 42 | ||
| line 43 | ||
| line 44 | ||
| line 45 | ||
| line 46 | ||
| line 47 | ||
| line 48 | ||
| line 49 | ||
| line 50 | ||
| line 51 | ||
| `; | ||
| ``` | ||
|
|
||
| Example of **correct** code for this rule: | ||
|
|
||
| ```js | ||
| exports[`a more manageable and readable snapshot 1`] = ` | ||
| line 1 | ||
| line 2 | ||
| line 3 | ||
| line 4 | ||
| `; | ||
| ``` | ||
|
|
||
| ## Options | ||
|
|
||
| This rule has option for modifying the max number of lines allowed for a | ||
| snapshot: | ||
|
|
||
| In an `eslintrc` file: | ||
|
|
||
| ```json | ||
| ... | ||
| "rules": { | ||
| "jest/no-large-snapshots": ["warn", { "maxSize": 12 }] | ||
| } | ||
| ... | ||
| ``` |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,7 +10,7 @@ | |
| "email": "[email protected]", | ||
| "url": "jkimbo.com" | ||
| }, | ||
| "files": ["docs/", "rules/", "index.js"], | ||
| "files": ["docs/", "rules/", "processors/", "index.js"], | ||
| "peerDependencies": { | ||
| "eslint": ">=3.6" | ||
| }, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| 'use strict'; | ||
|
|
||
| const snapshotProcessor = require('../snapshot-processor'); | ||
|
|
||
| describe('snapshot-processor', () => { | ||
| it('exports an object with preprocess and postprocess functions', () => { | ||
| expect(snapshotProcessor).toMatchObject({ | ||
| preprocess: expect.any(Function), | ||
| postprocess: expect.any(Function), | ||
| }); | ||
| }); | ||
|
|
||
| describe('preprocess function', () => { | ||
| it('should pass on untouched source code to source array', () => { | ||
| const preprocess = snapshotProcessor.preprocess; | ||
| const sourceCode = "const name = 'johnny bravo';"; | ||
| const result = preprocess(sourceCode); | ||
|
|
||
| expect(result).toEqual([sourceCode]); | ||
| }); | ||
| }); | ||
|
|
||
| describe('postprocess function', () => { | ||
| it('should only return messages about snapshot specific rules', () => { | ||
| const postprocess = snapshotProcessor.postprocess; | ||
| const result = postprocess([ | ||
| [ | ||
| { ruleId: 'no-console' }, | ||
| { ruleId: 'global-require' }, | ||
| { ruleId: 'jest/no-large-snapshots' }, | ||
| ], | ||
| ]); | ||
|
|
||
| expect(result).toEqual([{ ruleId: 'jest/no-large-snapshots' }]); | ||
| }); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| 'use strict'; | ||
|
|
||
| // https://eslint.org/docs/developer-guide/working-with-plugins#processors-in-plugins | ||
| const preprocess = source => [source]; | ||
|
|
||
| const postprocess = messages => | ||
| messages[0].filter( | ||
| // snapshot files should only be linted with snapshot specific rules | ||
| message => message.ruleId === 'jest/no-large-snapshots' | ||
| ); | ||
|
|
||
| module.exports = { | ||
| preprocess, | ||
| postprocess, | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| // Jest Snapshot v1, https://goo.gl/fbAQLP | ||
|
|
||
| exports[`no-large-snapshots ExpressionStatement function should report if node has more lines of code than number given in sizeThreshold option 1`] = ` | ||
| Array [ | ||
| Object { | ||
| "data": Object { | ||
| "lineCount": 83, | ||
| "lineLimit": 70, | ||
| }, | ||
| "message": "Expected Jest snapshot to be smaller than {{ lineLimit }} lines but was {{ lineCount }} lines long", | ||
| "node": Object { | ||
| "loc": Object { | ||
| "end": Object { | ||
| "line": 103, | ||
| }, | ||
| "start": Object { | ||
| "line": 20, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| ] | ||
| `; | ||
|
|
||
| exports[`no-large-snapshots ExpressionStatement function should report if node has more than 50 lines of code and no sizeThreshold option is passed 1`] = ` | ||
| Array [ | ||
| Object { | ||
| "data": Object { | ||
| "lineCount": 52, | ||
| "lineLimit": 50, | ||
| }, | ||
| "message": "Expected Jest snapshot to be smaller than {{ lineLimit }} lines but was {{ lineCount }} lines long", | ||
| "node": Object { | ||
| "loc": Object { | ||
| "end": Object { | ||
| "line": 53, | ||
| }, | ||
| "start": Object { | ||
| "line": 1, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| ] | ||
| `; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,99 @@ | ||
| 'use strict'; | ||
|
|
||
| const noLargeSnapshots = require('../no_large_snapshots'); | ||
|
|
||
| // was not able to use https://eslint.org/docs/developer-guide/nodejs-api#ruletester for these because there is no way to configure RuleTester to run non .js files | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there an issue for that? If not, should we create one?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm looking more into this. I know it wasn't working properly for me when I tried using
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. News on this? |
||
| describe('no-large-snapshots', () => { | ||
| it('should return an empty object for non snapshot files', () => { | ||
| const mockContext = { | ||
| getFilename: () => 'mock-component.jsx', | ||
| options: [], | ||
| }; | ||
| const result = noLargeSnapshots(mockContext); | ||
|
|
||
| expect(result).toEqual({}); | ||
| }); | ||
|
|
||
| it('should return an object with an ExpressionStatement function for snapshot files', () => { | ||
| const mockContext = { | ||
| getFilename: () => 'mock-component.jsx.snap', | ||
| options: [], | ||
| }; | ||
|
|
||
| const result = noLargeSnapshots(mockContext); | ||
|
|
||
| expect(result).toMatchObject({ | ||
| ExpressionStatement: expect.any(Function), | ||
| }); | ||
| }); | ||
|
|
||
| describe('ExpressionStatement function', () => { | ||
| it('should report if node has more than 50 lines of code and no sizeThreshold option is passed', () => { | ||
| const mockReport = jest.fn(); | ||
| const mockContext = { | ||
| getFilename: () => 'mock-component.jsx.snap', | ||
| options: [], | ||
| report: mockReport, | ||
| }; | ||
| const mockNode = { | ||
| loc: { | ||
| start: { | ||
| line: 1, | ||
| }, | ||
| end: { | ||
| line: 53, | ||
| }, | ||
| }, | ||
| }; | ||
| noLargeSnapshots(mockContext).ExpressionStatement(mockNode); | ||
|
|
||
| expect(mockReport).toHaveBeenCalledTimes(1); | ||
| expect(mockReport.mock.calls[0]).toMatchSnapshot(); | ||
| }); | ||
|
|
||
| it('should report if node has more lines of code than number given in sizeThreshold option', () => { | ||
| const mockReport = jest.fn(); | ||
| const mockContext = { | ||
| getFilename: () => 'mock-component.jsx.snap', | ||
| options: [{ maxSize: 70 }], | ||
| report: mockReport, | ||
| }; | ||
| const mockNode = { | ||
| loc: { | ||
| start: { | ||
| line: 20, | ||
| }, | ||
| end: { | ||
| line: 103, | ||
| }, | ||
| }, | ||
| }; | ||
| noLargeSnapshots(mockContext).ExpressionStatement(mockNode); | ||
|
|
||
| expect(mockReport).toHaveBeenCalledTimes(1); | ||
| expect(mockReport.mock.calls[0]).toMatchSnapshot(); | ||
| }); | ||
|
|
||
| it('should not report if node has fewer lines of code than limit', () => { | ||
| const mockReport = jest.fn(); | ||
| const mockContext = { | ||
| getFilename: () => 'mock-component.jsx.snap', | ||
| options: [], | ||
| report: mockReport, | ||
| }; | ||
| const mockNode = { | ||
| loc: { | ||
| start: { | ||
| line: 1, | ||
| }, | ||
| end: { | ||
| line: 18, | ||
| }, | ||
| }, | ||
| }; | ||
| noLargeSnapshots(mockContext).ExpressionStatement(mockNode); | ||
|
|
||
| expect(mockReport).not.toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are the values not replaced here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m fine if eslint just picks this up from corresponding data obj, which it probably does (sorry, I’m not an eslint plug-in expert 😅)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does