Support Helm unittest snapshots

[alonsodomin]

Requesting feedback for #16532 and #11622.

The solution here is ad-hoc and specific for Helm. I'm unfamiliar with pytest-snapshot and the pytest implementation is more complex. This could however be broken down into two rules (as I think is one of the suggestions in #11622):

• A run goal implementation that only generates the snapshot files.
• A modification of the test goal that collects the snapshot files from the workspace (relative to the test files being run).

In Helm unittest this is somewhat easy as the snapshot resulting folder is hardcoded and can't be changed by the end user. However in Python world it seems that user could decide to use a different one, so a way of providing that info to Pants would be required.

---

EDIT: Closes #16532 providing an implementation in the Helm backend and a new core goal to keep the snapshots updated.

[benjyw]

Interesting problem!

Re "A modification of the test goal that collects the snapshot files from the workspace" - could this be accomplished via dep inference, with no modification of the test goal? The snapshot files are test inputs, so they are resource/file deps, and we could infer them by the presence of the snapshot dir, no?

[alonsodomin]

Would that mean that Pants would be able to see if the __snapshot__ folders exists and it will create some sort of synthetic resources targets?

Or the user would still need to add those to the BUILD files?

[benjyw]

Would that mean that Pants would be able to see if the __snapshot__ folders exists and it will create some sort of synthetic resources targets?

Or the user would still need to add those to the BUILD files?

Ideally the former. The user would need to add a resources/files target (which one depends on how helm unittest loads these files I guess), but we would infer the dep.

[huonw]

Huh, I was just thinking about snapshot testing (in Python, specifically) over the last few days, this is convenient! Thanks for starting it.

For Python (and other languages like JS) with a less strong convention, I was wondering about having an option like:

[pytest]
snapshot_globs = [
  "./__snapshots__/{test_file_name}.*", # substitutable templates
  "./__snapshots__/{test_file_name}/**/*"
]

(or maybe fields on the targets, in a way that's __defaults__ compatible.)

This could hook into tailoring, dependency inference and a Workspace.write_digest/synchronisation call.

For instance, if I had the config above, and a test path/to/test_foo.py and files path/to/__snapshots__/test_foo.json and path/to/__snapshots__/test_foo/image1.png:

1. pants tailor :: would add resource targets for those files
2. pants dependencies path/to/test_foo.py would include those resources
3. pants test path/to/test_foo.py would include those dependencies while running
4. running that test command with -- --snapshot-update (or whatever the flag is, for the particular test runner) would include the dependencies, and also detect changes to them (including file deletions) and sync them back to the repo, like you've done here

Some potential enhancements beyond that basic functionality might be:

• automatically tailoring after syncing the changes back (in case there's new or removed files)
• a pants-level --test-update-snapshots flag (or something), like you've got for helm, rather than needing to use the -- pass-through, so that one can run pants test --update-snapshots :: or similar and update helm, Python, and JS snapshots, even if the underlying runners use different flags.

Some of this feels very specific to snapshot testing, and there might be a more general framing (e.g. #18235 is related)... but that's probably okay?

[alonsodomin]

In general I like the idea of having the snapshots referenced from a resources target and dependency inference as basically plugs into the current behavior of grabbing those resources into the tests without much effort.

But I see two points of contention here in where we have at least two different approaches: 1) How to provide those targets to Pants and 2) The generation of the snapshot data.

Providing the targets

Considering how little standardization on the topic there is across languages (or even across frameworks/tooling of the same language). I see providing the snapshot globs as a field in the test targets as a fantastic option. Gives the possibility of having a default value that then can be overridden in different areas of the repo using __defaults__. Defining the globs at the subsystem could still be possible although I wouldn't choose for that in this approach as we may have two conflicting ways of providing a default.

However this approach looks to me to be at odds with the generation of the resources targets via tailor because if we rely on a field value in the test target, there may not be a test target from where to read the globs at the point that tailor is run. Seems to me that using a test field would mean having to use synthetic targets.

Generating the snapshot data

In this implementation in the Helm backend and I'm making use of the extra_output field in the TestResult since the test goal will write into the workspace (and it seems like that is its intended use). But, when combined with the update_snapshot setting at the subsystem, I see a problem here too: False positives from running tests.

The reason I see that potential problem is that subsystem settings can be input via command line or pants.toml files. So I can imagine a situation in which a user has added update_snapshot = true to their pants.toml and then runs tests normally, always getting a PASSED status for those that rely on snapshots because those are not really testing anything and are potentially overwriting the snapshots. This could be easily avoided if there was a way of having a cli-only option in the subsystem (forcing the generation of snapshots being something intentional) but I'm not sure that is possible.

In other conversations it was mentioned to use run as the goal that would generate those files, which relies on the user on doing target filtering and passing the right passthrough args (i.e.: --update-snapshot) to the underlying tool. This is obviously a pain for those that have a big monorepo that relies on different tools.

The third option is what is discussed in #18235. A goal like that one could pass the appropriate args to the underlying tooling and it feels also intentional from the user perspective. However I don't believe that this is something that has been agreed on yet.

[benjyw]

Re the "providing the targets" part: As you say - we already solve this today for "manual snapshots" that don't use any snapshotting framework. These are just data files that we manually write a resources() target and dependency for... I think we're better off leveraging that mechanism.

BUT: That said - if we need global/target config to specify a snapshot dir for generating the data, then we might as well leverage that for the first part as well.

I do think a generate-snapshots goal, akin to generate-lockfiles (or a more generic generate goal) is the way to go, rather than an option on test. It is more intentional, as you say. And generating snapshots is much more like generating lockfiles than it is like running tests.

[alonsodomin]

It makes sense to me to leverage the usage of resources instead of having a special case.

Made an initial implementation for a generate-snapshots goal and implemented it in the Helm backend. I still haven't implemented dependency inference or a tailor extension that would spot the __snapshot__ folders but wanted to have a checkpoint now to see if this implementation looks reasonable.

[benjyw]

I like how this turned out! I have a bunch of suggestions and nits for phrasing of docs, and some questions.

[benjyw]

Nice! I guess we should support generate-snapshots for Python too now... :)

[thejcannon]

@tobni for JavaScript snapshot testing