Correctly handle the existence of TestContext in base class

[Youssef1313]

Fixes #1482

[MattKotsenas]

I don't think this is a safe change to do?

It suppresses the compiler warning, but now there may be two different properties, one that has the necessary machinery to hook the setter, and one that doesn't.

I think this could lead to Verify not seeing the correct TestContext if anything we're to interact with the base class?

[Youssef1313]

@MattKotsenas There are two different properties, yes. One in base, and we shadow it in source-generated code generated by Verify. While shadowing is generally discouraged, I don't see much of an issue in that in this specific case.

The property generated by Verify is what MSTest will be dealing with, and the generated property is always setting the base one, so both properties are having the same value.

The only scenario where both properties can get out-of-sync is if something is setting the base property. But as TestContext property is only ever intended to be set by MSTest itself, there is not much of a concern in this case.

[MattKotsenas]

I'll leave it to @SimonCropp if he wants to accept the change as project owner.

I'd still suggest against it, as though @Youssef1313 is correct about how things should be, Verify will be at the mercy of all base classes behaving properly, and this change removes the only warning we currently have, which will make debugging these types of issues more difficult.

Instead I'd suggest either of the other two threads listed in the bug: fixing the Playwright tests as a targeted fix, or introducing the AsyncLocal to MSTest to sidestep the issue entirely.

However, if there's something I'm missing, please let me know!

[Youssef1313]

For MSTest, I'm definitely interested in introducing a Current property. But I'm not sure if it's going to happen soon.

Another alternative for Playwright is to rely on constructor injection, but that forces all test classes to have a constructor injecting the TestContext to base.
Or, alternatively, MSTest could allow TestContext parameter to TestCleanup methods (that's where Playwright needs to access TestContext), but I'm not sure if it's a good idea to do so.

[MattKotsenas]

@Youssef1313 , understood that the .Current property will likely take time. Isn't microsoft/playwright-dotnet#3210 another alternative for Playwright that's non-breaking and allows this scenario?

[SimonCropp]

@Youssef1313

For MSTest, I'm definitely interested in introducing a Current property. But I'm not sure if it's going to happen soon.

why? from my, perhaps naive, perspective, this should be fairly trivial thing to add?

could it perhaps be added faster as a [Experimental] feature?

[Youssef1313]

why? from my, perhaps naive, perspective, this should be fairly trivial thing to add?

Well, it's trivial thing to add and we already have the API internal. But we are already not very comfortable with TestContext API altogether, and we need to evaluate what our future vision for TestContext will be, and how adding such API will impact it. For example, we would like to start introducing new TestContext types at some point, where each TestContext type will be specific to a "stage" in the test lifecycle (e.g, assembly initialization, class initialization, test run, class cleanup, and assembly cleanup). The idea is that the TestContext of assembly initialization shouldn't have any properties that are irrelevant to assembly initialization. Today, TestContext passed to AssemblyInitialize has access to TestName (which is irrelevant to assembly initialization). Moreover, it doesn't throw today and it returns the name of the first test that will be run (in v4 we are changing it to throw).

So far, we don't yet have any concrete design for the specialized TestContext types we would like to introduce, and thus it's hard to know how much TestContext.Current API is going to have an impact. I'm guessing it's unlikely to have any big impact if we are going to leave the old TestContext for backcompat anyways, but still we need to be careful :)

Another thing is that I want to allow some time (and do more manual testing) around the propagation of ExecutionContext, which we have changed multiple times to fix some reported bugs.

[SimonCropp]

closing this in fav of this microsoft/playwright-dotnet#3210