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 notifications@github.com 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.

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 :)

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?

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 :)

Yep - makes sense

Should I refactor the ShouldRenderFilePath too do you think?

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

In reality, probably not.

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

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

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.

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.

Great work!

1.0.23 is deployed to nuget