Suggestion: Templated snapshot file paths

[Techassi]

This came up in #456 regarding #377.

I suggest supporting templated snapshot file paths. The user has access to various variables to construct the final path. The exact variables (and their names) will be discussed in this issue.

In #456 I suggested the following ones: crate, module, test_function_name, suffix, and input_file (for glob tests). To get the discussion going, I will lay out a bunch of details for the variables I initially suggested.

• The crate variable will resolve to the crate the test is part of.
• The module variable will resolve to the module the test is defined in. This variable will contain nested modules in a normalized form. For example for unit tests defined in foo::bar::test the value will be foo_bar_test.
• The test_function_name will resolve to the test function name. One thing I don't know: How does this behave for integration tests? Or is this only useful for unit tests?
• The input_file variable will resolve to the input file used in glob! tests.
• The suffix (and prefix) variables will be defined in Settings. If not defined, they will either produce an empty value or error out when used in the templated file name.

Additionally, we need to decide where and how to define the file name template. One obvious place is the Settings struct. Also, we need to decide the templating syntax, for example using { var } or {{ var }}.

I'm happy to hear feedback and I'm also happy to implement this once we nailed down all parts of this suggestion.

[max-sixty]

This sounds great!

Couple of small questions, no need to answer them all before adding some code. (If we're confident it's extensible, we could add something quite small to the repo and then gradually expand...)

• Do we start with allowing customizing the path only, or also the file name?
• Is it defined relative to the root of the crate? Or src/? Or the test module? It's worth trying to write out the current default (see https://github.com/mitsuhiko/insta/issues/377 for an example) to understand how the variables would work.
• The Settings struct is fine to start, but I think we probably want it in the settings file eventually, because it's likely something we want across a whole project (ref https://github.com/mitsuhiko/insta/issues/416, makes a good point re boilerplate on each test), rather than very different ones per test.
• Would extension play in here? Or that's added onto the end separately?
• Because this would go into the core insta, we want to keep the dependencies light. So that would mean either a very simple templating like { var } which we process ourselves, or a separate optional feature that includes something like jinja. I think the simple templating would be fine, though I could imagine people want to do [:-1] selection of paths etc...

[Techassi]

A bunch of answers/comments to/on your questions.

Do we start with allowing customizing the path only, or also the file name?

I envisioned templating the whole path and file name. For example {crate}_my/path/{test_function_name}.

Is it defined relative to the root of the crate? Or src/? Or the test module?

I would allow relative paths (no absolute ones) without any path traversals (..) relative to a well known location. I would suggest CARGO_MANIFEST_DIR, because it allows the most flexibility to store (input) and snapshot files related to the crate we are running the tests in. Users could then store snapshots in locations like mycrate/fixtures/snapshots, mycrate/src/snapshots or mycrate/tests. Basically which location fits their needs best.

The Settings struct is fine to start, but I think we probably want it in the settings file eventually, because it's likely something we want across a whole project

Yes, that makes sense. I wasn't sure what the exact status of the config file is, but I can see that we want to make it a first class citizen. I can imagine it might be quite hard to make it accessible in all assertions in an efficient way.

Would extension play in here? Or that's added onto the end separately?

Actually, good question. I would say we always add the extension (.snap and .snap.new) to the templated path outself (while removing any prior extension) to keep it consistent. This would help cargo-insta to discover files like it currently does.

If we want to enable customizing it, cargo-insta also needs to know it to discover the files correctly.

Happy to hear your opinions on it.

Because this would go into the core insta, we want to keep the dependencies light. So that would mean either a very simple templating like { var } which we process ourselves

Yes, makes sense. I would also argue that the simple syntax should be sufficient to start with.

[max-sixty]

That all sounds great! Very much agree with your points.

Re the root path — I do think we want to be able to write a template that gives the current behavior. The current behavior is ../snapshots/{...} relative to the path of the test file. So maybe that means we need something like "absolute path -> CARGO_MANIFEST_DIR" & "relative path -> test file path". Or some variable for the test file path.

FWIW we already have some support for custom extensions, and I've improved the support in 1.41. But ideally we can merge parts of the broader features without dealing with all possible issues, and extensions seem particularly minor.

Would you want to have a swing at an initial version of this, to start?

[Techassi]

I do think we want to be able to write a template that gives the current behavior. The current behavior is ../snapshots/{...} relative to the path of the test file.

True, I didn't think about that, so good catch.

So maybe that means we need something like "absolute path -> CARGO_MANIFEST_DIR" & "relative path -> test file path".

What exactly do you mean by that? Maybe a better solution might look like this:

• Allow relative paths, to enable the current behaviour. Relative paths are "capped" to traverse only up to CARGO_MANIFEST_DIR to prevent placing files outside of the Rust workspace.
• Allow absolute paths, which however need to start with CARGO_MANIFEST_DIR to again prevent unwanted path traversals. We could even provide a templating variable { cargo_manifest_dir } to ease construction of these absolute paths.

FWIW we already have some support for custom extensions, and I've improved the support in 1.41. But ideally we can merge parts of the broader features without dealing with all possible issues, and extensions seem particularly minor.

Yes, we can delay this feature/discussion. It would however be nice if you could bring me up to speed on the current situation. I haven't looked into custom extensions yet.

Would you want to have a swing at an initial version of this, to start?

Sure thing, I can start cooking up some stuff. Once I have some code ready, I will open a PR in the next few hours/days.

[max-sixty]

• Allow relative paths, to enable the current behaviour. Relative paths are "capped" to traverse only up to CARGO_MANIFEST_DIR to prevent placing files outside of the Rust workspace.

  • Allow absolute paths, which however need to start with CARGO_MANIFEST_DIR to again prevent unwanted path traversals. We could even provide a templating variable { cargo_manifest_dir } to ease construction of these absolute paths.

Yes this sort of thing sounds good! And we could have source_file as a variable so something like {source_file}/../snapshots/ is the default path

[max-sixty]

It would however be nice if you could bring me up to speed on the current situation. I haven't looked into custom extensions yet.

This is a field on Settings: https://github.com/mitsuhiko/insta/blob/ee829d825b1ff561a4696f42f7d3a7a0b9860b62/insta/src/settings.rs#L65, and can be passed as --extensions to cargo-insta: https://github.com/mitsuhiko/insta/blob/53dc3de744ab6d8a724071a85c65e282d20fddf9/cargo-insta/src/cli.rs#L93-L95