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

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

Merged
merged 2 commits into from
Jan 14, 2024

Conversation

0xced
Copy link
Contributor

@0xced 0xced commented Oct 13, 2023

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
Copy link

github-actions bot commented Oct 13, 2023

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
Copy link

coveralls commented Oct 13, 2023

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

@@ -277,7 +277,7 @@ private static void ForceEnumeration<T>(T subject, Func<T, IEnumerable> enumerab
/// current <see cref="object"/>.
/// </summary>
[Pure]
public static ObjectAssertions Should(this object actualValue)
public static ObjectAssertions Should([System.Diagnostics.CodeAnalysis.NotNull] this object actualValue)
Copy link
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
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
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
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
Copy link
Contributor

IT-VBFK commented Oct 14, 2023

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
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
Copy link
Contributor

IT-VBFK commented Oct 14, 2023

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

@lg2de
Copy link
Contributor

lg2de commented Oct 14, 2023

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
Copy link
Contributor

IT-VBFK commented Oct 14, 2023

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

Ahh.. thanks for clarification :)

@0xced
Copy link
Contributor Author

0xced commented Oct 14, 2023

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.

Copy link

github-actions bot commented Jan 6, 2024

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
Copy link
Contributor Author

0xced commented Jan 6, 2024

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

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
Copy link
Member

@dennisdoomen dennisdoomen left a comment

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
Copy link
Contributor Author

0xced commented Jan 13, 2024

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

Copy link
Member

@dennisdoomen dennisdoomen left a comment

Choose a reason for hiding this comment

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

Awesome

Copy link
Member

@jnyrup jnyrup left a comment

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
7 checks passed
@thj-dk
Copy link

thj-dk commented Mar 14, 2024

When is this planned to be released?

@ITaluone
Copy link
Contributor

ITaluone commented Mar 14, 2024

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: https://github.com/orgs/fluentassertions/projects/1

@dennisdoomen
Copy link
Member

@ITaluone is correct

@dggsax
Copy link

dggsax commented May 29, 2024

Is it possible to release it in an alpha release at the very least? Or are there items in #1677 that you need assistance with that could help speed this along. I very much look forward to taking advantage of the changes in this PR, thanks!

@dennisdoomen
Copy link
Member

Is it possible to release it in an alpha release at the very leas

I just tagged alpha 4. Since it's a pre-release, all bets are off.

@0xced
Copy link
Contributor Author

0xced commented May 29, 2024

Oopsie. 😖

https://github.com/fluentassertions/fluentassertions/actions/runs/9291245214/job/25569332022#step:5:451

[ERR] Push: error: Response status code does not indicate success: 403 (The specified API key is invalid, has expired, or does not have permission to access the specified package.).

@IT-VBFK
Copy link
Contributor

IT-VBFK commented May 30, 2024

Why not just compiling the latest develop yourself?

  • Clone this repo
  • run build.ps1 or build.sh and you have your "alpha version" 🤷‍♂️

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