Ayende @ Rahien

Refunds available at head office

Got to debug is a bug, fix your error messages

This is part of the RavenDB test suite.

image

It has a bug.

Can you see it?

One of the things that I learned when working on the Castle project is that errors are important. In fact, they are really important, so it is worth the time to check them very carefully.

In this case, this code would fail but won’t tell me why it is failing.

Changing it to this:

image

Means that I get the actual error message right there and then, no need to do anything special.

Tags:

Posted By: Ayende Rahien

Published at

Originally posted at

Comments

Scooletz
04/04/2012 10:25 AM by
Scooletz

Any with predicate will return true if the backupStatus.Messages is empty, so the Assert.False will throw. One of the solutions is what you provided, the second doing .Any() && .Any(yourPredicate)

Stefan
04/04/2012 10:29 AM by
Stefan

What about all other messages with severity error other than the first? They still get dropped.

Why not use Assert.Fail(message.Message); ? I find Assert.False(true,...) confusing.

Pop Catalin
04/04/2012 12:05 PM by
Pop Catalin

This is a bit of funny code: if (message != null) Assert.False(true, message.Message);

expecially the 'Assert.False(true...' part.

Can easily be replaced with:

Assert.Fail(message.Message) or Assert.IsNotNull(message, message.Message)

Pop Catalin
04/04/2012 12:07 PM by
Pop Catalin

Correction for above: Assert.IsNull should be used

Ryan
04/04/2012 12:41 PM by
Ryan

Assert.False(true,..) is a really confusing way to write an assertion.

Josh Kodroff
04/04/2012 02:12 PM by
Josh Kodroff

Umm... what's wrong with:

Assert.That(() => backup.Messages.Any(x => x.Severity == BackupStatus.BackupMessageSeverity.Error), Is.True, "Expected to find a backup message error, but didn't find one.");

Pretty explicit in my mind.

João P. Bragança
04/04/2012 04:10 PM by
João P. Bragança

He needs to show the actual error message in the tests.

Karep
04/04/2012 04:43 PM by
Karep

I hate Assert.False(true.... I was wondering if that was some typo.

Karep
04/04/2012 09:04 PM by
Karep

And also why you once break out of while (and method) and in other place you return?

Matthew Wills
04/05/2012 08:25 AM by
Matthew Wills

I'd likely lean towards:

var message = backupStatus.Messages.Where(x => x.Severity == BackupStatus.BackupMessageSeverity.Error).Select(z => z.Message).FirstOrDefault();

Assert.IsNull(message);

to avoid the if.

Matthew Wills
04/05/2012 08:30 AM by
Matthew Wills

Whoops, IsNull(message) should be IsNull(message, message).

Gian Maria
04/05/2012 05:26 PM by
Gian Maria

When a unit test fails the name of the unit test is important, but if you have some more descriptive failure message it will help you finding the real cause of the test failure.

I agree with matthew that an Assert.IsNull is a clearer form of assertion, but the key point is including the real error message in the unit test result to have a quick hint of why the test is failing.

Lookman
04/05/2012 09:02 PM by
Lookman

It will throw an exception if x.Severity is null.

Matthew Wills
04/06/2012 10:26 PM by
Matthew Wills

Gian,

I agree with matthew that an Assert.IsNull is a clearer form of assertion, but the key point is including the real error message in the unit test result to have a quick hint of why the test is failing.

Assert.IsNull(message, message) would do this (the second parameter being the failed assertion message).

It will throw an exception if x.Severity is null.

I don't think any of the proposed solutions will do that. They may have issues if backupStatus.Messages was null, or contained a null entry, though.

Comments have been closed on this topic.