API thoughts

Nov 29, 2011 at 3:29 PM

I find the need to use asserts as such:

Assert.IsTrue(tester.WithIncomingRequest("/").MatchesRoute("Home", "Index"));

To be somewhat meaningless, and just code noise.

Since matches route returns true/false this API would make more sense if you just needed to.

tester.WithIncomingRequest("/").ShouldMatchRoute("Home", "Index")
tester.WithIncomingRequest("/handler.axd/pathInfo").ShouldBeIgnored();

A similar construct would be to follow the fluent interface pattern and have code like:

tester.WithIncomingRequest("/").MatchesRoute("Home", "Index").Assert() or Verify, or IsTrue() etc for a fluent interface terminator.
Coordinator
Nov 30, 2011 at 7:53 PM

That is the path I originally took, but then my thought was that by simply returning a boolean, it makes it easier to Assert using your own unit test framework.

Implementing built-in Assert functionality seemed a bit of a duplication of effort, since every testing framework already has that.

Nov 30, 2011 at 10:01 PM

I thought the same as @dotnetchris when I put this project to use. It made failing unit tests completely nondescript.

I don't know if the approach I came up with is ideal (I used Assert* instead of Should* as he did), but it should get the job done nicely while providing a fairly detailed level of data for failing matches. I created a custom exception, AssertionException, that all the matching code throws when an issue comes up. I would hope any testing framework should be able to handle those sorts of exceptions as a test failure just fine. I modified the original calls (e.g., MatchesRoute) to call this new code and simply look for an AssertionException to return false. This will keep it from breaking any existing code that uses the project. I will push to a fork later tonight and submit a pull request shortly thereafter.

On a related note, I added unit tests for the match code for all the potential mismatches paths in the code. I used NUnit for the test project, but that won't force anything on a consumer of this project. It did, however highlight a potential bug that I will bring up in another discussion (noted in my fork's code and corresponding ignored unit test, but that can be removed). Long story short: I believe there is an inconsistency in how the code considers a null `routeValues` object on a found matched route to be an error while not having a problem with a found route containing more values than the route object being passed in. It's worth a look, I think.

Coordinator
Nov 30, 2011 at 10:21 PM

Partridge,

Not sure what version you forked, but if I understand your last paragraph correctly, I addressed that in the latest version. Again, I may not be understanding you correctly.

Originally, this would return true:

tester.WithIncomingRequest("/Foo/Bar/5").MatchesRoute("Foo", "Bar");

That is no longer the case. You are correct -- MatchesRoute should not return true on a "partial" match.

Starting with version 0.8.5, you would have to do the following to return true:

tester.WithIncomingRequest("/Foo/Bar/5").MatchesRoute("Foo", "Bar", new {id = 5});

Again, that is what I _think_ you are getting at, correct?

Coordinator
Nov 30, 2011 at 10:35 PM

Partridge -- Also, with regard to your other points:

I also have unit tests for this, but I didn't check in the unit test project as part of the repo. I probably should have. I used MSTest, but would have no problem swapping the unit test project over to NUnit and merging whatever tests you come up with. 

Regarding throwing a custom exception -- my only concern is that all the various test frameworks I mention in the docs should be able to interpret the exception as a test failure. I don't have a lot of experience with xUnit or MbUnit... I assume they would be okay but you should check to make sure.

There is also an obscure edge case issue that occurs when a null default value is supplied in a route definition. I have addressed it but have not pushed the fix as of yet.

Dec 1, 2011 at 3:18 AM

I put together two sample tests, one pass and one fail, and put those in each testing framework: xUnit, NUnit, MSTest, and MbUnit. I wasn't expecting otherwise, since an exception being thrown should be a pretty good indication of a failure, but all tests passed and failed just fine, respectively.

Here is an example output from a failed test (obviously messages can be tweaked, but I put in what I thought would be useful):

MvcRouteUnitTester.AssertionException: Controller mismatch: expected "IntentionallyNotRight" but was "Home" (for url: "/Home/About").
at MvcRouteUnitTester.RequestInfo.AssertMatchesRoute(String controller, String action, Object routeValues) in C:\somewhere\mvcrouteunittester\MvcRouteUnitTester\RequestInfo.cs:line 79 ...

Coordinator
Dec 1, 2011 at 3:30 AM

Very cool. Obviously much more informative as well. ;) 

I have been using this on my own for some time and it is usually quickly apparent why a test is failing, but this does make it crystal clear. Looking forward to seeing your code on this.

I'd like to run something by you tomorrow via email if you don't mind. Would be interested in your opinion on something regarding an idea I had for the library...

Dec 1, 2011 at 3:53 AM

Well, I would push my changes to my fork, but CodePlex is having some weird permissions issue (apparently seen elsewhere, too: http://codeplex.codeplex.com/discussions/238046).

Coordinator
Dec 1, 2011 at 1:58 PM

In the meantime, could you upload to your Dropbox and send me a link to it?

Dec 1, 2011 at 4:00 PM

Here's the patch I am trying to push to my fork for a pull request. The two ignored tests are the ones where I have found other things to resolve in later work.

http://dl.dropbox.com/u/6593594/MvcRouteUnitTester_ExceptionBasedTesting.patch

Dec 1, 2011 at 4:03 PM

BTW, I tried to keep the styling the same, but I may have had my tab lengths off. Were you using 3 spaces?

Regardless, I can change it for the final pull request.

I ended up writing a blog post about it: http://www.patridgedev.com/2011/12/01/adapting-visual-studio-code-styling-differences-for-open-source-project-contribution/).

Coordinator
Dec 1, 2011 at 4:35 PM

Thanks, I'll have a looky later on.

Yep, three spaces. Plus I always scope usings inside the namespace, which throws people off sometimes too. ;)

Dec 1, 2011 at 5:08 PM

I have no problem with the using statements. I picked that up from some utility a few months ago; it only bites me when I have to stumble back to VB land where that isn't acceptable. The three spaces definitely threw me, despite how trivial it is. It is super simple to adjust, but I have only ever dealt with 2 or 4.

CodePlex responded to my issue and "fixed the permissions" just like the others on here: http://codeplex.codeplex.com/discussions/238046. I have pushed to my fork, but I will make sure my tabs are fixed before I toss out a pull request.

If you see anything else in my changeset that needs tweaking before then, just let me know.

Dec 1, 2011 at 7:23 PM

I write unit tests in Machine.Specifications so I find it quite normal that a testing framework has it's own custom .Should extensions for validating its specific assertions. This is also the pattern Machine.Specifications follows, extension methods for Should______ that throw new SpecificationExceptions if anything fails them.

Coordinator
Dec 1, 2011 at 7:37 PM

Actually, I think I like the "Should" terminology a bit better than "AssertMatchesRoute" / "AssertGeneratesUrl". Reads more like plain English. Really a matter of taste though.

Coordinator
Dec 1, 2011 at 7:40 PM

Patrige: Do you think we should make AssertionException serializable?

Dec 2, 2011 at 4:13 PM

I wrote the code before reading those first two posts in this discussion, but I definitely wanted to go that route after I did. I switched to Should* in my fork and updated the pull request.

Since I haven't played with specification-driven stuff and I don't have that context registered in my brain, the term SpecificationException isn't as descriptive to me as AssertionException. Regardless, the change would seem arbitrary to me. I can certainly do so, but I don't see a benefit to it.

As for serializable on the exception, what does that buy us? I have only ever needed classes to be serializable for database-stored ASP.NET session objects, so I don't know all the other useful times for having a class be serializable. Again, if it helps, I will gladly slap that on there (assuming that's all it takes).

Coordinator
Dec 2, 2011 at 6:23 PM

I'd say AssertionException is a perfectly good thing to call it.

I just mention the serialization because it is customary to make custom exceptions serializable. Although now that I look closer I see that there aren't any custom properties, so it wouldn't be necessary.

Dec 2, 2011 at 8:44 PM

I haven't dealt with a lot of custom exceptions yet, so I hadn't seen that come up. I did find a reference about why one would want to do so. While I don't know how often an exception in a unit test utility would be marshalled between app domains, it took five seconds to make it meet the FxCop requirements.

Pull request updated.

Coordinator
Dec 3, 2011 at 8:38 PM

Been going through your pull request. Very, very nice contribution. Really great work.

Probably will just tweak the XML documentation a little, and add more tests.

Coordinator
Dec 5, 2011 at 3:41 AM

Pull request accepted and merged.