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

Setup sequence doesn't support ReturnsAsync with Func<> #795

Closed
kswider opened this issue Apr 3, 2019 · 12 comments · Fixed by #841
Closed

Setup sequence doesn't support ReturnsAsync with Func<> #795

kswider opened this issue Apr 3, 2019 · 12 comments · Fixed by #841
Assignees

Comments

@kswider
Copy link

kswider commented Apr 3, 2019

In SetupSequence there is an overload for Returns can that take Func<> as an argument, but there isn't such overload for ReturnsAsync.

I think that adding this overload to ReturnsAsync would be a nice enhancement.

@stakx
Copy link
Contributor

stakx commented Apr 4, 2019

Would you like to provide the implementation for it?

@kswider
Copy link
Author

kswider commented Apr 7, 2019

Yea, I think I can make it :)

@VladFasie
Copy link

could you please detail the requirements ?

@stakx
Copy link
Contributor

stakx commented Apr 8, 2019

@VladFasie:

The existing non-async method sequenceSetup.Returns(Func<T> valueFactory) returns a value of type T that is equal to whatever valueFactory() returns. Evaluating valueFactory happens once per invocation of the sequence setup.

I'd expect the planned sequenceSetup.ReturnsAsync(Func<T> valueFactory) to do the same, i.e. it should return a completed [Value]Task<T> whose .Result property equals the value produced by valueFactory(). Neither the result of that call nor the returned task should be cached, i.e. every invocation of the sequence setup produces a new task.

@VladFasie
Copy link

I can't make my branch public.

git push --set-upstream origin "#795"
remote: Permission to moq/moq4.git denied to VladFasie.
fatal: unable to access 'https://github.com/moq/moq4.git/': The requested URL returned error: 403

@stakx
Copy link
Contributor

stakx commented Apr 8, 2019

@VladFasie - First, please coordinate your efforts with @kswider, as he stated above that he might be working on this as well. It would be a shame if both of you work on this separately, as I will only be able to merge the changes from one of you.

If @kswider is fine with you going ahead here, then please send a pull request (PR) via GitHub. That is:

  1. Fork this repository to your own GitHub account.
  2. Clone your own fork repository.
  3. Create a topic branch off master.
  4. Add commits to that topic branch.
  5. Push your branch back to your repo.
  6. Submit a PR from this branch in your repository to master in this repository.

Since you've already started, you can skip 2, 3, and 4. Instead, add a remote in your local repo that points to your fork repo on GitHub.

Please make sure that you've read our contributing guide. Thanks!

P.S. not sure if GitHub handles a branch name #795 well, it might be prudent to choose a safer name like issue-795 or even something more telling, like sequencesetup-returnsasync. But that's just a suggestion, not a requirement.

@stakx stakx removed the up-for-grabs label Apr 8, 2019
@kswider
Copy link
Author

kswider commented Apr 8, 2019

If @VladFasie has already finished fixing that issue I'm ok with that. I planned to do it today :)

@cassdeckard
Copy link

I ran into this just yesterday and would love to see it added. In addition, there doesn't seem to be any way of accessing the arguments within the version of Returns() that takes a Func<>. Was that intentional or another oversight that could potentially be added along with this?

@stakx
Copy link
Contributor

stakx commented Apr 11, 2019

Was that intentional or another oversight that could potentially be added along with this?

Moq's API today is mostly driven by the community's needs, so it is not so much an oversight, but more that noone asked for nor implemented this.

Making these additions would certainly be possible.

(I personally think the whole SetupSequence API shouldn't be expanded much further, initially it was probably meant as a simple helper that spared you the work of setting up a callback with a state machine inside (though that remains of course possible for more complicated scenarios), it's something of a burden keeping it in synch with the main Setup API.)

@VladFasie
Copy link

I hope I will manage to come with a push in weekend

@stakx
Copy link
Contributor

stakx commented May 8, 2019

@kswider: @VladFasie closed his PR a moment ago. If you'd still like to proceed with this issue and submit your own, please feel free to go ahead.

@BenCoden
Copy link

Is there a workaround?
And @kswider are you making a PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants