UnitTests, will cleaning up your act-statement make your test more or less clear?

Question

Lets say that we are testing FooClass with the following method:

    public void Foo(string stringParameter, int intParameter, Action<Bar> successCallback, Action<Exception> errorCallback);

If the call to Foo succeeds, the successCallback will be called with the result of Foo in the form of a Bar object. If it fails, the errorCallback will be called with an Exception.

So the tests will look something like this:

[TestMethod]
public void Foo_UnderGivenConditions_WeExpectAGivenResult()
{
    //Arrange
    var fooObject = CreateFooObjectWithGivenConditions( ... );

    //Act
    fooObject.Foo(String.Empty, 0, (bar) => { ... }, (error) => { ... });

    //Assert
    Assert.AreEqual(..., ...);
}

Now, there are a lot of tests on this Foo-method, all of them containing a call to Foo, but not every test will care for all the parameters. Some may provide different string values, but doesn't care about the int param, and some will need to provide an error callback to assert that the right exception is thrown and so on.

Now, since all the parameters are mandatory, we have to pass a string value to the Foo method, even though the value has no meaning for the test. There will be a lot of "I don't care" or "some text". When some one else comes and read the test, they have to consider if the given value actually has a meaning for the test result or not. The same goes for the callbacks. Sometimes we need the callbacks to get to the result value or the exception, but most of the time we do not.

So lets implement an extension method, PerformFoo, that defaults every parameter:

public static Bar PerformBar(this FooClass fooObject, string stringParameter = "some text", int intParameter = 0, Action<Bar> successCallback = null, Action<Exception> errorCallback = null);
{
   Bar result = null;

   var ourCallback = (bar) => 
      { 
          result = bar; 
          if (successCallback != null) 
              sucessCallback.invoke();
      }

   fooObject.Foo(stringParameter, intParameter, ourCallback, errorCallback);

   return result;
}

The extension method will call Foo with default parameters, and even return the result if the successCallback is called. This lets us change the Act part of the test to something like:

//Act, all we care about is the string parameter:
fooObject.PerformFoo("A string that we care about");

//Act, we need the resulting bar when the int parameter is 10:
var bar = fooObject.PerformFoo(intParameter: 10);

//Act, we still needs to provide a callback to get the exception 
fooObject.PerformFoo("SomeInvalidValueCausingAnException", errorCallback: (error) => { exceptionThrown = error; }); 

So the questions would be:

• Does this make the tests more readable?
• Is it easier to get what the test really tests?
• The fact that we call PerformFoo, which doesn't really exist on the class under test make the test less worth as an documentation?
• Would dropping the extension method for a regular method taking the fooObject as the first parameter be less of a 'lie'? (e.g. PerformFoo(fooObject, intParameter: 10) )

How far would you go to make your tests clean and clear?

Answer 1

//Act, we need the resulting bar when the int parameter is 10:
var bar = fooObject.PerformFoo(intParameter: 10);

As a developer coming across this unit test, it'd take me a few seconds to get beyond that "proxy" method and figure out which behavior of the class under test is actually tested, as there is no direct mention of the original method. So I wouldn't say it is more readable than a test using the real thing. This additional layer is also error prone. In my opinion, the method used in the Act section should always be the very method we want to test.

In your original example, you use anonymous variables to make irrelevant parameters more transparent, I think it's the sensible way to go to simplify your tests, like

fooObject.Foo("", 0, (bar)=>{}, (bar)=>{});

Answer 2

If I were doing this, I would take a look at having a base setup that acts by calling Foo(). A base class would contain the Arrange with defaults, the Act would be in the base, and the assert would have to be assumed in the child test classes.

[TestClass] //you don't need this here but the 
            //compiler warns otherwise with mstest :(
public void Base_Fooey_fixture
{
   protected String StringParameter { get; set; }
   protected int IntParameter { get; set; }
   // ...
   [ClassInitialize]
   public void Arrange()
   {
     StringParameter = "Default Value";
     //...
   }
   [TestInitialize]
   public void Act()
   {
    Foo(StringParameter, IntParameter, SuccessCallback, ErrorCallback);  
   }
}
[TestClass]
public class RealTest : Base_Fooey_fixture
{
   [ClassInitialize]
   public class MyArrange()
   {
     StringParameter = "My real string value";
   }

   [TestMethod]
   public void Im_gonna_assert_for_this_setup()
   {
      //...
   }
}

Answer 3

Not sure if using the callbacks for exception assertion is any better then [ExpectedException] attribute.

For a test code, it's not any different from any other code in that you want it to be clear.

In my tests, I usually apply programming principles to the test code which i find productive without hesitation.

Composition is a powerful tool to form ubiquitous language specific to the domain model, but to my taste, your PerformBar method is too general. I usually end up with more specialized composition methods.

public class RandomQueueGeneratesSequence
{
 private IEnumerable<T> ActualResult<T>(IEnumerable<T> source, int seed)
 {
  // Setup
  var q = new RandomQueue(new Random(seed));

  // Act
  foreach(var i in source)
    q.Add(i);
  var l = new List<T>();
  while( q.HasItems )
  {
   l.Add(q.GetItem());
  }
  return l;
 }
 [Test, Combinatorial] 
 public void ContainingAllSourceItems([Values(0,1,12345)] int seed)
 {
  CollectionAssert.AreEqualent(new[]{1,2,3}, ActualResult(new[]{1,2,3},seed));
 }
 [Test, Combinatorial]
 public void WithSingleItem([Values(0,1)] expectedValue, [Values(0,1,12345)] int seed)
 {
  Assert.AreEqual(expectedValue, ActualResult(new[]{expectedValue}, seed).Single());
 }
 [Test]
 public void InOrderBySeed1()
 {
  CollectionAssert.AreEqual(new[]{1,2}, ActualResult(new[]{1,2}, 123));
 }
 [Test]
 public void InOrderBySeed2()
 {
  CollectionAssert.AreEqual(new[]{2,1}, ActualResult(new[]{1,2}, 456));
 }
}

In this example, it's not reasonable to compose the asserting methods, because different ones are used (Assert.AreEqual, CollectionAssert.AreEqual, CollectionAssert.AreEqualent).

But if there's more then two tests follow some pattern, found in InOrderBySeed, move an assert to a composing method, and name it an Assert:

 protected void AssertInOrder<T>(IEnumerable<T> expected, IEnumerable<T> source, int seed)
 {
  CollectionAssert.AreEqual(expected, ActualResult(source, seed));
 }

 public void InOrderBySeed1()
 {
  AssertInOrder(new[]{1,2}, new {1,2}, 123);
 }
 [Test]
 public void InOrderBySeed2()
 {
  AssertInOrder(new[]{2,1}, new[]{1,2}, 456);
 }
 [Test]
 public void InOrderBySeed3()
 {
  AssertInOrder(new[]{3,1,2}, new[]{1,2,3}, 789);
 }

Generally, when your test code becomes clear enough, you will find yourself not needing comments anymore. Especially when the intent is clear from a method name. In this example - Assert prefix obsoletes the // Assert comment.