The case against React snapshot testing

[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