Visualstudio-docs: Assert.That used for assert extension violates this rule

[mateusz-dobrowolny]

Hi,

https://docs.microsoft.com/en-us/dotnet/api/microsoft.visualstudio.testtools.unittesting.assert.that?view=mstest-net-1.2.0

I wrote assert extension as designed by Microsoft.
Unfortunately, the 'assert' object is never used in the extension method, which violates this rule. I had to add suppress message.

Please update this documentation to cover such a case.

---

Document Details

⚠ Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.

• ID: 5030cf5c-ae08-5a08-0b47-995006379292
• Version Independent ID: 9e1df70a-127c-eafc-e027-360797faf3a6
• Content: CA1801: Review unused parameters - Visual Studio
• Content Source: docs/code-quality/ca1801-review-unused-parameters.md
• Product: visual-studio-dev15
• GitHub Login: @gewarren
• Microsoft Alias: gewarren

[gewarren]

I tried this out and I'm not seeing CA1801 fire. Instead I see IDE0060 (Remove unused parameter 'assert' if it is not part of a shipped public API):

@mateusz-dobrowolny Did you see something different? And, are you asking us to update the documentation to say it's okay to suppress the warning for the first parameter of extension methods? /cc @mavasani

[mateusz-dobrowolny]

@gewarren
IDE0060 is in VS 2019.
I am using VS 2017 - there is IDE up to 0049.
Seems like you are using VS 2019, with max IDE number 0061.

As for me (I have discussed it internally with architects), the best would be to make singleton 'This' to have all methods available.

So I could use:

public static void MyAssumptionIsCorrect(this Assert assert, object obj)
{
    assert.IsTrue(obj.field == ... );
}

No I am forced to use

Assert.IsTrue()

inside.

Back to your second question?
Suppressing the warnign for the first parameter of extension method seems like accepting bed design, because the first parameter seems to be the most important - it determines what object we are extending.
I think 'That' singleton from Assert class should be mentioned literally without any generalization, if there will be decision to update documentation.
However, I'd prefer modifying the code of Assert class instead.

[gewarren]

I think 'That' singleton from Assert class should be mentioned literally without any generalization, if there will be decision to update documentation.
However, I'd prefer modifying the code of Assert class instead.

@mateusz-dobrowolny In that case, the best place to log this issue is probably here on the testfx repo. If that doesn't work out, we can add a note to this doc about Assert.That.

[mateusz-dobrowolny]

So ... the testfx team closed my issue.
I understand that this means they want documentation update.

https://github.com/microsoft/testfx/issues/623#issuecomment-504376086

Can we reopen this issue or should I open a new one?

[gewarren]

@mateusz-dobrowolny I think the testfx team is suggesting that the code analysis rule should ignore the 'this' parameter. I asked a clarifying question in that issue. I think that adding a note to the documentation would also be considered a workaround to the actual issue.

[mateusz-dobrowolny]

@gewarren My understanding is that you may a little bit misinterprete the answer of the testfx team.
Extension methods are desinged in C# language to use syntax (thix Class parzmeterName, ... ) and they are "extending" the Class. So the goal is to use this word.
And the answer was (I am observer of that issue, so I got notifications as well):

No that is not the recommendation.

My understanding is that testfx team suggest to suppress the rule only in case of Assert class.

[mateusz-dobrowolny]

😻