Use source generator instead of base class for MSTest

[MattKotsenas]

Fixes #1184

Introduce an attribute [UsesVerify] for MSTest so that tests can set up Verify without needing to use the VerifyBase class. Relaxing the base class requirement makes it easier to use Verify in more cases, where other test scenarios may also require using a base class.

The attribute triggers a source generator that adds the required TestContext property. The setter then stashes the context on an async local for use by the Verifier.

Work remaining

• [ ] Write the actual source generator
• [ ] Follow up on DerivePathInfo types / method name
• [ ] Determine strategy if the host test already defines a TestContext property
• [ ] Determine which unit tests to add to prevent VerifyBase regressions until it's removed
• [ ] Migrate tests to source generator

[MattKotsenas]

I haven't forgotten about this. I got pulled into since stuff at work and have to put this on hold for another week or two.

[SimonCropp]

i tried to build and got

UsesVerify.g.cs(16,19): Error CS0542 : 'VerifyBase': member names cannot be the same as their enclosing type
16>UsesVerify.g.cs(18,28): Error CS0548 : 'VerifyBase.VerifyBase.TestContext': property or indexer must have at least one accessor
16>CSC: Error CS8785 : Generator 'UsesVerifyGenerator' failed to generate source. It will not contribute to the output and compilation errors may occur as a result. Exception was of type 'ArgumentOutOfRangeException' with message 'repeatCount ('-4') must be a non-negative value. (Parameter 'repeatCount')
1

note i am using Rider. Any ideas?

[SimonCropp]

This PR is big and (in my opinion) difficult to review; do you want me to split it up into preparatory refactoring changes?

no need to split. it is a big PR. but many of the changes are not significant

Agree on reflection / strategy for finding test Type and MethodInfo

happy with reflection. can UnsafeAccessor with a #if be used?

Agree on if we want to allow hooking the TestContext setter

can you expand on this

Agree on using ForAttributeWithMetadataName, which limits users to .NET 7 SDK+ (but can still use any supported TFM). Where should this be documented?

I will include .NET 7 SDK+ in the main doco. so u dont need to add it as part of this PR

Are there other scenarios that should have VerifyBase tests?

IMO the current tests are sufficient

[SimonCropp]

ignore my build error. a restart of rider fixed it

[MattKotsenas]

ignore my build error. a restart of rider fixed it

Yeah, this is a downside of using a generator (or analyzer) as a ProjectReference; it's loaded into the build server and thus old versions can be cached, see https://github.com/dotnet/roslyn/issues/48083

These problems should only happen the first time the generator is added to a project and won't impact users since they won't be changing/building the generator as part of their build.

[MattKotsenas]

Regarding using UnsafeAccessor instead of traditional reflection:

Not possible today because the return type of _testMethod is Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.ObjectModel.TestMethod, which is technically public, but it's in the MSTest.TestAdapter package in the build/ directory (not the lib/ directory), so as far as I know it's "unspeakable".

We need a new API like https://github.com/dotnet/runtime/issues/90081 to be able to express the return type of the _testMethod field, and the type of "this" for the AssemblyName property.

I'll update the code with a comment to make this clear and so that if/when those APIs land we can update more easily.

[MattKotsenas]

Regarding hooking the TestContext: I think this is a case where I was overcomplicating things.

In theory a user could have a test setup where they also want to do some custom action when the TestContext was set. Let's say they had code like this:

private TestContext _localField;

public TestContext TestContext
{
    get { return _localField; }
    set
    {
        Console.WriteLine(value);
        _localField = value;
    }
}

because the generator needs to write this property to set Verifier.CurrentTestContext.Value, there's no easy way for a user to hook the set.

We could support this scenario one of two ways:

1. Updating our source generation to look more like this

public TestContext TestContext
{ 
    get;
    set
    {
        CurrentTestContext.Value = value;
        VerifyOnSetTestContext(value);
    }

    private virtual void VerifyOnSetTestContext(TestContext context)
    {
    }
}

and the user could implement public override void VerifyOnSetTestContext(TestContext context) => Console.WriteLine(context);

2. We could document Verifier.CurrentTestContext and tell the user to instead of using [UsesVerify] to set the async local themselves

No existing code using VerifyBase could be in this situation since the property was defined on the base.

So in summary I'm not sure this is a use case worth supporting without someone asking. My recommendation would be to keep the source generator as-is. If you think this is a scenario worth supporting (or someone asks for it) I'd prefer option 1 over option 2 since it provides a cleaner contract with the user.

Let me know if I'm still not making sense :)

[MattKotsenas]

OK, I think I'm good with the changes here. Let me know if there's anything else from your side.

Thanks for the review and all the help!

[SimonCropp]

So in summary I'm not sure this is a use case worth supporting without someone asking. My recommendation would be to keep the source generator as-is. If you think this is a scenario worth supporting (or someone asks for it) I'd prefer option 1 over option 2 since it provides a cleaner contract with the user.

ok. happy with leaving as is for now.

head up: I am going to push some minor cleanups

[MattKotsenas]

OK, all comments addressed. The two items to specifically take a look at are:

1. Take a look at the note around IndentedStringBuilder allocations
2. Let me know if you agree / disagree on the TestContext reflection choice

In all other cases I think I took your recommendations / suggestions. If you have any other feedback or thoughts, please let me know. Thanks!