Issue4415

[OsirisTerje]

Removing params from framework, and handle params in classic.

See #4415

Note: This PR builds on top of the PR #4417 . See commits after #ccd58ab

[manfred-brands]

@OsirisTerje It would have been nicer if you had made a PR to merge into Issue4416 iso master.

Observations:

• CollectionAssert should derive from AssertBase and not Framework.Assert.
• In various overrides you removed the null parameter for the params args. This cause the compiler to pass an empty array as args and therefore missing the optimization in ConvertMessageWithArgs to not call Format. This again can cause an exception if the message parameter contains a {...} value that not has a matching argument or is not even a number. This can be partly fixed by changing the condition in ConvertMessageWithArgs into (args is null || args.Length == 0). But passing null is more efficient. But see next point.
• In the classic versions with no argument you pass in "". This can be removed as there is a That without arguments.
• In some classic versions that call other classic versions you called ConvertMessageWithArgs prematurely resulting in it being called twice.
• In framework versions you now pass in "" iso null. As SA1122 says this means the compiler creates lots of embedded empty strings.
• You forget to remove params from Assert.ThatAsync.
• You probably wanted that analyzer fix to convert format specification strings to formattablestrings.

I think I have addressed all of the above in my review commit.

One remark is regarding optimization. Assuming NUnit test pass in 99% of the cases, the string will be formatted unnecessary. And that operation is quite expensive. This is not only the case for the classic versions with param args but also for FormattableString which by default is formatted before passing to the Assert.That.
The first we can fix by using the overload with Func<string?> getExceptionMessage parameter. Addressed in 2nd commit.

The latter can be fixed by adding an overload accepting DefaultInterpolatedStringHandler but this requires .NET6.0
I think it is a worthwhile addition, but means we have to add a NET6.0 build (and possibly drop netstandard2.0)
I'm happy to add that if you agree.

[OsirisTerje]

@manfred-brands Awesome - thanks a lot for the comments and the changes !!
I'll change it to merge to 4416.

[OsirisTerje]

@manfred-brands

The latter can be fixed by adding an overload accepting DefaultInterpolatedStringHandler but this requires .NET6.0
I think it is a worthwhile addition, but means we have to add a NET6.0 build (and possibly drop netstandard2.0)
I'm happy to add that if you agree.

I like the DefaultInterpolatedStringHandler . What if we add the Net6 build, but for now keep the netstandard 2.0 and add a compiler directive for the code for netstd 2. ? That way we can keep the netcore3.1 in play a little bit longer. (Would have loved some metrics on the framework usage though...)
We need a compiler directive switch there anyway to keep net462 in play.

[OsirisTerje]

@manfred-brands
I just did the same (removed params) for the Assume and Warn classes, and removed and fixed corresponding tests.

If you have time, (after the DefaultInterpolatedStringHandler ) you could start up with the CallerArgumentExpression.
We have the issues #4413 and older #3936. I tested out the CAE in the experimental code there.

We should consider the sequence of the messages, we have 3 types, user added message, result message and then CAE message. I believe the sequence could be like that too, or possible CAE message as 2nd.

[stevenaw]

Still on vacation but I've been following from my phone.

I like the NET6 target as it would also allow for many more internal optimizations by allowing us to use more newer span-based APIs when possible, among other newer performance-based APIs. In there past one noted concern was keeping on top of which newer runtimes to target as the framework has historically had a slower release cadence than the newer .NET LTS schedule but I think we've agreed in another issue that the quicker framework release cadence we're aiming for can help here 🙂

[manfred-brands]

I started on the .NET6 target. Have to work through some Obsolete and non-available APIs.
That is the problem with NetStandard. It compiles fine, but results in PlatformNotSupported exceptions at runtime. We do catch those, but for later targets I have to make the calls conditional.
Create #4420 on top of this branch.

@stevenaw Once we have the build in place we can see what optimizations we can apply.

[OsirisTerje]

Merging this since #4420 handles net6 target