VerifyTests / Verify

Verify is a snapshot tool that simplifies the assertion of complex data models and documents.
MIT License
2.49k stars 141 forks source link

Support storing parameterized method snapshots in one file #468

Closed AvremelM closed 2 years ago

AvremelM commented 2 years ago

Is the feature request related to a problem

Parameterized test methods with thousands of TestCases are unacceptably slow. In my case, I have a parameterized test method with 3600 cases. Total runtime without calling .Verify() is ~200ms. With Verify it is ~56s+. This is (as far as I can tell) due to the IO cost of reading/writing thousands of files, because each TestCase has its own file.

Describe the solution

Support storing all snapshots for a parameterized test method in one file. (This would be similar in concept to Snapper's [StoreSnapshotsPerClass], but the primary feature here would be to store snapshots per test method. Edit: actually, Snapper supports this case with their concept of "Child Snapshots" and .ShouldMatchChildSnapshot(...))

Describe alternatives considered

I tried just combining all the TestCases into one result object before calling .Verify(allCaseResults). This works to reduce total runtime, but it sacrifices all the benefits of having individual Test Cases, and makes determining which case caused a failure very time-consuming.

Additional context

I created a discussion item but I realized afterward that those don't seem to gather any attention.

SimonCropp commented 2 years ago

it is an interesting idea. but would this actually result in a performance improvement? wouldn't the same amount of IO still need to happen, but they just happen a single file instead of multiple files?

SimonCropp commented 2 years ago

have a parameterized test method with 3600 cases. Total runtime without calling .Verify() is ~200ms. With Verify it is ~56s+.

can you run a profiler and see what the proportional break down of time is?

SimonCropp commented 2 years ago

are you using xunit?

AvremelM commented 2 years ago

(Edit: using NUnit)

Well, the idea is that there would only be the overhead of opening, reading (and writing) 1 file once, instead of 3600 files once.

I'm having difficulty separating out the noise in the profiling results. But here's a pared down NUnit test method as an example:

private static IEnumerable<int> testCases = Enumerable.Range(0, 3600);

[TestCaseSource(nameof(testCases))]
public Task Test(int testCase)
{
    var result = testCase * 2;  
    return Verifier.Verify(result)
                 .UseParameters(testCase)
                 .UseDirectory("snaps")
                 .AutoVerify()
                 .DisableDiff();
}

The total runtime here is ~90s, even on subsequent runs when AutoVerify() is removed. However, removing the call to Verifier.Verify(dateString) drops the total runtime to ~350ms. (Obviously we would expect some Verifier to add some extra weight, but this is quite a lot.)

Combining the test case results before calling Verify cuts the time down to ~800ms.

[Test]
public Task Test()
{
    var result = Enumerable.Range(0, 3600).Select(i => i * 2);
    return Verifier.Verify(result)
                 .UseParameters(1)
                 .UseDirectory("snaps")
                 .AutoVerify()
                 .DisableDiff();
}

There is clearly something very inefficient in running large quantities of Tests that call Verify. So while I can't yet be 100% sure that the File IO is the culprit, it does seem the most likely candidate.

AvremelM commented 2 years ago

(The 3600 cases is just what I had for the actual tests I was running. For testing/profiling it might be better to use 1000 or something, which still shows the problem, but won't waste as much of your time)

SimonCropp commented 2 years ago

does nunint run each test case sequentially? or are they all run in parallel?

the idea is that there would only be the overhead of opening, reading (and writing) 1 file once, instead of 3600 files once.

but how do you achieve that? nunit runs tests cases in isolation. there is no shared context.

even assuming verify can track the start of the first test case, how would i know that the last test cas has run and the file should be flushed?

you can even run one test case and not the others, so in that case how would Verify know that this is a single test run and not "user has reduced the number of scenarios

AvremelM commented 2 years ago

I believe they are run asynchronously, but only in parallel if specifically requested. I don't think it'd be an issue either way, see below.

but how do you achieve that? nunit runs tests cases in isolation. there is no shared context.

I did it manually by defining a [OneTimeSetUp] method that loads a file, deserializes it, and stores it in a static Dictionary field. The tests then use that dictionary to look up the expected result for each test case. I didn't handle updating the snapshot, though that could presumably be done via a separate "received" Dictionary and [OneTimeTearDown].

This works fine with NUnit, even when running just one case.

even assuming verify can track the start of the first test case, how would i know that the last test cas has run and the file should be flushed?

[OneTimeTearDown]. Most test frameworks have similar functionality.

you can even run one test case and not the others, so in that case how would Verify know that this is a single test run and not "user has reduced the number of scenarios

Verify wouldn't need to know, per se. By loading the combined snapshot once before any test run, Verify would always have the full results loaded, and it would know which case/s were modified because of .UseParameters(...).

SimonCropp commented 2 years ago

ok. i am happy to consider a PR that implements this feature

SimonCropp commented 2 years ago

how you going with that PR?

SimonCropp commented 2 years ago

what is your hardware? on my machine your scenario (below) runs in 1-7 seconds

private static IEnumerable<int> testCases = Enumerable.Range(0, 3600);

[TestCaseSource(nameof(testCases))]
public Task Test(int testCase)
{
    var result = testCase * 2;  
    return Verifier.Verify(result)
                 .UseParameters(testCase)
                 .UseDirectory("snaps")
                 .AutoVerify()
                 .DisableDiff();
}
SimonCropp commented 2 years ago

image

AvremelM commented 2 years ago

1) I honestly haven't had time to work on PR. I'm also not sure my solution is the most ideal. It occurred to me that maybe letting the user provide their own read/write persistence would be a more robust/generalizable improvement.

2) Huh. My hardware really shouldn't be the issue, but maybe my environment is? I'm using Rider, but I'll try VS and console runner tomorrow and see if that makes a difference.

AvremelM commented 2 years ago

Tried VS2019, VS2022, Rider, and running dotnet test and vstest.console.exe in the command line. Tried targeting both netcoreapp3.1 and net6.0.

Hardware: CPU: Core i7-6700 (@ 3.40GHz) RAM: 64GB (@ 2133MHz) SSD: Samsung 860 EVO

image

SimonCropp commented 2 years ago

i think a better approach for something like this is

  record TestResult(int input, int result);
    [Test]
    public Task Test()
    {
        var list = new List<TestResult>();
        foreach (var input in Enumerable.Range(0, 3600))
        {
            var result = input * 2;
            list.Add(new (input, result));
        }

        return Verify(list).UseDirectory("snaps");
    }
SimonCropp commented 2 years ago

the above takes me (on current machine) from 12sec to 350ms

SimonCropp commented 2 years ago

@AvremelM does the above work for you? or are you work on a PR to add your single file approach?

AvremelM commented 2 years ago

I mentioned that approach above.

Describe alternatives considered I tried just combining all the TestCases into one result object before calling .Verify(allCaseResults). This works to reduce total runtime, but it sacrifices all the benefits of having individual Test Cases, and makes determining which case caused a failure very time-consuming.

Re PR, it looks like the changes required would be quite extensive, and I unfortunately don't have the time (not even sure I have the skill). For now, I've stopped using Verify for these tests. (And this feature request can be closed, as far as I'm concerned).

In any case, I wonder if the more generic approach of allowing custom persistence (i.e., exposing and abstracting IoHelpers to allow overriding) could be simpler and allow for more broad extensibility.

Do you have any thoughts on that?

SimonCropp commented 2 years ago

i dont know how an abstraction over IoHelpers would help. most of that file is about abstracting over the fact that some APIs dont exists in older TFMs and to have common encoding. i dont see how having an abstraction over that api would help you

AvremelM commented 2 years ago

I don't necessarily mean IoHelpers itself; it's just the most centralized class right now for actual persistence. OpenWrite or WriteText methods accept a filepath which is just a key for the snapshot's value.

Essentially, if you can specify your own persistence methods, you can reuse files (#478), merge snapshots via custom logic (#468), store snapshots in a DB instead of the filesystem, use a virtual filesystem, etc etc.

It's a much more useful refactor, in theory.

SimonCropp commented 2 years ago

it is interesting in theory. but IMO i dont think there is the ROI there to justify it. will close this for now. thanks for all the input