[Feature Request] Attach .received. files to test context for MSTest

Question

[Feature Request] Attach .received. files to test context for MSTest

MattKotsenas opened this issue 9 months ago · 23 comments

Is the feature request related to a problem

As stated in the getting started wizard, debugging test failures usually requires updating the build pipeline YAML to upload .received. files as logs / artifacts.

Describe the solution

MSTest has support for attaching "result files" (see https://learn.microsoft.com/en-us/dotnet/api/microsoft.visualstudio.testtools.unittesting.testcontext.addresultfile?view=visualstudiosdk-2022). It would be nice if failing tests attached the corresponding *.received.* file to make it easier to view / diff failures without needing to modify the build pipeline YAML.

The result is that the received file is available to view / download like this: https://learn.microsoft.com/en-us/azure/devops/pipelines/test/review-continuous-test-results-after-build?view=azure-devops#troubleshooting-data-for-test-failure

Describe alternatives considered

The alternative is likely do-nothing. Motivated people can modify their build pipelines to collect and upload received files on failure.

I'm happy to do the work here, but I'm not sure where this code best belongs. I think the idea would be to catch exceptions in the TestBase::Verify call, attach the file, and then re-throw, but there may be a better option.

If you have any questions / concerns, please let me know. Thanks!

Answer 1 · 2024-04-11T00:47:17.000Z

is there any value in doing this locally? ie should it only happen on a build server?

Answer 2 · 2024-04-11T01:11:23.000Z

It has some value when running locally. Specifically, in Visual Studio the test details pane links to the received file like this:

which can be nice, especially when people are newer to Verify and may not have a diff tool set up yet.

VS Code doesn't appear to show the attachment in its test results view. I don't use Rider so I don't know there.

I'm interested in what you're considering / concerned about regarding doing it in all cases. If there's not a compelling reason to make CI behave differently, I'd like to have local behave the same just to minimize "works on my machine".

That said, one downside I can see to this behavior would be if someone:

has very large (likely binary) received files that are now being saved along with the test result files
they weren't already using the suggested YAML to save received files as artifacts

that could result in increased artifact sizes for that user.

I could imagine adding a setting to control the behavior, and if we want to limit it to CI scenarios use a trick like AssemblyMetadataAttribute to smuggle the value from $(ContinuousIntegrationBuild) to set the default.

Thoughts?

Answer 3 · 2024-04-11T02:19:57.000Z

ok in that case we should do it in all cases

I'm interested in what you're considering / concerned about regarding doing it in all cases

i am only asking since we need to make a decision about when autoverify is enabled https://github.com/VerifyTests/Verify/blob/main/docs/verify-options.md#autoverify

and there is nuance about autoverify an CI

so given you will be the consumer. should we do the attachments when autoverify is on. since in that case it will not be a filure

Answer 4 · 2024-04-11T02:23:14.000Z

oh also. i assume u want both a "new content" (ie first failure) and a "diff content" (subsequent failure) to be attached?

Answer 5 · 2024-04-11T02:24:16.000Z

and do u care about deleted files? ie a test can produce multiple files. if on a subsequent run a diff set of files is produced, the redundant ones are cleaned up.

Answer 6 · 2024-04-11T04:29:30.000Z

so given you will be the consumer. should we do the attachments when autoverify is on. since in that case it will not be a filure

I think even if autoverify is on we should still attach the received file. My rationale is that the real feature is answering the question "what was the received file?" That question is usually followed by "why didn't it match the verified file?". In the autoverify case there's no question 2, but the attachment does still answer question 1, and thus the attachment still makes sense to me.

Answer 7 · 2024-04-11T04:31:25.000Z

oh also. i assume u want both a "new content" (ie first failure) and a "diff content" (subsequent failure) to be attached?

Yeah, I believe I do. I honestly didn't realize the tool differentiated between those two cases. I assume both types of failure can't happen for the same test case though, correct?

Answer 8 · 2024-04-11T04:41:33.000Z

and do u care about deleted files? ie a test can produce multiple files. if on a subsequent run a diff set of files is produced, the redundant ones are cleaned up.

I'm not exactly sure what should happen here. Does this example capture the scenario?

Before

File name	Contents
A.verified.txt	A
B.verified.txt	B

After

File name	Contents
A.verified.txt	A
A.received.txt	AA
C.received.txt	C

In this example I would expect to have A.received.txt and C.received.txt as attachments.

Assuming autoverify is off, does the failure exception mention that B has been deleted? If so then I'm happy with that outcome.

I don't need B.verified.txt because that's in source control. I assume there is no B.received.txt to attach because that would be ambiguous with an empty output.

Let me know if this is helpful, or if there are follow up questions. Thanks!

Answer 9 · 2024-04-11T05:52:24.000Z

Assuming autoverify is off, does the failure exception mention that B has been deleted? If so then I'm happy with that outcome.

its the opposite. autoverify is on then B.verified.txt wont exist on disk.

but note if autoverify is on then A.received.txt and C.received.txt will not exist on disk. they will be A.verified.txt and C.verified.txt on disk

i am not sure on the order of the callbacks and the act of autoverify. i will check.

but i think i understand your desired result

Answer 10 · 2024-04-11T12:10:43.000Z

can u try 24.0.0-beta.1

Answer 11 · 2024-04-12T00:40:47.000Z

Took a look today, and I think things mostly match what I would expect, with 1 possible change and I think 1 bug / issue.

Testing attachments

Here's what I tried:

Scenario	AutoVerify	Has attachment	Matches my mental model
Initial failure	On	Yes	⚠️
Initial failure	Off	Yes	✅
Subsequent diff	On	Yes	⚠️
Subsequent diff	Off	Yes	✅
Initial & subsequent diffs with multiple Targets	On	Multiple	⚠️
Initial & subsequent diffs with multiple Targets	Off	Multiple	✅
Deleting a Target	On	No	✅
Deleting a Target	Off	No	✅

⚠️: With AutoVerify I would still expect the .received. file to be attached rather than the .verified. file (source). I get that AutoVerify automatically promotes the received to verified, however that feels to me like a thing that happens after the mismatch, rather than as a part of the mismatch. At the same time, I will admit that I'm not an AutoVerify user, so perhaps my mental model is wrong.

IDisposable breaking change

Separately, I think there's a design issue with the IDisposable usage in VerifyBase (source). As-is, it doesn't implement the Dispose pattern, so if my unit test implements IDisposable itself, it will shadow VerifyBase::Dispose and the TestContext won't be cleared.

More importantly, making VerifyBase implement IDisposable would require anyone with a disposable test class to need to change their code by moving anything in public void Dispose() to the new protected virtual void Dispose(bool disposing), which may have further downstream effects. This feels like a potentially big breaking change.

I think a better solution to clean up the test context would be to use [TestCleanup], which I validated will work on a base class (though it runs before any Dispose calls). Here's a sample:

[TestClass]
public class UnitTest1 : Bar
{
    [TestMethod]
    public Task TestMethod1()
    {
        Assert.IsTrue(true);
        return Task.CompletedTask();
    }

    [TestCleanup]
    public void Cleanup()
    {
        Console.Write("cleanup1"); // Runs 1st
    }
}

public class Bar : Foo, IDisposable
{
    public void Dispose()
    {
        Console.Write("dispose"); // Runs 3rd
    }
}

public class Foo // this is the stand-in for VerifyBase
{
    [TestCleanup]
    public void BaseCleanup()
    {
        Console.Write("cleanup2"); // Runs 2nd
    }
}

Thoughts?

Answer 12 · 2024-04-12T01:50:57.000Z

With AutoVerify I would still expect the .received. file to be attached rather than the .verified. file (source). I get that AutoVerify automatically promotes the received to verified, however that feels to me like a thing that happens after the mismatch, rather than as a part of the mismatch. At the same time, I will admit that I'm not an AutoVerify user, so perhaps my mental model is wrong.

the problem is that the MSTest attachment code flow is delayed. so it stores a list of file paths in mem, then processes them at some point after the test has finished processing. so at that point, with AutoVerify, the received file no longer exists. so we cant add the received file. if MSTest attachment API added a way of "take a copy of this file when i add it" then we could do it. or if they added an api to name the attachment so it can be diff from the file name.

Is my understanding incorrect? do you see a better way forward?

happy to accept a PR that moved VerifyBase to leverage [TestCleanup]

Answer 13 · 2024-04-12T01:55:49.000Z

happy to accept a PR that moved VerifyBase to leverage [TestCleanup]

actually scratch that. i will just do it now

Answer 14 · 2024-04-12T02:04:55.000Z

the problem is that the MSTest attachment code flow is delayed. so it stores a list of file paths in mem, then processes them at some point after the test has finished processing. so at that point, with AutoVerify, the received file no longer exists. so we cant add the received file. if MSTest attachment API added a way of "take a copy of this file when i add it" then we could do it. or if they added an api to name the attachment so it can be diff from the file name.

Is my understanding incorrect? do you see a better way forward?

You're correct, it's deferred processing, and really just a path, so we'd need to change AutoVerify to in this case not delete the file, or otherwise make a copy. What you have now is probably fine. If a user is using AutoVerify they are probably also unlikely to be looking at the attachments (since the test will pass). If someone complains we can revisit.

happy to accept a PR that moved VerifyBase to leverage [TestCleanup]

I saw you mentioned you might do it yourself. Also wanted to state / let you know that I'm happy to make the change, but I won't have time until tomorrow. I meant it mostly as a warning that you probably want to fix before publishing 24 as stable.

Thanks again for all your help!

Answer 15 · 2024-04-12T05:21:35.000Z

Ugh, I see that the [TestCleanup] has to be public. That's probably fine for now, though I'd like to find a way to avoid mucking with the user's class.

If you're OK with the public cleanup change I'm OK with v24. If you don't want to ship it, I can revisit this feature as part of or after the base class removal. That might make this type of change easier. Up to you.

Edit: I've avoided AsyncLocal up until this point, so still getting up-to-speed, but do we need to clear the value? Looking at posts like https://til.cazzulino.com/dotnet/asynclocal-never-leaks-and-is-safe-for-callcontext-like-state and https://github.com/davidfowl/AspNetCoreDiagnosticScenarios/blob/master/AsyncGuidance.md#asynclocalt, it seems that assuming it's used correctly (i.e. as a static) it should automatically clear when the ExecutionContext is cleaned up. Thoughts?

Answer 16 · 2024-04-12T10:20:28.000Z

but do we need to clear the value

fair enough. i will remove the cleanup

and i will deploy a stable

Answer 17 · 2024-04-12T13:06:42.000Z

Does this PR mean that with v24, what's described in https://github.com/VerifyTests/Verify/blob/main/docs/build-server.md is no longer needed?

Answer 18 · 2024-04-12T19:01:01.000Z

I'll let @SimonCropp jump in and correct me, but that was my intention. If you're using MSTest and you're uploading your test results / TRX file, then you no longer need to edit your build pipeline / YAML.

I'm only an MSTest and xUnit v2 user, so I can't vouch for the other test frameworks. A quick look shows xUnit v2 doesn't have this feature (v3 will xunit/xunit#2457). NUnit appears to (https://docs.nunit.org/articles/nunit/writing-tests/TestContext.html#addtestattachment-37), so support could be added there. The other frameworks I'm not familiar with.

Answer 19 · 2024-04-12T21:28:20.000Z

@cmeeren yes that is the intent. i will update the docs shortly

Answer 20 · 2024-04-13T00:45:05.000Z

releasing nunit attachment support now. although i note appveyor does not detect attachments for mstest or nunit. @MattKotsenas what build server(s) did u test on?

Answer 21 · 2024-04-13T01:30:06.000Z

I tested on Azure DevOps, which is the place I use MSTest. All my GitHub actions pipelines happen to run xUnit.

I can set up a GitHub actions pipeline against MSTest to check there. Let me know if you'd like me to do that to do that, so that I don't duplicate effort.

Answer 22 · 2024-04-13T02:56:58.000Z

@MattKotsenas if you could test in GH actiosn that would be great. then i will make the docs reflect that

Answer 23 · 2024-04-13T07:36:28.000Z

Wasn't able to get GitHub actions to display in any of the popular report actions either. dorny/test-reporter@v1 states so in their docs Looking at the code for EnricoMi/publish-unit-test-result-action@v2, it seems they only expect result files as well.

On the bright side, that means if the user is using MSTest or NUnit and runs tests like this:

- task: DotNetCoreCLI@2
  inputs:
    command: 'test'

Or manually publishes test results like this:

- task: PublishTestResults@2
  inputs:
    testResultsFormat: 'VSTest'
    testResultsFiles: '**/*.trx'

they can omit the PublishBuildArtifacts and related steps.