Why dropping composite message formatting support also for Assert.Pass/Fail/... where there are no actual and constraint parameters?

[maettu-this]

NUnit2050 tells me to replace composite message formatting by a simple string literal (not possible where variable values need to be injected) or an interpolatable string (would be possible but adds too much noise as variable names are much longer than {0} or {1}).

I fully understand the benefits of using CallerArgumentExpression for Assert.That and Assume.That. But why also for...

Assert.Pass
Assert.Fail
Assert.Warn
Assert.Ignore
Assert.Inconclusive

...where there neither is a condition nor actual/constraint parameters?

[OsirisTerje]

Where do you see there is a CallerArgumentExpresson for these? E.g.: Pass only take a string, no caller expression there.
Is it in the documentation? If so, that is in error.

[stevenaw]

@OsirisTerje I'm not at a computer but I think I see what @maettu-this is referencing. There is a table in the markdown file linked which includes those other methods.

This is likely a documentation issue as the methods themselves take basic, undecorated strings. I'm unsure if it's just in that one place we need to update

[OsirisTerje]

My bad for not looking up the link there . Good you caught that @stevenaw , and thanks @maettu-this for finding this.
I'll move this issue over to the analyzer.

[manfred-brands]

The Analyzer warns that because the NUnit 4 code no longer has the params overload. NUnit 3 did. It is part of the NUnit3 -> NUnit 4 migration path. So the analyzer and documentation matches the NUnit 4 code base.

These params overloads were removed as part of nunit/nunit#4419 as a pre-cursor to allow [CallerArgumentExpression].
I agree with @maettu-this that we were a bit too zealous and also removed overloads that don't need that and are also always unconditionally formatted.

However I'm not sure if we should re-instate these in NUnit and then update the analyzer or for consistency sake leave it as it is so we have consistency and only support InterpolatedString. Otherwise we might get questions of why can I use params for Assert.Warn but not for Assert.That?

@OsirisTerje Your opinion?

[OsirisTerje]

@manfred-brands I am also unsure here.

The reporter is correct we have an inconsistency here.

The reporter seems to hold the opinion that it should stay as strings, but I don't share that an interpolated string is more noisy than an interpolated string, rather the opposite:

($"This is {givenname}, my last name is {surname}")

versus

("This is {0}, my last name is {1}",givenname,surname)

On the other side, we haven't had anyone complaining about this (ordinary strings here, interpolated otherwise), so I don't think people really notice.

Four of these asserts are also in their own group in the docs (Special Assertions) and Warn is on its own. The keyword Warn ( used for Warn.If andWarn.Unless use CAE.

For just these with only strings, a CAE doesn't (at first) seem to add anything, except for the fact that the output message tells where the message comes from, they don't do that now, but all functions where we have CAE does that, including the classic asserts *since they re/route to the constraint ones.

Writing a

Assert.Warn("There is something strange here");

ends with an output like this:

versus if we use something with CAE:

Warn.If(MyValue != 42, "There is something strange here");

ends up like this, which imho is better:

PS: The use of negative logic here makes the example a bit harder, but you see the gist of it.

So... after this little "walk inside my thoughts", I think I would prefer it to use an interpolated string and adding CAE. :-)

[maettu-this]

Let me add three pieces to consider:

the "master" of the message

For Assert.That|Assume or If, NUnit is the message "master", in most cases the user doesn't need to add any additional message, the NUnit standard message is perfect.

For the others like Assert.Fail or Assert.Inconclusive, the developer is the message "master", the developer even is obliged to create the message.

conditional vs. unconditional

The above shown Warn.If is conditional, same as Assert.That, i.e. CAE makes sense. But I have multiple cases where an Assert.Inconclusive or Assert.Fail is unconditional, because more complex test logic has already determined the condition. The message is used to easier locate the root cause of a failed test run, e.g. a non-responding device.

composite message formatting vs. interpolated strings

don't share that an interpolated string is more noisy than an interpolated string

Many may agree for the stated example, but I personally prefer composite message formatting especially in case of formatting numbers, e.g.:

Assert.Inconclusive("Test skipped because buffer size {0} is not supported by VSPD.", outputBufferSize);
Assert.Fail("Count of {0} exceeds the capabilities of this test!", someCollection.Count());

vs.

Assert.Inconclusive("Test skipped because buffer size {outputBufferSize} is not supported by VSPD.");
Assert.Fail("Count of {someCollection.Count()} exceeds the capabilities of this test!");

With composite message formatting...

• ...the main statement of the messages is easier to catch, e.g. "buffer size is not supported by VSPD".
• ...there is less noise within the message, the noise is moved to the far end of it.

But maybe this is just personal taste.

[OsirisTerje]

@stevenaw wrote the following about SpecFlow and their NUnit integration:

Reports on the SpecFlow forums show that NUnit 4 "just works" despite Specflow being compiled against NUnit 3.13.1: https://support.specflow.org/hc/en-us/community/posts/12618091291292-NUnit-4-support

From what I can see, this is because they primarily integrate on the assertion side by only using Assert.Inconclusive(string) and Assert.Ignore(string), neither of which was moved out into ClassicAsserts. https://github.com/SpecFlowOSS/SpecFlow/blob/master/Plugins/TechTalk.SpecFlow.NUnit.SpecFlowPlugin/NUnitRuntimeProvider.cs#L15C13-L20

This means we should be very careful and only change this after talking with the SpecFlow guys. We don't want to break them.

Which again means: Documentation should be updated.

[stevenaw]

Thanks for cross-posting and sharing this here @OsirisTerje Agreed, we should be careful with those methods here.

I'm unsure if there's a proposed direction here yet but I think we should be mindful of backwards compatibility in general here too as it sounds like we're considering changing or adding method signatures in the framework, which would be released as a semver-minor version bump.

[OsirisTerje]

Agree. Perhaps we should add a v5 branch, and merge all breaking changes in there.

[manfred-brands]

Agree. Perhaps we should add a v5 branch, and merge all breaking changes in there.

We should first decide if these changes are worthwhile enough to break compatibility or reject them and don't do them.

[OsirisTerje]

Sure :-)

Imho, I'm leaning on keeping these as they are. If we do so, the 2050 rule should not trigger on these, and documentation updated.

Comments: @nunit/framework-team @nunit/analyzers-team

[mikkelbu]

I think I also lean towards keeping these as they are.

[stevenaw]

No strong opinions on my end either way.
I tend to err on the side of caution and compatibility when it comes to signature changes or additions. No change is, of course, safest of all.

[manfred-brands]

It seem we are in agreement to not change this again in NUnit which would require another major version change.

This means that the analyzer is correct that it warns for Assert.Warn etc. when compiled against NUnit 3 as NUnit 4 no longer supports any params overloads.

[maettu-this]

@OsirisTerje and @manfred-brands I am a bit confused now, as I understood...

If we do so, the 2050 rule should not trigger on these, and documentation updated.

...to lean towards not breaking compatibility against NUnit 3, i.e. reintroduce the params overload for those Assert that only take a single string argument. But maybe I have misinterpreted the statements and they actually meant not breaking compatibility against NUnit 4.

But even if not reintroducing the params overload in NUnit, I think the documentation of NUnit2050 should be refined. Today's...

interpreted as the actual and constraint expression strings

...just doesn't make sense for those Assert that don't have actual and constraint expressions.

[manfred-brands]

interpreted as the actual and constraint expression strings

...just doesn't make sense for those Assert that don't have actual and constraint expressions.

Correct. The table could be move higher to refer to method that used to have params overloads.

The special section is only relevant for Assert.That with 3 string arguments:

[TestCase("NUnit 4", "NUnit 3")]
public void TestMessage(string actual, string expected)
{
    Assert.That(actual, Is.EqualTo(expected), "Expected '{0}', but got: '{1}'", expected, actual);
}