Better default for Jest UpdateSnapshot

[automartin5000]

For the test task, it looks like the default behavior is for jest to run with the --update-snapshot flag. This seems to be the case even in projen's own builds.

I'm new to snapshot testing, but based on some simple testing, that seems to mean that tests always pass as snapshots are just always updated and code changes never fail. Shouldn't the default be UpdateSnapshot.NEVER? Indeed, it seems like that was the intention of this code.

[mrgrain]

There's a separate mechanism that fails PR builds if they have any mutations, including snapshot changes (or mutates the PR if you are using mutable PRs). And PRs require reviews, so this is still fine.

A lot of folks don't agree with this approach, so we probably ought to change the default at some point.

---

My very personal opinion is that I like to view snapshot changes using the file system and my IDEs diff tool, not whatever jest puts on the command line. From a untrusted code standpoint, there's also no guarantee that someone has actually looked at the snapshot changes and decided they are good vs just re-run the test with --update-snapshot. So the only safe mechanism to see if I want this change or not is reviewing the changes. At the end of the day I am committing/pushing/merging code. It's my responsibility to make sure it's the code I want. To me the extra step of failing tests doesn't provide much benefit, but I can appreciate that it does to others.

[automartin5000]

There's a separate mechanism that fails PR builds if they have any mutations, including snapshot changes (or mutates the PR if you are using mutable PRs). And PRs require reviews, so this is still fine.

A lot of folks don't agree with this approach, so we probably ought to change the default at some point.

My very personal opinion is that I like to view snapshot changes using the file system and my IDEs diff tool, not whatever jest puts on the command line. From a untrusted code standpoint, there's also no guarantee that someone has actually looked at the snapshot changes and decided they are good vs just re-run the test with --update-snapshot. So the only safe mechanism to see if I want this change or not is reviewing the changes. At the end of the day I am committing/pushing/merging code. It's my responsibility to make sure it's the code I want. To me the extra step of failing tests doesn't provide much benefit, but I can appreciate that it does to others.

My biggest concern with this approach is that it runs counter to standard testing workflows, at least the ones I'm used to: Write some tests, run projen test, push to branch.

What happens if someone accidentally runs projen test locally before testing snapshots? Then they don't get feedback until the anti-tampering check fails? Then the developer has to go and parse the anti-tampering check and figure out what failed.

[mrgrain]

What happens if someone accidentally runs projen test locally before testing snapshots? Then they don't get feedback until the anti-tampering check fails? Then the developer has to go and parse the anti-tampering check and figure out what failed.

Yeah, I understand that concern. It's totally valid, hence why I think the default should change. My only response to this is that the developer has to actively ignore the pending local changes and decide to not commit them to even get into this situation.