Allow file extension to be part of the unique snapshot check

[joelverhagen]

Describe the bug

Verify acts in a strange way when there are two Verify calls with the same snapshot prefix but a different file extension. I was expecting file extension to be part of the "identity" of the snapshot file (as the file system would allow) but this appears to not be the case. The snapshot gets renamed based on whichever Verify(..., extension: "csv or json") goes last.

Apologies if this should be a feature request instead of a bug report. It feels buggy to me but that's perhaps just due to a bad expectation on my part.

I ran into this because I have a JSON file called ..._PackageReadmes.verified.json and a CSV called ..._PackageReadmes.verified.csv in the same test file. It ran into this problem.

Minimal Repro

namespace VerifyExt;

[UsesVerify]
public class Tests
{
    [Fact]
    public async Task MyTest()
    {
        VerifierSettings.AutoVerify();
        VerifierSettings.DisableRequireUniquePrefix();

        await Verify("foo", extension: "csv").UseTextForParameters("MyFoo");

        // 'Tests.MyTest_MyFoo.verified.csv' is on disk right now

        await Verify("bar", extension: "txt").UseTextForParameters("MyFoo");

        // Now it is 'Tests.MyTest_MyFoo.verified.txt'. The CSV is gone!
    }
}

Submit a PR that fixes the bug

TODO. I will work on this now. I think I will try adding an option IncludeExtensionInUniquePrefix to take the stance that this is by design instead of a bug. Let me know if that's the wrong direction.

[SimonCropp]

can you instead use this approach>

[Fact]
public async Task MyTest()
{
    await Verify(new List<Target>
    {
        new("csv", "foo"),
        new("txt", "bar")
    });
}

[joelverhagen]

I tried this in my test suite and face the same problem. It appears to be a race condition based on what happens before each Verify call. I had some async work reading from Azure Storage inside async helper methods that at the end call Verify.

There appears to be some sync setup in the Verify task. If any of this is interleaved then the behavior is inconsistent. I'll confirm this theory by switching the order of the Verify calls and the subsequent awaits.

[joelverhagen]

Oh, I misread your suggestion (I saw that as the Task.WhenAll combination). This is not as easy to generalize with simple test helpers that collect some test output via async calls then at the end of the helper method call Verify. I guess this means I need to collect a bunch of Target instances in my tests and pass them all to a single Verify call at once. I'll go try.

[joelverhagen]

Yes that works. This test I added in my PR then modified passes:

    [Fact]
    public async Task MultipleExtensions()
    {
        #region UniqueForFileExtension
        await Verify(new[]
        {
            new Target("txt", "the text"),
            new Target("xml", "<a>b</a>"),
        }).AutoVerify();
        #endregion
        Assert.True(File.Exists(CurrentFile.Relative($"Tests.{nameof(MultipleExtensions)}.verified.xml")), "The xml snapshot should exist.");
        Assert.True(File.Exists(CurrentFile.Relative($"Tests.{nameof(MultipleExtensions)}.verified.txt")), "The txt snapshot should exist.");
    }

I'll go give it a try in NuGet Insights and see how it shakes out with helper methods.

[SimonCropp]

i usually push back on people using multiple Verify calls in the same test. since it means u often need to run the test multiple times to get all the snapshots to be created

[joelverhagen]

Makes sense. I think it's a bit of a challenge for migration since you want to treat a verify call like an assert. And having multiple asserts per test is very common.

Sure, sometimes N asserts map to a single verify as part of the snapshot but in my case it means I have to collect a bunch of Target instances and then call a single verify. I haven't had a chance to iron it out completely but I think it'll work. It's just a shift in test infra from simple asserts nested in helper methods to returning Targets to a single Verify directly in the test method.

[SimonCropp]

I think it's a bit of a challenge for migration since you want to treat a verify call like an assert. And having multiple asserts per test is very common.

yeah usually that is mitigated by the fact that Verify supports anon objects. https://github.com/VerifyTests/Verify/blob/main/docs/anonymous-types.md

[joelverhagen]

I tried out a flow where I have an AddTargetsAsync helper method that is called through an extended test. This collects Target instances in a List<Target> field. Then call Verify at the end on that list. This is to align with your recommendation of only having a single Verify call per test. This seems perhaps workable but had two problems:

1. We lose fail fast on the tests. If there is a <Step 1> then <Step 2> arrangement in the test, there are now no assertions at the end of <Step 1> and the <Step 1> failures now happen at the end of <Step 2> along with <Step 2> failures if any. We could perhaps break up <Step 1> and <Step 2> into multiple tests but this feels painful since we're basically duplicating the <Step 1> setup into another test just to run <Step 2> and losing the "narrative" of a longer, multi-step integration test.
2. It's not clear how to have an assertion at the end of <Step 2> that asserts that some data hasn't changed from a value produced by <Step 1>. It's like I want to say simultaneously want to say "Verify step 1 output hasn't changed and verify 2 output is the same as step 1" without storing two snapshots (since they are just duplicated in this case).

I felt like the approach I had before where I had multiple Verify calls spread throughout the test and/or test helpers was actually working more to my liking, I just had the file extension conflict issue. I think I might go back to that and just work around it by not using file extension to disambiguate a snapshot and add something to UseTextForParameters to make it work, if there's not a desire to let file extension be part of the uniqueness of a snapshot.

[aradalvand]

@SimonCropp

can you instead use this approach>

[Fact]
public async Task MyTest()
{
    await Verify(new List<Target>
    {
        new("csv", "foo"),
        new("txt", "bar")
    });
}

How could you use that approach when one of the snapshots is JSON (for which you'd normally use VerifyJson) and the other is not?

[aradalvand]

This should very much be allowed IMO. Surprised to find out it isn't.

[SimonCropp]

@aradalvand please share a minimal repo of what you want to achieve in a GH repository

[SimonCropp]

closing this one. I suggest instead you submit a PR that meets your requirements with a single Verify call