Make the MsTest source generator emit an override modifier when the base class has a TestContext property

[m-ringler]

When a base class of the TestClass already has a TestContext property this PR makes the source generator emit an override keyword on the TestContext property that it generates.

• When the base class property is virtual, the resulting code will compile.
• When the base class property is non-virtual, this will lead to a CS0506 compiler error (instead of CS0108). I think that is the more helpful behavior, as we now show the developer how to solve the problem.

The behavior is unchanged when a TestContext property is declared in the TestClass itself.

Fixes #1437

[m-ringler]

@dotnet-policy-service agree company="Zeiss"

[SimonCropp]

@MattKotsenas got time to do a review?

[MattKotsenas]

This looks fine to me, however supporting this scenario brings up caveats that are worth thinking through.

Consider code like this:

public class BaseClass
{
    protected TestContext _testContext = null!;
    public virtual TestContext TestContext { get { return _testContext; } set { _testContext = value; } }
}

[TestClass]
[UsesVerify]
public partial class TestClass : BaseClass
{
}

BaseClass might expect that _testContext is set by inheritors, however our partial method won't know to do that. So, if BaseClass only ever uses the property, things are fine. But if it uses the field, things will be broken.

This gets more confusing if you add a third class to the mix:

[TestClass]
public class TestClass2 : TestClass
{
    public override TestContext TestContext { get; set; }
}

without the additional TestContext property this inheritence hierarchy works fine. Prior to this change, adding the override results in CS0506. After this change, it compiles but fails at runtime with System.Exception: TestContext is null. Ensure test class has a [UsesVerify]attribute (or inherits fromVerifyBase).

It's probably on balance an OK change to take, as I doubt many people are doing tricky things with the TestContext, but it's perhaps worth calling out in the docs?

[m-ringler]

We could mitigate the 1st scenario by also calling the base setter in the generated setter.

We could mitigate the 2nd scenario by sealing the overridden TestContext property.

What do you think?

[SimonCropp]

@MattKotsenas thoughts?

[MattKotsenas]

I didn't realize you could mark a single method as sealed!

Both seem like good additions to prevent accidental misuse.

@m-ringler , are you willing to try adding both to this PR?

[m-ringler]

Yes, I will.

[m-ringler]

@MattKotsenas, @SimonCropp
I've made the two changes we discussed.
I've also updated the VerifyGenerator method to check that there are no diagnostics when expectedDiagnostics is null or empty.

[MattKotsenas]

LGTM. Left 1 item for code style, but no other concerns from my end. Thanks for this!

[MattKotsenas]

nit: I think this is the only place in the codebase to use the query-style syntax. It's up to @SimonCropp but I'd assume you want method-style for consistency.

[SimonCropp]

i do prefer method-style. but i can fix that after merge

[SimonCropp]

@m-ringler thanks for this. i will deploy it now