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

IAsyncEnumerable ReturnsAsync support #1250

Closed

Conversation

cdonnellytx
Copy link

This adds ReturnsAsync overloads that can return IAsyncEnumerable<T> based on an input IEnumerable<T> value.

This was first requested in #962.

Additional information

  • Because IAsyncEnumerable is not supported in .NET 4.5, but is supported in .NET Framework 4.6.1 and later, Moq now additionally targets .NET 4.6.1 so that Framework users may also benefit from this feature.
  • System.Linq.Async has also been added as an additional dependency.

@cdonnellytx cdonnellytx changed the title IAsyncEnumerable support IAsyncEnumerable ReturnsAsync support Mar 30, 2022
@cdonnellytx cdonnellytx marked this pull request as ready for review March 30, 2022 20:00
@stakx
Copy link
Contributor

stakx commented Mar 30, 2022

Hi @cdonnellytx and thanks for contributing. Your PR is touching a few things that I was hoping to leave behind in the past, so I am not convinced this should get merged:

  • I don't think adding new netXY target frameworks is the right way to go (I'd rather we keep them to a minimum) as the .NET Framework is now legacy tech.

  • Even more so if it means adding conditional compilation symbols, too. Ideally we wouldn't have any of those. I once made an effort to get rid of as many of them as I could, as they can make code harder to reason about.

  • The .ReturnsAsync, .ThrowsAsync APIs have become mostly redundant, since you can now setup a [Value]Task's .Result directly and just use .Returns, .Throws. I was hoping we could let those older APIs die off, since (as you discovered) they add a ton of repetitive method overloads.

Your PR's main goal (glossing over the time delay overloads) appears to be saving users from having to call .ToAsyncEnumerable() manually. Is that assessment correct?

@cdonnellytx
Copy link
Author

Your PR's main goal (glossing over the time delay overloads) appears to be saving users from having to call .ToAsyncEnumerable() manually. Is that assessment correct?

Yes, that is correct. I am hoping to avoid having to do that, especially since [Value]Task have the equivalent ReturnsAsync methods. Though if those are going away, it doesn't makes sense to add them (back) for IAsyncEnumerable.

One difference though is that IAsyncEnumerable doesn't have a .Result or any other equivalent property; the nearest thing I could find is a ToEnumerable() extension method:

- service.Setup(m => m.GetRelatedIdsAsync(1)).ReturnsAsync(new[] { 1, 2, 3 });
+ service.Setup(m => m.GetRelatedIdsAsync(1).ToEnumerable()).Returns(new[] { 1, 2, 3 });


Unfortunately, that doesn't work out-of-box today; Moq would have to be updated to support that expression -- and I'm not familiar enough with expression trees to know if that would work.

Regarding target frameworks, I went with net461 because that is the minimum supported target for IAsyncEnumerable, which meant adding a target to support it in later .NET Framework versions. The alternative would be to raise Moq's minimum .NET Framework target to 4.6.1 (or possibly 4.6.2 since that will be the new minimum supported version after April 26).

Regarding time delay overloads, are you planning on supporting some kind of delay testing in the next version of Moq?

@stakx
Copy link
Contributor

stakx commented Jan 3, 2023

Finally found time to take a closer look at this. Sorry to say but I'm going to close this without merging.

  • The ReturnsAsync methods produce Tasks or ValueTasks – I don't think we need to have a ReturnsAsync method for IAsyncEnumerable<> (which is not a task, and thus doesn't really fit the existing pattern) just because there's "async" in the interface's name.
  • Doing a setup.Returns(enumerable.ToAsyncEnumerable()) isn't really that much more complicated than setup.ReturnsAsync(enumerable). I don't think we need to add a convenience helper method to the core library (that we'll then have to maintain) just so users can avoid calling that extension method themselves.

@stakx stakx closed this Jan 3, 2023
@devlooped devlooped locked and limited conversation to collaborators Sep 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants