Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

Assume we have a method that validates an object before saving it to a DB. It returns back KVP something like:

public KeyValuePair<bool, List<FooErrorReason>> ValidateABar(Bar b)
{
    var reasons = new List<FooErrorReason>();
    if (string.IsNullOrEmpty(bar.Name)) 
    {
        reasons.Add(new FooErrorReason { PropName = "Name", Message = "The name was missing" };
    }
    if (string.IsNullOrEmpty(bar.OtherName)) 
    {
        reasons.Add(new FooErrorReason { PropName = "OtherName", Message = "The other name was missing" };
    }
    return new KeyValuePair<bool, List<FooErrorReason>(reasons.Count == 0, reasons);
}

So now I want to test that, for a missing bar.Name, the error is returned. My question is on the make up of the test, concerning what to Assert.

public void ValidateABar_Fails_For_Missing_Name()
{
    //arrange
    var b = new Bar { OtherName = "Other" };

    //act
    var result = ValidateABar(b);

    //assert
    Assert.IsFalse(result.Key);
    Assert.AreEqual(1, result.Value.Count);
    Assert.IsTrue(result.Value.Any(x => x.PropName.Equals("Name")));
}

I have 3 Assert calls and it has a whiff of code smell about it, but I can't quite convince myself either way.

I'm asking myself 'What do you want to test?' and I do kind of feel that I should test that this failed, but failed specifically for the property of the test.

Is it enough to just use

Assert.IsTrue(result.Value.Any(x => x.PropName.Equals("Name")));

Or should I also check that there are no other errors I have picked up along the way?

Say for example someone added another validation rule without my knowing about it, I would think I would want my tests to tell me about this the next time I ran them. As an example, let's say someone changed the method as below:

public KeyValuePair<bool, List<FooErrorReason>> ValidateABar(Bar b)
{
    var reasons = new List<FooErrorReason>();
    if (string.IsNullOrEmpty(bar.Name)) 
    {
        reasons.Add(new FooErrorReason { PropName = "Name", Message = "The name was missing" };
    }
    if (string.IsNullOrEmpty(bar.OtherName)) 
    {
        reasons.Add(new FooErrorReason { PropName = "OtherName", Message = "The other name was missing" };
    }

    //added this!
    if (string.IsNullOrEmpty(bar.ThirdName)) 
    {
        reasons.Add(new FooErrorReason { PropName = "ThirdName", Message = "The third name was missing" };
    }
    return new KeyValuePair<bool, List<FooErrorReason>(reasons.Count == 0, reasons);
}

The test would now fail and I would see that this rule had been added, and amend my test accordingly (and presumably write another test to cover the new rule).

Am I taking the 'one Assert per test rule' too literally?

share|improve this question
1  
Be careful of wrapping a bool and a list of messages like that. it's not a key-value pair, so don't call it one. If you MUST take short-cuts, at least use a Tuple<T1, T2>. Much better though, to create a FooValidationReponse object with Success and Messages properties, to make it very clear what's going on. –  pdr Jan 3 '13 at 13:26
    
I agree and we are using a Response object. I thought KVP was clearer for a quick question, but can see how it is not now it is written out. Will bear in mind for the future! –  glosrob Jan 3 '13 at 13:28
add comment

2 Answers

up vote 6 down vote accepted

You are taking one assertion per test too literally, but not too seriously. It's a good rule to follow.

However, there are cases, like this one, where you cannot fully assert your one assertion with literally one Assert method. What you are trying to assert is that the response is showing an error message. Assert.AreEqual doesn't really allow you to do that, unless you write an equality comparer for your response object.

Another option is to use the Assert.That syntax and create your own IResolveConstraint object as a comparer. With a little effort, you could have something as simple as

Assert.That(response, Fails.With.Message("Blah"));

But really, is it worth the effort? What you have is just fine and it is asserting your response is correct. That is one assertion.

In other cases, you won't actually have any calls to Assert methods, because the thing you're asserting is that a mocked method was called correctly. That's still an assertion, even if it isn't an Assert, per se.

For example, say you have a Save method, which sends data to a repository (not caring if it's SQL, file system, mock object, etc.) and then returns nothing.

public void Save()
{
     // How do I test this method with an Assert call???
     FooRepository.Save(this.Name, this.OtherName.Trim());
}

What you don't want to do is set an expectation on a mock method and then assert the response from the method under test. That would be two assertions in one test (although only one Assert call) and, if one thing goes wrong, it will be more difficult to figure out exactly what.

In a case like that, write two tests:

  • one to test that the method is called correctly
  • one to assert the contents of any response

In the latter, simply stub the method, so that it passes regardless of whether the method is called, as long as the response is correct.

TL;DR: Do not confuse asserting one thing with having one call to an Assert method. They're not the same thing.

share|improve this answer
    
Did your last sentence mean to say Test the expectation in one test and stub the method to assert the response object **in another test** ? I'm having a hard time following your answer though I think it might agree with me, I think you could use a TL;DR just to clarify because I'm finding it quite confusing.. –  Jimmy Hoffa Jan 3 '13 at 15:21
    
@JimmyHoffa: Well noted. Clearer now? –  pdr Jan 3 '13 at 15:31
    
Thanks, I think I understand the difference now. Useful distinction between Assert and asserting. –  glosrob Jan 3 '13 at 17:40
add comment

honestly the "one assert per test" rule is overrated, a better rule would be "on act per test" and have a smell when you have more than a few asserts which means that the act does too much

you have a method with a compound output so it makes sense to test all parts of the output. Doing this in a single test means that when a new field is required you only need to change 1 test instead of 3

I believe that you can add a string to the assert to be displayed when it fails, or you should at least see the line where it fails in the stacktrace. this way you can narrow it down what went wrong

besides the first relfex you should have should be to add a breakpoint before the first assert and run the test again and see what output you got

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.