testfx icon indicating copy to clipboard operation
testfx copied to clipboard

Suggestion: Issue diagnostic for IsNull/IsNotNull when called with non-nullable type

Open bjornhellander opened this issue 1 year ago • 1 comments

Would be nice with a diagnostic when Assert.IsNotNull and Assert.IsNull is called with a value type other than Nullable<T>, since those asserts will either always pass or always fail.

AB#2112219

bjornhellander avatar Jun 15 '24 09:06 bjornhellander

Hi @bjornhellander! Thanks for the suggestion, this would be valuable, adding for our next milestone.

Evangelink avatar Jun 16 '24 18:06 Evangelink

I see that a pull request has been created, but I wonder if we could discuss the way forward briefly first, unless you have already discussed this internally?

Since I created this issue, I have discovered MSTEST0025 which triggers on various calls which will always fail. One way of handling this issue could be to 1) update MSTEST0025 to also check for calls to IsNull with a value type other than Nullable<T> and 2) create a new analyzer that checks various calls that will always pass, including calls to IsNotNull with a value type other than Nullable<T>.

Another way could be to promote MSTEST0025 to detect any assert call with a pre-determined outcome, regardless if they always pass or always fail.

Or do you think that creating a brand new analyzer specifically for IsNull and IsNotNull is the way to go for this?

I created the issue after refactoring some code, probably changing some field or property from Nullable<T> to T. I see this as the main situation that these diagnostics would be helpful in detecting. It feels like the purpose of MSTEST0025 is different. So I understand completely if you want to create a new analyzer, but just wanted to check if you had discussed any alternative options.

bjornhellander avatar Jul 07 '24 16:07 bjornhellander

Hi @bjornhellander,

Thanks for the comment! Indeed, I went too fast!

Based on your suggestions and after a quick chat with the team, I think we will move forward with:

  1. Update MSTEST0025 to handle the always failing part of Assert.IsNull and Assert.IsNotNull. We may want to change the suggestion because replacing with Assert.Fail seems to be only a small fraction of the reason.

  2. Create a new rule for handling cases where we have assertions that are always passing (e.g. Assert.IsTrue(true), Assert.AreEqual(myVar, myVar), Assert.IsNull(myNonNullableStruct)...)

Evangelink avatar Jul 08 '24 09:07 Evangelink