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

Implement support for IAsyncEnumerable the same way how Nunit4 implemented it #2535

Draft
wants to merge 16 commits into
base: develop
Choose a base branch
from

Conversation

zachsaw
Copy link

@zachsaw zachsaw commented Jan 2, 2024

Draft PR to implement support for IAsyncEnumerable - option 3 as discussed in Issue 1213? The first commit does not take dependency on System.Linq.Async but does not offer strongly typed BeEquiavlentOf. Taking dependency on System.Linq.Async should be no worse than System.Threading.Tasks though.

This is also the implementation NUnit4 has gone with. Yes, it does violate sync-over-async practice, but for all intents and purposes, FluentAssertion is a unit test framework. Blocking enumeration of AsyncEnumerable isn't as big a sin here vs high throughput web servers.

Note: This is a draft PR to demo a concept, not prod ready in any shape or form.

IMPORTANT

  • If the PR touches the public API, the changes have been approved in a separate issue with the "api-approved" label.
  • The code complies with the Coding Guidelines for C#.
  • The changes are covered by unit tests which follow the Arrange-Act-Assert syntax and the naming conventions such as is used in these tests.
  • If the PR adds a feature or fixes a bug, please update the release notes with a functional description that explains what the change means to consumers of this library, which are published on the website.
  • If the PR changes the public API the changes needs to be included by running AcceptApiChanges.ps1 or AcceptApiChanges.sh.
  • If the PR affects the documentation, please include your changes in this pull request so the documentation will appear on the website.
    • Please also run ./build.sh --target spellcheck or .\build.ps1 --target spellcheck before pushing and check the good outcome

Copy link

github-actions bot commented Jan 2, 2024

Qodana for .NET

20 new problems were found

Inspection name Severity Problems
Possible multiple enumeration 🔶 Warning 13
Possible 'null' assignment to non-nullable entity 🔶 Warning 3
Auto-property accessor is never used (private accessibility) 🔶 Warning 2
Collection is never updated (private accessibility) 🔶 Warning 1
Possible 'System.NullReferenceException' 🔶 Warning 1

💡 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

namespace FluentAssertions.Collections;

[DebuggerNonUserCode]
public class AsyncEnumerableAssertions<T> : AsyncEnumerableAssertions<IAsyncEnumerable<T>, T, AsyncEnumerableAssertions<T>>
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be much simpler to just have the new Should overload enumerate the async collection and then return the existing GenericCollectionAssertions?

Choose a reason for hiding this comment

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

lol yeah you're right. That also allows us to avoid taking dependency on System.Linq.Async!

Copy link

Choose a reason for hiding this comment

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

ok that actually failed when we have a class that implements multiple IAsyncEnumerable generic types. It no longer sees the interface having multiple generic types due to the use of ToBlockingEnumerable().

i.e. this fails.

    // public class AsyncEnumerableOfStringAndObject : IAsyncEnumerable<object>, IAsyncEnumerable<string>

    [Fact]
    public void
        When_a_object_implements_multiple_IAsyncEnumerable_interfaces_but_the_declared_type_is_assignable_to_only_one_and_runtime_checking_is_configured_it_should_fail()
    {
        // Arrange
        IAsyncEnumerable<string> collection1 = new AsyncEnumerableOfStringAndObject();
        IAsyncEnumerable<string> collection2 = new AsyncEnumerableOfStringAndObject();

        // Act
        Action act =
            () => collection1.Should().BeEquivalentTo(collection2, opts => opts.RespectingRuntimeTypes());

        // Assert
        act.Should().Throw<XunitException>("the runtime type is assignable to two IAsyncEnumerable interfaces")
            .WithMessage("*cannot determine which one*");
    }

Copy link

Choose a reason for hiding this comment

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

AsyncEnumerableAssertions is gross as it violates DRY having lots of shared code with GenericCollectionAssertions. Would be open to any suggestions to avoid this.

Looking at the many usages of ReferenceTypeAssertions.Subject, it is probably wise to keep Subject the actual type which is IAsyncEnumerable?

Copy link

Choose a reason for hiding this comment

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

Ok I've updated AsyncEnumerableAssertions to reuse GenericCollectionAssertions where possible.

Copy link

Choose a reason for hiding this comment

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

I've had to revert the above. We lose ReferenceEquals checks when we do that as the references are no longer the original one (due to calling ToBlockingEnumerable()) so they are no longer "equal".

It appears code duplication here is necessary.

@dennisdoomen dennisdoomen changed the title [Issue 1213] Implement support for IAsyncEnumerable the same way how Nunit4 implemented it Implement support for IAsyncEnumerable the same way how Nunit4 implemented it Jan 6, 2024
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

3 participants