MSTest v4 TestContext properties should not be nullable

[Mertsch]

Describe the bug

MSTest 3.0.0-preview-20221122-01 TestContext.TestName is nullable, which is technically correct, but makes no sense to me.

Expected behavior

"All" (those which make sense) Properties of TestContext should not be nullable and throw an exception if the underlying property dictionary has those properties removed or replaced by nulls.

Example: public virtual string TestName => GetProperty<string>("TestName") ?? throw new InvalidOperationException("Missing TestName");

Having nullable on all those properties just makes tests much more verbose. And when I expect a TestRunDirectory, havin null there is an exceptional case anyway. Plus if I really needed an optional TestRunDirectory I could retrieve it via GetProperty().

Actual behavior

https://github.com/microsoft/testfx/blob/8161dfbba093957e2cd1e6aabaf099a10e2915d3/src/TestFramework/TestFramework.Extensions/TestContext.cs#L122

[Evangelink]

Hi @Mertsch. I have changed labels to be an improvement because current implementation does return null sometimes but I agree with you that given my current knowledge of the codebase there is no good reason not to make it non-null and throw if property is not found in the bag.

Codebase needs a good cleanup and consolidation that we are planning to do.

[Mertsch]

I am not sure, but isn't string? breaking? We are getting null ref warnings all over the place for TestName which is new in 3.x compared to 2.x where we optimistically assumed those were NotNull.

If you plan on keeping the code as is shouldn't the class be excluded from nullable ref types feature. To not break existing customers. I personally would still be using v3 and just "bang" ! everything, but I guess there may be quite a few people annoyed by suddenly being confronted with compiler warnings which they may (like me) treat as errors.

[Evangelink]

That's a good suggestion. I need to double check if the well-known property could be somehow overridden and if not it might be safer to assume they are not null. Let me think about it.

[Evangelink]

I had a quick look and we need to do another good refactoring because having this abstract class uninitialized forces us to assume null values. Looks like some properties are also set later in the process, I am not sure if we could change the shape and time to ensure this is not null too.

We will work on this for MSTest v4 as you suggested, this would help signaling "breaking change" for users.

[Youssef1313]

Currently, TestName is always non-null. I think we can mark it non-null in a minor release. I don't see any breaking impact by that.

However is it actually correct that it's always non-null? Shouldn't TestName be null for AssemblyInitialize and ClassInitialize? Currently, the TestContext for these cases are attached to the "first" test running in the assembly, or the "first" test running in class. That probably isn't very well-defined if parallelization is enabled.

[Evangelink]

Summary of our offline discussion: Let's make non-nullable and introduce an (internal) way for us to recognize what's the "context" of the TestContext and throw an exception if it's used as part of Asm/Class Init/Cleanup contexts.