jest-community / eslint-plugin-jest

ESLint plugin for Jest
MIT License
1.12k stars 228 forks source link

[no-large-snapshots] Unexpected rules are being enabled for snap files #1559

Open asapach opened 2 months ago

asapach commented 2 months ago

Since snapshot processor has been removed in #1532 (v28.0), we see that unexpected rules are now being enabled for .snap files from ESLint config, which didn't previously run. Because snap files are generated, it doesn't generally make sense to run the same rule set as the rest of the source code, and disabling such rules via overrides seems like overhead. It would be good to either have an option to restore the previous behavior where only no-large-snapshots rule was enabled for snapshots, or to minimize the overhead.

Steps to reproduce:

  1. Follow the guide in no-large-snapshots docs
  2. Enable more rules in your ESLint config that might clash with snapshots, e.g. no-irregular-whitespace
  3. See that ESLint now produces more errors than it did prior to v28.0
G-Rath commented 2 months ago

Managing what rules are enabled for particular files should be the responsibility of the configurer, not us, since you can change those in ways that we can't pickup - that's why we don't explicitly configure overrides/files in our shared configs, and why the snapshot processor was removed in the first place (as there are some other rules people did want to apply to their snapshots).

Could you share more details about the setup and config you're using?

asapach commented 2 months ago

We use eslint:recommended in our root config and jest plugin for our tests, including no-large-snapshots rule. After upgrading to v28.0 we've noticed that some snapshots were causing lint errors, because no-irregular-whitespace was complaining about <NBSP> characters, which as it turns out came from &nbsp; HTML entities in our tests. This was completely unexpected, since no violations were previously reported there. After some investigation we've created an override for *.snap files to modify the no-irregular-whitespace rule: { skipTemplates: true }, which worked around the issue. However, this presents some questions:

  1. Are more rules being run on snapshots that previously thought?
  2. Was the snapshot processor disabling them or hiding the errors?
  3. How do we make sure that a minimal rule set is enabled for snapshots only? Both for performance and maintenance reasons.
G-Rath commented 2 months ago

I believe this primarily boils down to your configuration - the processor had a postprocess function which filtered out all messages that didn't come from jest/no-large-snapshots, so it would do nothing to prevent rules from being run against snapshot files themselves.

So then it comes down to your config: if you structured your config in a way that meant snapshot files would be targeted by a particular rule then yes it would be run regardless of the processor, and by extension ensuring what rules are applied to snapshots is something for you to manage - off the top of my head I'm pretty sure so long as you're not using --ext snap then all that would be required is to configure jest/no-large-snapshots in a dedicated config block that just targets *.snap, and use ignores in your main Jest config block (which assumably has files: ['path/to/tests/**']) to exclude *.snap.

That might also be the case if you are using --ext snap but I can't remember if that expands the scope of what you've got in your config...? it seems like it would be silly to but I can't say for sure so won't claim that is the be-all-end-all solution :)

The good news is from what I understand about how flat config works this should be a lot simpler since --ext is gone meaning you just need to lay your configs out as I described above and you should be fine.

sergei-startsev commented 2 months ago

@G-Rath ignores is only available in ESLint 9, and many projects aren't ready to update eslint to the latest version for various reasons (e.g. plugins don't support v9 yet), so it's not the case for most projects at the moment.

sergei-startsev commented 2 months ago

I'd say it's quite unexpected that eslint recommended rules, which don't make any sense for snapshots, began to apply to them. And the only option to effectively address this issue for many projects is to introduce own preprocessor.

asapach commented 2 months ago

eslint@8 doesn't have ignores, but it does have ignorePatterns, but the problem is that either way ESLint emits a warning in this case:

warning: File ignored because of a matching ignore pattern. Use "--no-ignore" to override
G-Rath commented 2 months ago

Yeah sorry that's me mixing up the docs - pre v9 it's called excludedFiles

G-Rath commented 2 months ago

Ok so to recap I think there's three main ways you can address this:

  1. switch to configuring all rules via overrides, then you can use excludedFiles to exclude snapshots
    • this shouldn't be that hard (note you just need to move extends and rules - everything else can remain at the top level), and as a bonus is more "flat config"-y
  2. explicitly disable rules that are triggered on snapshot files - so far you've just mentioned the one so it seems pretty easy to just evaluate what to do for rules on snapshots as they come up
  3. use your own processor to enable an "allowlist" of rules to report on snapshots; you only need about 5 lines of code for this

Keep in mind one of the reasons removing the processor came up was because people wanted to run other ESLint rules against their snapshots - so restoring it would only serve your case.

On the note of performance and maintenance: that's on you as the configurer - as a plugin, we have no way to prevent you from running rules on files; the best we can do is filter out messages from rules after they've been run via postprocess.

asapach commented 2 months ago

I think we'll go with option 1, but it would probably be a good idea to document it in no-large-snapshots docs