search

markvincze / Stubbery

Library for creating Api stubs in .NET. https://markvincze.github.io/Stubbery/
MIT License
74 stars 14 forks source link

Allow returning IActionResult and auto Complex Type serialize to Json #25

Closed eByte23 closed 4 years ago

eByte23 commented 4 years ago

This PR put support for return result that Inherit from IActionResult e.g. FileContentResult etc.. The dependency on Mvc DI extension is for IActionExecutor which would be used for a lot of different ActionResult's

markvincze commented 4 years ago

Hi!

Thanks for your PR, sorry for the delayed review.
Actually I'v been thinking about how to enable using different result objects than just strings, and I might rework this part in the future, but I think this PR is a good first step. Can you please write two new unit tests, one for testing the IActionResult-case, one for testing when the response is object?

eByte23 commented 4 years ago

Thanks for the reply, I'll make the changes and add some tests a little later today.

eByte23 commented 4 years ago

@markvincze I have rebased the pr, resolved all comments and added the requested tests. If there are any other changes you'd like let me know. Cheers

markvincze commented 4 years ago

@eByte23,

I'm afraid there is a problem now, in the new version we're not setting the status code, because this line:

httpContext.Response.StatusCode = StatusCodeProvider(httpContext.Request, arguments);

was removed, so we're returning 404 instead of 200. You'll see if you run all the tests locally.
I guess if you add that to the same place it was, that'll be the correct behavior, right? Because the ActionResult-branch possibly runs afterwards, and it can still change the status code to what it wants.

And unrelated issue is that we have a problem with the CI build, it doesn't really run the tests. I'll try to fix that separately on master.

eByte23 commented 4 years ago

Sorry about that missed it in the rebase as it was the same line as the response.write before.

Should have revert that now, tests ran locally fine.

markvincze commented 4 years ago

Great, thanks for the contribution! I'll merge the PR now, and will release the new package a bit later, probably today.

markvincze commented 4 years ago

Sorry for the delay, I released 2.5.0 now, it includes the PR.

eByte23 commented 4 years ago

No problem at all much appreciated