views:

527

answers:

6

I am currently writing some unit tests for a business-logic class that includes validation routines. For example:

public User CreateUser(string username, string password, UserDetails details)
{
    ValidateUserDetails(details);
    ValidateUsername(username);
    ValidatePassword(password);

    // create and return user
}

Should my test fixture contain tests for every possible validation error that can occur in the Validate* methods, or is it better to leave that for a separate set of tests? Or perhaps the validation logic should be refactored out somehow?

My reasoning is that if I decide to test for all the validation errors that can occur within CreateUser, the test fixture will become quite bloated. And most of the validation methods are used from more than one place...

Any great patterns or suggestions in this case?

+9  A: 

Every test should only fail for one reason and only one test should fail for that reason.

This helps a lot with writing a maintainable set of unit tests.

I'd write a couple of tests each for ValidateUserDetails, ValidateUsername and ValidateUserPassword. Then you only need to test that CreateUser calls those functions.


Re read your question; Seems I misunderstood things a bit.

You might be interested in what J.P Boodhoo has written on his style of behaviour driven design. http://blog.jpboodhoo.com/HowIrsquomCurrentlyWritingMyBDDStyleTestsNdashPart2.aspx

BDD is becoming a very overloaded term, everyone has a different definition and different tools to do it. As far as I see what JP Boodhoo is doing is splitting up test fixtures according to concern and not class.

For example you could create separate fixtures for testing Validation of user details, Validation of username, Validation of password and creating users. The idea of BDD is that by naming the testfixtures and tests the right way you can create something that almost reads like documentation by printing out the testfixture names and test names. Another advantage of grouping your tests by concern and not by class is that you'll probably only need one setup and teardown routine for each fixture.

I havn't had much experience with this myself though.

If you're interested in reading more, JP Boodhoo has posted a lot about this on his blog (see above link) or you can also listen to the dot net rocks episode with Scott Bellware where he talks about a similar way of grouping and naming tests http://www.dotnetrocks.com/default.aspx?showNum=406

I hope this is more what you're looking for.

Mendelt
I rephrased my question a bit - please read again
JacobE
+2  A: 
  • Let Unit Tests (plural) against the Validate methods confirm their correct functioning.
  • Let Unit Tests (plural) against the CreateUser method confirm its correct functioning.

If CreateUser is merely required to call the validate methods, but is not required to make validation decisions itself, then the tests against CreateUser should confirm that requirement.

David B
+1  A: 

You definitely need to test validation methods.

There is no need to test other methods for all possible combinations of arguments just to make sure validation is performed.

You seem to be mixing Validation and Design by Contract.

Validation is usually performed to friendly notify user that his input is incorrect. It is very related to business logic (password is not strong enough, email has incorrect format, etc.).

Design by Contract makes sure your code can execute without throwing exceptions later on (even without them you would get the exception, but much later and probably more obscure one).

Regarding application layer that should contain validation logic, probably the best is service layer (by Fowler) which defines application boundaries and is a good place to sanitize application input. And there should not be any validation logic inside this boundaries, only Design By Contract to detect errors earlier.

So finally, write validation logic tests when you want to friendly notify user that he has mistaken. Otherwise use Design By Contract and keep throwing exceptions.

Konstantin Spirin
A: 

I would add a bunch of test for each ValidateXXX method. Then in CreateUser create 3 test cases for checking what happens when each of ValidateUserDetails, ValidateUsername and ValidatePassword fails but the other succeed.

erikkallen
A: 

I'm using Lokad Shared Library for defining business validation rules. Here's how I test corner cases (sample from the open-source):

[Test]
public void Test()
{
  ShouldPass("[email protected]", "pwd", "http://ws.lokad.com/TimeSerieS2.asmx");
  ShouldPass("[email protected]", "pwd", "http://127.0.0.1/TimeSerieS2.asmx");
  ShouldPass("[email protected]", "pwd", "http://sandbox-ws.lokad.com/TimeSerieS2.asmx");

  ShouldFail("invalid", "pwd", "http://ws.lokad.com/TimeSerieS.asmx");
  ShouldFail("[email protected]", "pwd", "http://identity-theift.com/TimeSerieS2.asmx");
}

static void ShouldFail(string username, string pwd, string url)
{
  try
  {
    ShouldPass(username, pwd, url);
    Assert.Fail("Expected {0}", typeof (RuleException).Name);
  }
  catch (RuleException)
  {
  }
}

static void ShouldPass(string username, string pwd, string url)
{
  var connection = new ServiceConnection(username, pwd, new Uri(url));
  Enforce.That(connection, ApiRules.ValidConnection);
}

Where ValidConnection rule is defined as:

public static void ValidConnection(ServiceConnection connection, IScope scope)
{
  scope.Validate(connection.Username, "UserName", StringIs.Limited(6, 256), StringIs.ValidEmail);
  scope.Validate(connection.Password, "Password", StringIs.Limited(1, 256));
  scope.Validate(connection.Endpoint, "Endpoint", Endpoint);
}

static void Endpoint(Uri obj, IScope scope)
{
  var local = obj.LocalPath.ToLowerInvariant();
  if (local == "/timeseries.asmx")
  {
    scope.Error("Please, use TimeSeries2.asmx");
  }
  else if (local != "/timeseries2.asmx")
  {
    scope.Error("Unsupported local address '{0}'", local);
  }

  if (!obj.IsLoopback)
  {
    var host = obj.Host.ToLowerInvariant();
    if ((host != "ws.lokad.com") && (host != "sandbox-ws.lokad.com"))
      scope.Error("Unknown host '{0}'", host);
  }

If some failing case is discovered (i.e.: new valid connection url is added), then the rule and the test gets updated.

More on this pattern could be found in this article. Everything is Open Source so feel free to reuse or ask questions.

PS: note that primitive rules used in this sample composite rule (i.e. StringIs.ValidEmail or StringIs.Limited) are thoroughly tested on their own and thus do not need excessive unit tests.

Rinat Abdullin
+2  A: 

What is the responsibility of your business logic class and does it do something apart from the validation? I think I'd be tempted to move the validation routines into a class of its own (UserValidator) or multiple classes (UserDetailsValidator + UserCredentialsValidator) depending on your context and then provide mocks for the tests. So your class now would look something like:

public User CreateUser(string username, string password, UserDetails details)
{
    if (Validator.isValid(details, username, password)) {
       // what happens when not valid
    }

    // create and return user
}

You can then provide seperate unit tests purely for the validation and your tests for the business logic class can focus on when validation passes and when validation fails, as well as all your other tests.

MrWiggles