Ayende @ Rahien

It's a girl

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

Neil Mosafi
08/13/2009 11:26 PM by
Neil Mosafi

Oh that's nasty. At least put a message if you're gonna do that!

RobK
08/13/2009 11:35 PM by
RobK

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.

Neil Mosafi
08/13/2009 11:39 PM by
Neil Mosafi

@RobK hmm, surely seeing Expected 'True', Was 'False' is going to trigger the developer's "surely there's a better way" instinct?

Damien Guard
08/13/2009 11:41 PM by
Damien Guard

Just this week I found myself using Assert.True when I really wanted Assert.Contains...

[)amien

Andrew Davey
08/13/2009 11:43 PM by
Andrew Davey

What you really want is a macro (if using boo or nemerle) that takes the boolean expression and produces a nice descriptive message.

RobK
08/13/2009 11:58 PM by
RobK

@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.

John Rayner
08/14/2009 12:28 AM by
John Rayner

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.

Diego Mijelshon
08/14/2009 12:38 AM by
Diego Mijelshon

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.

gegen
08/14/2009 12:47 AM by
gegen

like

Dmitry
08/14/2009 12:59 AM by
Dmitry

Sometimes you might want to test the == operator implementation instead of the Equals() method.

Peter Morris
08/14/2009 09:08 AM by
Peter Morris

Yeah, not nice :-)

I used to hate seeing this

if (completed == true)

but at least that was just personal taste :-)

Fabian Schmied
08/14/2009 09:27 AM by
Fabian Schmied

Assert.That (pet.Owner, Is.EqualTo ("Baz"))

:)

Stephen Harrison
08/14/2009 11:18 AM by
Stephen Harrison

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.

Erik van Brakel
08/14/2009 01:04 PM by
Erik van Brakel

@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.

Mr_Simple
08/14/2009 01:14 PM by
Mr_Simple

I agree, however a simple change will tell you exactly what the issue is.

Assert.True(pet.Owner == "Baz", "pet.Owner == Baz");

Thomas Eyde
08/14/2009 01:19 PM by
Thomas Eyde

The last resort arrives faster in MSTest :-(

Jeremy Likness
08/14/2009 01:19 PM by
Jeremy Likness

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?

Tim Wilde
08/14/2009 01:44 PM by
Tim Wilde

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>

Tim Wilde
08/14/2009 01:50 PM by
Tim Wilde

sigh Expect is generic... Oren's blog at my generic type!

Justin Chase
08/14/2009 03:02 PM by
Justin Chase

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.

Tim Wilde
08/14/2009 05:35 PM by
Tim Wilde

@ 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.

Ayende Rahien
08/14/2009 09:33 PM by
Ayende Rahien

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

Fabio Maulo
08/15/2009 12:04 AM by
Fabio Maulo

That is the reason because I have re-wrote all messages of MsTests

in Sharp Tests Ex

Robz
09/04/2009 05:18 PM by
Robz

@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.

Comments have been closed on this topic.