Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue4413 caller argument expression #4430

Merged
merged 14 commits into from Jul 24, 2023

Conversation

manfred-brands
Copy link
Member

@manfred-brands manfred-brands commented Jul 22, 2023

@OsirisTerje I see a problem, the compiler cannot distinguish between:

Assert.That(actual, constraint, [CAE] string actualExpression =  "", [CAE] string constraintExpression =  "")
{}
Assert.That(actual, constraint, string? message, [CAE] string actualExpression =  "", [CAE] string constraintExpression = "")
{}

When calling it like:

Assert.That(actual, constraint, message)

Because it matches both.

@manfred-brands
Copy link
Member Author

See here:
image

It thinks we passed an actualMessage not a message.

@manfred-brands manfred-brands force-pushed the Issue4413_CallerArgumentExpression branch from 2ceeee2 to 1f54e70 Compare July 22, 2023 11:20
@OsirisTerje
Copy link
Member

Got the same. I changed the nullable message strings into non-null. That sølvet it. Suggest you merge my cae branch into your. I can merge back again tomorrow to get your changes in.

@OsirisTerje
Copy link
Member

@manfred-brands Partially merged this into the CallerArgumentExpression branch, PR #4427

We should handle conditional formatting as an addition afterwards. I am not sure why we actually need it btw, but perhaps some discussion I have missed?

There are no overloads that don't compile now.

@manfred-brands
Copy link
Member Author

manfred-brands commented Jul 23, 2023

We should handle conditional formatting as an addition afterwards. I am not sure why we actually need it btw, but perhaps some discussion I have missed?

String formatting is a very expensive operation. Doing this for every unit tests will make tests run much slower.
I created a benchmark test quantifying this, comparing 3 cases:

  • NUnit3's: format and params arguments: UseFormatAndArguments
  • NUnit4's: string using $"" syntax: UsePreformattedMessage
  • PRs: FormattableString using $"" syntax: UseFormattableMessage

On tests that never fail, the pre-formatted option is 6x slower (net7.0) or even 22x slower (net48).
The FormattableString is still 2x slower than the message and arguments, because it needs to allocate the FormattableString instance.
I don't know what in real world the amount of tests is that pass in extra arguments.
From our main applications 209978 Assert calls only 445 use a format string ({0}) and 370 use a interpolated string ($"").

|                 Method |            Runtime |       Mean | Ratio | Allocated | Alloc Ratio |
|----------------------- |------------------- |-----------:|------:|----------:|------------:|
|  UseFormatAndArguments |           .NET 7.0 |   4.306 ns |  1.00 |      56 B |        1.00 |
| UsePreformattedMessage |           .NET 7.0 |  26.917 ns |  6.25 |     104 B |        1.86 |
|  UseFormattableMessage |           .NET 7.0 |   9.198 ns |  2.14 |      88 B |        1.57 |
|                        |                    |            |       |           |             |
|  UseFormatAndArguments | .NET Framework 4.8 |   5.954 ns |  1.00 |      56 B |        1.00 |
| UsePreformattedMessage | .NET Framework 4.8 | 134.632 ns | 22.62 |     160 B |        2.86 |
|  UseFormattableMessage | .NET Framework 4.8 |  12.593 ns |  2.12 |      88 B |        1.57 |

AssertPerformance.zip

@manfred-brands manfred-brands force-pushed the Issue4413_CallerArgumentExpression branch from 38191ea to 7f5dd54 Compare July 24, 2023 02:59
@manfred-brands manfred-brands marked this pull request as ready for review July 24, 2023 03:29
@manfred-brands
Copy link
Member Author

@OsirisTerje Once I noticed that you removed the overloads with no message parameter, I could finish mine.
Seeing what you did, I fixed the nunit test asserting message contains from 'Is.EqualTo' into 'Does.Contain'.
I added nunit tests to assert that the message contains the actual and constraint expressions.

Note that there is no conditional on nunit462/net6.0. the CAE works for all TargetFrameworks.

@manfred-brands manfred-brands force-pushed the Issue4413_CallerArgumentExpression branch from 61d678a to c1cb2bf Compare July 24, 2023 03:36
@manfred-brands manfred-brands changed the title WIP: Issue4413 caller argument expression Issue4413 caller argument expression Jul 24, 2023
@OsirisTerje OsirisTerje merged commit 04f772b into master Jul 24, 2023
6 checks passed
@OsirisTerje OsirisTerje deleted the Issue4413_CallerArgumentExpression branch July 24, 2023 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants