Implemented ShouldRenderFileContents and deprecated ShouldRenderFile

[AlexArchive]

I have implemented the ShouldRenderFileContents method and it's relevant overloads and I have marked the ShouldRenderFile method as obsolete.

I did not implement support for the overload that takes a string. I want to implement that overload but first I would like to validate that this code is correct.

Thanks.

[robdmoore]

Looks good. I think checking content type before checking the content itself makes sense though. Thoughts?

On 22 Aug 2014, at 2:57 am, ByteBlast [email protected] wrote:

I have implemented the ShouldRenderFileContents method and it's relevant overloads and I have marked the ShouldRenderFile method as obsolete.

I did not implement support for the overload that takes a string. I want to implement that overload but first I would like to validate that this code is correct.

Thanks.

You can merge this Pull Request by running

git pull https://github.com/ByteBlast/TestStack.FluentMVCTesting master
Or view, comment on, or merge it at:

#23

Commit Summary

Added support for checking for a file content result.
Added support for checking a file content result's binary contents.
Added support for checking a file content result's content type.
Deprecated ShouldRenderFile.
File Changes

M TestStack.FluentMVCTesting.Tests/ControllerResultTestTests.cs (46)
M TestStack.FluentMVCTesting.Tests/TestControllers/ControllerResultTestController.cs (6)
M TestStack.FluentMvcTesting/ControllerResultTest.cs (26)
Patch Links:

https://github.com/TestStack/TestStack.FluentMVCTesting/pull/23.patch
https://github.com/TestStack/TestStack.FluentMVCTesting/pull/23.diff
—
Reply to this email directly or view it on GitHub.

[AlexArchive]

I thought about this briefly earlier and decided that it feels more natural to throw exceptions according to the order of the arguments. I also ordered the conditional tests in this way because I applied similar logic previously to the ShouldRenderFilePath method - I wanted to be consistent.

I do not have any strong feelings one way or the other, though. Let me know what you want me to do.

Thanks for the comments :)

[robdmoore]

Consistency is important and throwing in the order of the args makes sense.

The only thing I'm wondering is it it makes it easier from a debugging point of view to see the wrong content type before knowing that something about the byte[] is wrong?

[AlexArchive]

What you say makes a lot of sense to me now. You are saying that an invalid content type has the potential to set red flag as it were - testing the content type first could prevent the developer from being lead down the garden path.

I will refactor now :)

[robdmoore]

Yep - makes sense

[AlexArchive]

Should I refactor the ShouldRenderFilePath too do you think?

[robdmoore]

Sure, these methods are new so the breaking change shouldn't matter right?

[AlexArchive]

In reality, probably not.

[robdmoore]

Cool. I'll unlist that one and deploy again when your changes are in :)

[robdmoore]

btw - if you could adjust the readme/docs to add the new methods that would be awesome :)

[AlexArchive]

I tweaked the order of the tests and added some unit-tests to document the behaviour.

I also snuck in another commit - improved formatting for the test message.

[AlexArchive]

if you could adjust the readme/docs...

Sure. I will do that when issue #3 is closed. Only the filestream test overloads to go. I am going to comment on the actual issue now.

[robdmoore]

Great work!

[robdmoore]

1.0.23 is deployed to nuget