Visitar URL original
Add [NotNull] attribute on the Should() method for object assertions by 0xced · Pull Request #2380 · fluentassertions/fluentassertions · GitHub
Skip to content

Add [NotNull] attribute on the Should() method for object assertions - #2380

Merged
jnyrup merged 2 commits into
fluentassertions:developfrom
0xced:Should-NotNull
Jan 14, 2024
Merged

jnyrup merged 2 commits into
fluentassertions:developfrom
0xced:Should-NotNull

Conversation

@0xced

@0xced 0xced commented Oct 13, 2023

Copy link
Copy Markdown
Contributor

Compelling example:

[Theory]
[InlineData(true)]
[InlineData(false)]
public void Test1(bool currentUser)
{
    IPrincipal principal = currentUser ? new WindowsPrincipal(WindowsIdentity.GetCurrent()) : new ClaimsPrincipal();
    IIdentity? identity = principal.Identity;
    
    identity.Should().NotBeOfType<GenericIdentity>();
    identity.IsAuthenticated.Should().BeFalse();
}

Fixes #1115

@github-actions

github-actions Bot commented Oct 13, 2023 •

Copy link
Copy Markdown

Qodana for .NET

It seems all right 👌

No new problems were found according to the checks applied

💡 Qodana analysis was run in the pull request mode: only the changed files were checked

View the detailed Qodana report

To be able to view the detailed Qodana report, you can either:

  1. Register at Qodana Cloud and configure the action
  2. Use GitHub Code Scanning with Qodana
  3. Host Qodana report at GitHub Pages
  4. Inspect and use qodana.sarif.json (see the Qodana SARIF format for details)

To get *.log files or any other Qodana artifacts, run the action with upload-result option set to true,
so that the action will upload the files as the job artifacts:

      - name: 'Qodana Scan'
        uses: JetBrains/qodana-action@v2023.2.8
        with:
          upload-result: true
Contact Qodana team

Contact us at qodana-support@jetbrains.com

@coveralls

coveralls commented Oct 13, 2023 •

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 7515208120

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 97.48%

Totals Coverage Status
Change from base Build 7484912142: 0.0%
Covered Lines: 11762
Relevant Lines: 11944

💛 - Coveralls

/// </summary>
[Pure]
public static ObjectAssertions Should(this object actualValue)
public static ObjectAssertions Should([System.Diagnostics.CodeAnalysis.NotNull] this object actualValue)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not add the attribute to all Should() methods?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a very good question! I think we should add the [NotNull] attribute on all Should methods that return a ReferenceTypeAssertions<,>. So I started to write a test and noticed that I could not assert parameterInfo.IsDecoratedWith<NotNullAttribute>, hence #2385. 😄

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I started to write a test and noticed that I could not assert parameterInfo.IsDecoratedWith, hence #2385. 😄

I don't think we need to have tests for that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, at least it was very interesting to dive into all the assertion types, I learnt a lot. And the test is now written anyway, alongside other tests on the Should methods. But this pull request could also be merged without the tests added in AssertionExtensionsSpecs and thus without requiring #2385 to also be merged.

@IT-VBFK

IT-VBFK commented Oct 14, 2023

Copy link
Copy Markdown
Contributor

Isn't this something that pretends what isn't there?

In such a case we can get rid of all null checks inside in the assertions?

@dennisdoomen

Copy link
Copy Markdown
Member

Isn't this something that pretends what isn't there?

In such a case we can get rid of all null checks inside in the assertions?

I'm not entirely sure what you are saying? If you mean that we're kind of pretending that the subject is never null, that's true. We're relying on the assertion method itself to take care of that.

@IT-VBFK

IT-VBFK commented Oct 14, 2023

Copy link
Copy Markdown
Contributor

Ok, then this is just to silence the IDE, right?

@lg2de

lg2de commented Oct 14, 2023

Copy link
Copy Markdown
Contributor

Isn't this something that pretends what isn't there?

In such a case we can get rid of all null checks inside in the assertions?

Newer compiler version use such attributes to create warnings. Older version does not.
And: It is a warning which the user can ignore or even disable.
So, the Should method cannot ensure non-nullability with the attribute.

@IT-VBFK

IT-VBFK commented Oct 14, 2023

Copy link
Copy Markdown
Contributor

So, the Should method cannot ensure non-nullability with the attribute.

Ahh.. thanks for clarification :)

@0xced

0xced commented Oct 14, 2023

Copy link
Copy Markdown
Contributor Author

I have rebased this pull request on top of #2385 because the new specs (Should_methods_returning_reference_type_assertions_are_annotated_with_not_null_attribute and Should_methods_not_returning_reference_type_assertions_are_not_annotated_with_not_null_attribute) require the new assertions on ParameterInfo.

@github-actions

github-actions Bot commented Jan 6, 2024 •

Copy link
Copy Markdown

Qodana for .NET

It seems all right 👌

No new problems were found according to the checks applied

💡 Qodana analysis was run in the pull request mode: only the changed files were checked
☁️ View the detailed Qodana report

Contact Qodana team

Contact us at qodana-support@jetbrains.com

@0xced

0xced commented Jan 6, 2024

Copy link
Copy Markdown
Contributor Author

I just rebased this pull request on the develop branch and make it not depend on #2385 at all.

The assertion is now written like this:

parameter.GetCustomAttribute<NotNullAttribute>().Should().NotBeNull();

instead of this (which would require #2385 to be merged first):

parameter.Should().BeDecoratedWith<NotNullAttribute>();

@zhangpengchen

Copy link
Copy Markdown

Hi @dennisdoomen ,

Would you consider releasing this feature soon so we can finally avoid workarounds by mixing the native assertions and fluent assertions in the tests.

Compelling example:

```csharp
[Theory]
[InlineData(true)]
[InlineData(false)]
public void Test1(bool currentUser)
{
    IPrincipal principal = currentUser ? new WindowsPrincipal(WindowsIdentity.GetCurrent()) : new ClaimsPrincipal();
    IIdentity? identity = principal.Identity;

    identity.Should().NotBeOfType<GenericIdentity>();
    identity.IsAuthenticated.Should().BeFalse();
}
```

Fixes fluentassertions#1115
@dennisdoomen
dennisdoomen requested a review from jnyrup January 13, 2024 16:32

@dennisdoomen dennisdoomen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also update the releases.md

@0xced

0xced commented Jan 13, 2024

Copy link
Copy Markdown
Contributor Author

I just added an entry in the release notes under improvements in 6ac05e8.

@dennisdoomen dennisdoomen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome

@jnyrup jnyrup left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed that [NotNull] is not added to these overloads of Should, but guess it doesn't really matter that much.

AssertionExtensions.cs:168:    public static ExecutionTimeAssertions Should(this ExecutionTime executionTime)
AssertionExtensions.cs:753:    public static TypeSelectorAssertions Should(this TypeSelector typeSelector)
AssertionExtensions.cs:788:    public static MethodInfoSelectorAssertions Should(this MethodInfoSelector methodSelector)
AssertionExtensions.cs:813:    public static PropertyInfoSelectorAssertions Should(this PropertyInfoSelector propertyInfoSelector)
AssertionExtensions.cs:865:    public static TaskCompletionSourceAssertions<T> Should<T>(this TaskCompletionSource<T> tcs)
AssertionExtensions.cs:894:    public static TaskCompletionSourceAssertions Should(this TaskCompletionSource tcs)

@jnyrup
jnyrup merged commit ca87a81 into fluentassertions:develop Jan 14, 2024
@0xced
0xced deleted the Should-NotNull branch January 14, 2024 19:38
@holm-dk

holm-dk commented Mar 14, 2024

Copy link
Copy Markdown

When is this planned to be released?

@ITaluone

ITaluone commented Mar 14, 2024 •

Copy link
Copy Markdown
Contributor

I think there is no specified date for now :)

I assume, that at least this ticket have to be completely addressed: #1677
and/or every further issue in v7 milestone/project: v8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support c# 8 nullable reference

10 participants