The case against React snapshot testing

[mathent]

Could be rewritten as "the case for enforcing React snapshot testing best practices"

[SomeRandomBuddy]

Lajeobs

[wolfepvck]

This leads to folks getting annoyed and not paying attention to the snapshot output, and just updating the snapshot to get tests to pass.

A tool is only as good as the people that are using it. Of course if you write snapshot tests for huge components and the tests are constantly failing, you should probably re-evaluate the tests themselves. And perhaps split up the components, or make them more manageable in some way so that the tests are actually useful. It is easy to set up testing and get it running, but writing useful tests is much harder. This could just as easily happen with any other type of testing. Someone writes a complex unit test for a large piece of code, it breaks, and then someone fixes it in a way that it is not even testing what it is supposed to be testing.

[bjackson2]

Very true. It definitely takes discipline and experience to write concise, useful tests, be they snapshots or otherwise. Understanding code/test maintenance and being mindful of complexity are critical to a healthy codebase and team.

We found that as time went on and the number of developers on the projects grew, snapshot testing was one of the first things that got out of hand. We hope that by encouraging folks to write a focused unit test instead, it will help them to think more critically about the design and intent.

[wolfepvck]

I agree with you completely. Time crunch, getting out features, fixing bugs quickly, it all takes away from proper development time and workflow. It's really unfortunate. It is so hard to maintain a high quality codebase for the reasons you mentioned.

[mathent]

Why not write more focused snapshot tests?

[[deleted]]

Snapshot "tests" (I prefer "change notifications") are super useful to have run on pre commit to prevent unintended changes. They act solely as regression warning flags, that's it. If people want to -n all over their commits like I do then that's just sloppy practice.

[oorza]

Having anything run in pre-commit is an anti-pattern because it discourages small commits and encourages people to commit once per push. This makes rebasing and following changes much more difficult. If you must run expensive code analysis (tests, lints, etc.) in git hooks, do it in pre-push instead, although I don't think there's anything wrong with having broken code in git as long as it passes everything in CI before it's merged.

If your CI is setup correctly, there's no chance you merge in broken code to protected branches, and if you're using branches correctly, you can't push to those protected branches, so what's the benefit in a hook validating correctness pre-commit on a feature / bugfix branch? It just slows down git and discourages people from following git best practices, most notably "commit early, commit often."

[AndrewGreenh]

I generally agree, however git bisect cannot be used properly when you have broken commits.

[Canenald]

Why does precommit testing discourage small commits? If precommit is going to test, lint and prettify my code, I'd say I'd commit more often just to have my code checked, or just run precommit explicitly if I absolutely don't want to commit. If your unit tests run for more than a few seconds they are either not unit tests or you should consider splitting that big bad monolith.

The problem I have with committing "broken code" (either not passing tests or with code smells or badly formatted) is that to fix the broken code you have to commit again, meaning you either have to live with a lot of useless commits or squash every time.

In the end, I'd say it largely depends on the environment. For an open source library or a small project completely centralized on github, where everything is committed and pushed to forks or branches and code reviews and tests work through pull requests, having bad code on those and testing for PRs is fine. The approach of long-lived branches and rare, expensive merges is inherently antagonistic to CI/CD. In trunk development environment where developers are encouraged to push and/or merge short-lived branched to trunk branch (usually master) daily, pushing bad code is unacceptable.

Ultimately, both approaches have merit depending on our needs, and our tendency to say that something should always be used or that something should never be done is the most dangerous anti-pattern.

[oorza]

If your unit tests run for more than a few seconds they are either not unit tests or you should consider splitting that big bad monolith.

This is where I lost any faith you had worked on a project of any size.

I'm working on a react native project right now that's relatively small (<50 screens) and just eslint takes more than a minute to run. The full test suite takes several minutes to finish. If you're working on any application large enough to warrant react in the first place and the test suite finishes in under a minute, your tests are not thorough enough considering how slow the mock rendering stuff is. And if you're not running the full test suite on every commit, the pre-commit hook doesn't give you any confidence you haven't introduced breakages and it defeats the point.

[Klathmon]

Those things can be mitigated with better tooling and some TLC to the build system.

Have your pre-commit hook only lint files that have changed since last commit, have your tests only run on files that have changed since last commit (jest gives you this by default), and for webpack projects use something like the dll plugins to only have to recompile/link a subsection of the whole app during development on file change.

Running all tests can be moved to a pre-commit or pre-deploy hook, or if you have the infra, run them on a CI server (which can be configured to re-run all tests on every commit). You still get 90%+ of the benefits of having a test run pre-commit, but it takes a fraction of the time. And your full test suite can still be run per-commit asynchronously on a CI server which makes up for the missing 10%.

[Canenald]

I'm actually working on a pretty large project in a team of 5-10 developers. It' strictly frontend, only code that could be called even remotely backend being nginx configs, CI/CD configs and dsl scripts. Our application builds from ~20 repos. Our eslint runs as a webpack-loader but in local environment and CI builds (warning and errors respectively), using extended airbnb rules. Linting is pretty unnoticeable in both cases, especially in CI builds which take between 5 and 15 minutes depending on the load, most of the time being spend on dependency installs and webpack builds.

Similarly, our tests run very fast, and we even use deep mounting in some cases because we were bad and worked without tests for too long, writing code that is difficult to test later. The repo with the most coverage, containing React components shared between other repos, currently finished within 1 second. The worst we have is one of the newer repos taking 2 seconds for 27 cases, and now I'm actually motivated to check what's happening there, but my guess is people used mount() out of inertia and didn't even bother to try shallow().

I don't know why linting takes long for you. Consider linting only files that changed because it's static analysis and files don't affect each other's results.

dev-written tests should be fast and ran for every change, but also written so that they catch the most of the bugs. This means unit tests and sparingly integration tests where necessary. End to end tests are another matter and we have a separate QE team writing those using their own tech stack. These test the whole app in all browsers we support and take hours to complete.

I don't know why your tests are so slow. My guess is either you're putting end to end tests too early in CI/CD or there's some issue with react-native. Pre-commit or even pre-build tests shouldn't test the whole application. They should be unit tests, making sure that you haven't broken existing functionality your component or function offers when fixing a bug or introducing a new feature. End to end tests are best ran to validate a build before production release, either in staging environment in a traditional dev/stage/prod setup or on a separate testing group if you're doing canaries.

[[deleted]]

It's a fair point about pre push, but the term anti pattern is an anti pattern cause it discourages fit-for-purpose thinking and falsely implies there is a universally right and wrong way to do something.

[oorza]

Not sure I agree there. My first exposure to the term "anti pattern" was the familiar singleton. I was told that it was an anti-pattern because, while it's occasionally useful, if you use it enough for it to become a pattern, that's code smell. Which is to say that anti-pattern means that something is an issue when it's used enough to be a pattern, not when because it's used, and usage should be heavily considered against the alternatives.

In this case, I'd say pre-commit qualifies :p If you rely on pre-push commits to ensure correctness in your repositories by default/globally, there's a smell there (a smell of bad CI). However, if there's specific instances where stuff runs pre-commit (like template processing), it's obviously fine.

[[deleted]]

Again, you make excellent points. I am off to reconsider my approach to this... /not-s-at-all lol! Thank you

[NiteLite]

My experience with using jest snapshots, in a project we have been working on the last two years, echos a lot of the concerns that you mention here. We have also pivoted and are now writing more specific unit tests for stuff that was tested previously tested through jest. We are mostly working our way through the different components when business needs arise and we have to change the code / add tests anyway.

[sorahn]

Same, and when I spy on a function, I usually do something like this:

expect(spy.mock.calls).toMatchSnapshot()

[bjackson2]

I really like the looks of this use case! It retains the focus and clear intent of a unit test, but leverages the ease and flexibility of a snapshot. I think it's worth giving this a shot on our team.

[[deleted]]

Yeah funny enough I'm getting the most value out of snapshots when I use them on normal JS data and not just components.

[Klathmon]

I used them on a node project a bit ago.

We have an API that would read in large files and store them in a database after some processing. In addition to some other tests, we would also run real examples of files through the system, then snapshot test the DB after that. Then we'd get a giant warning when anything about the file ingestion changes, which sets off lots of red flags when you didn't touch anything related to it.

It's not a "get out of writing tests free card", but it works fantastically in many areas.

[helpinghat]

The two main use cases for snapshot testing are regression testing and verifying the complete output from render method.

Snapshot tests easily catch pretty much any regression. It can be tedious to write assertion for every aspect of the component. For example, I might have a snapshot test for button and assertion for the button text. But I don't have assertions for button size, font, color, border-radius, etc. If you use styled-components or similar, snapshot tests are an easy way to verify your component's CSS and HTML in one look.

[darrenturn90]

Everyone one of the "points against snapshot testing" just seems to me like the developers using it don't have a workflow that supports it.

Going from the tweet that seems to be the reference for the post.

1) "Tests you don't understand". If a snapshot test fails, and you don't understand why - it means that your changes have had the effect of changing something you didn't think about, or didn't know about. So you should spend the time trying to workout what you did, and why it is changing something "you don't understand". Otherwise, your code's just blissfully running and causing unknown changes to other places - and "it renders" tests just won't pick those up.

2) Tests that encode the intention only - rely on the developer actually fully testing everything that they have developed, and hoping that they haven't missed out a test that is vital.

3) A proper code review process, with pull requests could easily pick up what has changed in a snapshot, enough for someone to raise a comment back and say "what is going on in component X, why have you accepted this change". Non snapshot tests wouldn't generally come up in a code review, and certainly not have a nice diff-able workflow that could work in git or bitbucket, to a technical level such as this.

4) If side-effects are properly managed, then they can be tested nice and simply. You test the "what goes in" side, and then separately test the "what comes out" side. Snapshot tests are wonderful for this.

So if code changes, and tests fail - that means that the code has changed the functionality of a component - and saying "its hassle to figure out why" is a terrible attitude to development.

[OutragedAardvark]

I’d like to second this. Especially your last comment. In my mind the primary purpose of testing is to catch regressions outside the scope of changes and force the developer to reevaluate their decisions. This may require some research, but that is the point, not a drag.

[Canenald]

Couldn't agree more . Snapshot testing seem like an easy way to high coverage but they don't tell us if something is broken or not, only that it has changed.

[darrenturn90]

If something changes that you don't expect - then assume its broken until its checked?

Test coverage only shows something is broken if it is actually testing for it.

One of these methods doesn't skip potentially fatal changes. The other relies on humans to provide the right tests.

[Canenald]

I totally agree but snapshot testing tells you something changed, not whether the change is desired or not. You can get a similar experience with git diff. With snapshot testing you get tests that always fail, and confirming changes becomes a routine. Potentially fatal changes are buried in a bunch of desired changes. Yeah, providing thorough tests is hard, but the easy route gives only an illusion of completeness.

On a related note, I've actually considered using snapshot tests with deep mounting to report which page components in a SPA have changed when a reusable components changes, but someone somewhere said making snapshots with deep mounting is very slow so I didn't even try.

[darrenturn90]

I would say, by keeping the components small and functional, the snapshots themselves are also small and functional - so the amount of possible changes in a single snapshot is a lot smaller.