Fail when new snapshots are created in CI

[novemberborn]

When running tests in CI environments, if new snapshots are created it implies the tests are behaving differently then during development. This can hide bugs. The snapshot assertions should fail, rather than silently creating the snapshot and passing.

@avajs/core and others, what do you think?

[impaler]

It seems like a reasonable requirement to prevent false positives. I think an error message for this would need to describe the reasoning and fix, something along the lines of:

"Snapshot {x} is missing a fixture file. To prevent false posititives, a verified fixture file is required when testing in the CI environment."

[novemberborn]

How about:

Could not find snapshot. Note that snapshots are not created automatically when in a CI environment.

(This would be shown as the message for the t.snapshot() assertion failure.)

[sindresorhus]

👍

[AJamesPhillips]

Thanks all for the excellent contributions to date, much appreciated!
Any further thoughts on this? I wasn't clear on how Ava would tell it was running in a CI environment. You use is-ci which uses https://github.com/watson/ci-info/blob/master/index.js Got it.
Perhaps this behaviour of erring if no snapshot was found could be enabled by default, and to generate the new snapshot files you would have to pass the -u flag? This would be a breaking change and would not score great on the "self documenting options" but I can't think of anything better right this moment. Anyone considering better ideas?

[novemberborn]

@AJamesPhillips would you be willing to help out with this?

[AJamesPhillips]

Hi @novemberborn , I had a quick look at the code. I would prefer to add a flag which if set, will fail the tests if a snapshot can not be located, as opposed to only failing if on a ci environment and a snapshot can not be located.
I just had a case now where our tests passed when they should have failed. In this case they were run locally inside a docker container. This would not be caught by the is-ci check proposed. Any thoughts on this approach (they're not mutually exclusive so we could do both at some point)? Thanks.

[novemberborn]

In this case they were run locally inside a docker container.

If you run tests in a Docker container, how do you update the snapshots?

The idea is that during development you'd notice the new snapshot files (since you need to commit them). I think if we implement the CI flag then you could set CI=1 in your container and AVA will treat it as such. Would that be sufficient?

[AJamesPhillips]

Yes apologies that wasn't clear @novemberborn . Locally we normally run and update our tests outside of docker. On our jenkins ci server we build a docker image and run the ava tests inside the container. In this case I was running the ci build and tests locally to check it was working correctly. It wasn't because the build step did not copy the snapshots to the right file name and location for ava to see them, so ava then generated new snapshots which would have masked a subsequent real error.

if we implement the CI flag then you could set CI=1 in your container

That would work, though setting CI=1 when running a container locally is strange. Not saying I object, just saying I think it wouldn't be obviously for others. In our case it would be reasonable as it would sit in the script file we call on the ci server to setup, build and run the tests in the container. So setting CI=1 inside a file like test-ci.sh would not be unreasonable.

[novemberborn]

In our case it would be reasonable as it would sit in the script file we call on the ci server to setup, build and run the tests in the container.

Arguably you'd want to forward env variables from Jenkins to your container, so I think it's reasonably that you'd need to do the same when testing your container setup outside of Jenkins. I admit it's not immediately obvious, but that also goes for having a configuration option.

[billyjanitsch]

I would love this as a flag as well, and IMO it should be enabled by default in all environments -- not just CI. I've never liked the behavior of auto-creating new snapshots without -u: I imagine most users think of running ava as a pure function which returns a boolean describing whether their tests passed or failed. So it's odd for ava to have side effects by default, especially those which modify VCS-tracked files.

Pragmatically, it complicates scenarios like running a test suite as a commit/push/prepublish hook. I've often successfully pushed commits (relying on my git hook to fail if the tests do anything other than succeed) only to realize that the test suite passed but dirtied the repo, so my (incorrect) commit shouldn't have gone through.