Assert.True is the tool of last resort
I was just reviewing someone’s code, and as I was browsing through the tests, I run into the following anti pattern, just about all the asserts were specified as:
Assert.True(pet.Owner == "Baz");
The problem with that is that this is a perfect case of “let’s create a test that will tell us nothing when it is failing”.
It seems like a minor thing, to use the more explicit version:
Assert.Equal("Baz", pet.Owner);
But it isn’t, when this assert fails, we are going to get a lovely error message telling us exactly what went wrong. From maintainability standpoint, being able to see why a test failed without debugging it is not a plus, it is essential.
Assert.True is a tool of last resort, don’t use it if you have any choice at all.
Comments
Oh that's nasty. At least put a message if you're gonna do that!
Gotta chalk that one up to naivete (probably), and probably RTM. I remember the first time I saw the output from a failing test that was written with the appropriate Assert methods. Game changing.
@RobK hmm, surely seeing Expected 'True', Was 'False' is going to trigger the developer's "surely there's a better way" instinct?
Just this week I found myself using Assert.True when I really wanted Assert.Contains...
[)amien
What you really want is a macro (if using boo or nemerle) that takes the boolean expression and produces a nice descriptive message.
@Neil - Oh, it did. Was very very junior at the time and was learning TDD through examples that THE senior dev had worked up. Learned many lessons with that one.
I agree the code you present is nasty, but there's an interesting technique at themechanicalbride.blogspot.com/.../...rt-for.html. Your code looks like:
TestResult.Assert( () => "Baz" == pet.Owner );
The difference is that this translates (at runtime) into:
Assert.AreEqual( "Baz", pet.Owner, "pet.Owner = 'Baz'" );
IOW it presents the Lambda expression as the assertion failure message.
I saw that in a previous job, thankfully I convinced them to fix it.
There is, however, a legitimate use of Assert.True:
Assert.True(theObject.IsValid)
seems cleaner than:
Assert.AreEqual(true, theObject.IsValid)
Of course it's a matter of taste.
like
Sometimes you might want to test the == operator implementation instead of the Equals() method.
What do you think about this approach: themechanicalbride.blogspot.com/.../...rt-for.html ?
Yeah, not nice :-)
I used to hate seeing this
if (completed == true)
but at least that was just personal taste :-)
Assert.That (pet.Owner, Is.EqualTo ("Baz"))
:)
During my day job at a large software house I recently came across some fresh code that contained forty Assert.True(foo.Equals(bar)) assertions (as well as a few Assert.IsNotNull) in a single test. The test fixture contained even more!
Yes 40!
Yes all of them in one test!
Yes none of the 40 Assert.True’s had any messages in them (neither did any of the Assert.IsNotNull tests that were also in the same test).
Yes the code was reviewed by one of the senior developers (actually both developers have a high job title than me).
I wanted to cry, or stick pins in my eyes or something.
I think this was a case of the developer being new to NUnit/unit testing so I try to suggest better ways of doing the tests when I can.
@Stephen
A higher job title doesn't mean they're better than you with every technique ;-) Maybe those seniors come from a time unit testing wasn't so hot as it is now. Anyway, the correct reaction would be to ask the seniors what they think about it, if they're professionals they'll either say they missed it (doubtful :P) or learn from it and pick it up next time.
I agree, however a simple change will tell you exactly what the issue is.
Assert.True(pet.Owner == "Baz", "pet.Owner == Baz");
The last resort arrives faster in MSTest :-(
I've always thought this "felt" like a hack, but what would a cleaner method be?
bool handled;
try {
something;
}
catch (ExpectedException) {
handled = true;
}
Assert.IsTrue(handled, "The exception was not properly handled.");
What is the "appropriate" way to test the exception handling stack?
I quite like the NUnit fluent stuff with a bit of extension method sugar ( http://code.google.com/p/nunit-extmethods/) thrown in for readability.
From Assert.Equal( "Baz", pet.Owner );
To: Assert.That( pet.Owner.Equals( "Baz" ) );
To: pet.Owner.Should( Be.EqualTo( "Baz" ) );
@Jeremy Likness, build yourself a helper method for that stuff :) I use:
Expect <expectedexception.ToBeThrownBy( () => something );
http://codepaste.net/ngxb72>
sigh Expect is generic... Oren's blog at my generic type!
The error message from Assert.AreEqual is pointless. Oh great now you know it the failing test was because some value wasn't equal to another? So what.
You're going to have to go to that test, and debug it to find out WHY the assert failed anyway. The only valuable part of a failing assert is the stack trace and the red light. If you have the file and line number of the test that's all you need.
Having a million different Assert.ThisAndThat's is not wrist friendly and is extra maintenance just to get some extra information that is worthless.
@ Justin Case, I'll agree with you that if all you use the unit tests for is traffic lights indicating whether the code works or not, then yes the extra asserts are overkill.
However, if you want to use the unit tests as developer documentation, it's nice to have something that is as readable and descriptive as possible which take as little parsing as possible and the extra assert variants help there.
Justin,
If you know what the values are, you usually have a LOT more information about the error.
In addition to that, many testing frameworks do things like string diffs which are really helpful
That is the reason because I have re-wrote all messages of MsTests
in Sharp Tests Ex
@Jusin: It's the difference between
TEST FAILED
and
[bob] is not equal to [bob.]
I never have to run the debugger. I just go back to my code or test and make the fix.
Illustrated from your comments I would say you don't do much automated testing where people like me live in them.
Comment preview