Adds no-large-snapshots rule #24

anescobar1991 · 2017-11-30T02:54:07Z

Implements no-large-snapshots rule as discussed in issue #21. This rule serves to discourage having large snapshots which are difficult to review and can lead to bugs being snuck in.

I added it to the recommended config as a warn and kept the default max of 50 lines threshold. I can modify this as needed though if you all would like.

anescobar1991 · 2017-11-30T02:56:40Z

index.js

 module.exports = {
  configs: {
    recommended: {
+      parserOptions: {


Added this bit in because snapshots contain backticks (`) which are not valid in es5.

But we shouldn't lint snapshots, so this isn't needed

EDIT: wait, processors. fancy

ntwb · 2017-11-30T11:00:23Z

Neat rule, but I don't think it should be added to the recommended config.

stylelint currently has 5 snapshot tests but each is thousand/s of lines of code.

How stylelint uses snapshots at the moment is to take a large real world CSS file, e.g. BootStrap and lint against it with a specific set of stylelint rules, if a change is made to stylelint that breaks one of these snapshots then we know we have introduced a regression that is more than likely not covered by a unit test https://github.com/stylelint/stylelint/tree/master/system-tests

SimenB · 2017-11-30T13:57:21Z

index.js

      rules: {
        'jest/no-disabled-tests': 'warn',
        'jest/no-focused-tests': 'error',
        'jest/no-identical-title': 'error',
+        'jest/no-large-snapshots': 'warn',


I agree it should not be in the recommended config

SimenB · 2017-11-30T14:04:52Z

package.json

@@ -23,6 +23,7 @@
    "eslint": "^4.10.0",
    "eslint-config-prettier": "^2.7.0",
    "eslint-plugin-prettier": "^2.3.1",
+    "eslint-plugin-self": "^1.0.1",


should update the lockfile?

Also, very fancy 😀

Yes! I was using npm so didn't see that.

SimenB · 2017-11-30T14:05:31Z

.eslintrc.js

+  overrides: [
+    {
+      files: ['*.test.js'],
+      env: {


can you import the list maintained in the plugin?

SimenB · 2017-11-30T14:08:16Z

rules/no_large_snapshots.js

+
+module.exports = context => {
+  if (context.getFilename().endsWith('.snap')) {
+    const lineLimit = context.options[0] || 50;


I think it should take an object as options, just to be easier to expand in the future.

Yeah I was debating that. Will do.

SimenB · 2017-11-30T14:09:00Z

rules/__tests__/no_large_snapshots.test.js

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


Is there an issue for that?

If not, should we create one?

I'm looking more into this. I know it wasn't working properly for me when I tried using ruleTester to test this rule and I thought I had read in their docs that it does not support non .js extensions but I can't find where I read that anymore.

News on this?

SimenB · 2017-11-30T14:10:24Z

docs/rules/no-large-snapshots.md

+
+## Rule Details
+
+This rule looks at all Jest snapshot files (files with `.snap` extension) and


is it possible to ignore any of them (// eslint-disable)? without using overrides in the config

Yes it does work. But on snapshot updates it would be wiped I believe so not sure how useful that would be.

Correction: Simply running jest with -u flag will not wipe the comment, but if there is an actual change to the snapshot file that has a comment then the entire file is regenerated and as such the comment wiped.

Should we include some sort of config then, to be able to ignore specific snapshots?

Specific snapshots or entire snapshot files? If it is the latter then I am not sure how much value that would add, we would just be duplicating eslint functionality (can use overrides or have separate .eslintrc files).

If you mean ignoring specific snapshots then yes I believe that can be done pretty easily, I can work on that today.

I meant snapshot files, not specific snapshots within them 🙂

Thinking about it more I am not sure that being able to ignore a specific snapshot through a config would be a good user experience. They would have to provide the snapshot names in an array which could get unwieldy. It may be better to ignore whole files using built in eslint functionality.

How are you envisioning it?

Yeah, .eslintignore is probably enough :)

anescobar1991 · 2017-11-30T21:14:43Z

@SimenB Just pushed changes that takes care of all the feedback. Few things to note: eslint-plugin-self is no longer needed and I removed parserOptions from the recommended config since this rule is no longer being exported with it.

SimenB · 2017-11-30T21:44:33Z

README.md

@@ -40,6 +40,7 @@ Then configure the rules you want to use under the rules section.
    "jest/no-disabled-tests": "warn",
    "jest/no-focused-tests": "error",
    "jest/no-identical-title": "error",
+    "jest/no-large-snapshots": "warn",


I thought this was a list of all the rules exported that the user can configure independently of the recommended config. Why wouldn't it be in there?

well, they now match the recommended config. Maybe that's weird? It doesn't list all rules

Yeah you're right that is weird. As a user I took that as an example of how you could configure the rules exported within this plugin. To me the table near the bottom of the README is what lets me know whether a rule is in the recommended config or not.

Either way let me know how you want me to proceed.

I don't really care either way, but having 2 lists of all rules doesn't make sense.

Take a look at e.g. https://github.com/yannickcr/eslint-plugin-react (just a couple examples, then a huge list). Or maybe https://github.com/benmosher/eslint-plugin-import, which has 5, then "etc.". followed by a list

Makes sense. I was just trying to follow what I thought was the pattern already established. But I see that it wasn't a complete list to begin with so my bad! I will remove.

SimenB

Can you update the changelog as well?

SimenB · 2017-11-30T23:35:22Z

GTG from my side, would just like another set of eyes on it 🙂

thymikee · 2017-12-01T04:24:43Z

docs/rules/no-large-snapshots.md

+```json
+...
+  "rules": {
+    "jest/no-large-snapshots": ["warn", { "sizeThreshold": 12 }]


Can’t we just name it “size”?

or maxSize?

Done in 8267cfa

thymikee · 2017-12-01T04:33:56Z

rules/__tests__/__snapshots__/no_large_snapshots.test.js.snap

+      "lineCount": 52,
+      "lineLimit": 50,
+    },
+    "message": "Expected Jest snapshot to be smaller than {{ lineLimit }} lines but was {{ lineCount }} lines long",


Why are the values not replaced here?

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 😅)

I’m fine if eslint just picks this up from corresponding data obj, which it probably does

It does

anescobar1991 · 2017-12-01T22:48:11Z

Anything pending or can this be merged?

SimenB · 2017-12-02T09:09:36Z

I would like to figure out https://github.com/jest-community/eslint-plugin-jest/pull/24/files#r154083575

SimenB · 2017-12-02T09:12:35Z

Release blocked on #20 (#8), unfortunately.

SimenB · 2017-12-04T15:16:08Z

Published as 21.4.0

anescobar1991 commented Nov 30, 2017

View reviewed changes

SimenB reviewed Nov 30, 2017

View reviewed changes

feat(rules): add no-large-snapshots

631b7b5

anescobar1991 force-pushed the feature/add-no-large-snapshots-rule branch from 8dc0cd8 to 631b7b5 Compare November 30, 2017 21:07

SimenB reviewed Nov 30, 2017

View reviewed changes

SimenB approved these changes Nov 30, 2017

View reviewed changes

SimenB requested review from orta, rogeliog, jkimbo and thymikee November 30, 2017 21:46

anescobar1991 added 2 commits November 30, 2017 15:26

chore(changelog): update

f4ccc37

docs(README): remove rule from rule config example

3990217

thymikee reviewed Dec 1, 2017

View reviewed changes

thymikee approved these changes Dec 1, 2017

View reviewed changes

feat(no-large-snapshots-rule): rename threshold option

8267cfa

SimenB merged commit a42d917 into jest-community:master Dec 2, 2017

SimenB mentioned this pull request Dec 4, 2017

New rule proposal: disallow large snapshots (no-large-snapshots) #21

Closed

dependencies bot mentioned this pull request Dec 4, 2017

Update 35 dependencies from npm storybookjs/storybook#2429

Merged

renovate bot mentioned this pull request Feb 2, 2018

chore(deps): update dependency eslint-plugin-jest to ^21.0.0 yarnpkg/yarn#5192

Merged


		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


		## Rule Details

		This rule looks at all Jest snapshot files (files with `.snap` extension) and

Adds no-large-snapshots rule #24

Adds no-large-snapshots rule #24

Conversation

anescobar1991 commented Nov 30, 2017 • edited Loading

Choose a reason for hiding this comment

SimenB Nov 30, 2017 • edited Loading

Choose a reason for hiding this comment

ntwb commented Nov 30, 2017

SimenB Nov 30, 2017 • edited Loading

Choose a reason for hiding this comment

SimenB Nov 30, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimenB Nov 30, 2017 • edited Loading

Choose a reason for hiding this comment

anescobar1991 Nov 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anescobar1991 Nov 30, 2017 • edited Loading

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

anescobar1991 commented Nov 30, 2017

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

SimenB Nov 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimenB left a comment

Choose a reason for hiding this comment

SimenB commented Nov 30, 2017

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

Choose a reason for hiding this comment

anescobar1991 commented Dec 1, 2017

SimenB commented Dec 2, 2017

SimenB commented Dec 2, 2017

SimenB commented Dec 4, 2017

anescobar1991 commented Nov 30, 2017 •

edited

Loading

SimenB Nov 30, 2017 •

edited

Loading

SimenB Nov 30, 2017 •

edited

Loading

SimenB Nov 30, 2017 •

edited

Loading

SimenB Nov 30, 2017 •

edited

Loading

anescobar1991 Nov 30, 2017 •

edited

Loading

anescobar1991 Nov 30, 2017 •

edited

Loading

SimenB Nov 30, 2017 •

edited

Loading