4

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?

6
  • 2
    Why not just have different overloads of Foo that accept different parameter subsets, and default the missing parameters? If you're already working from a fixed API for Foo, then yes, I'd say this is a sensible approach. Aug 11, 2011 at 19:19
  • In production, Foo will always be called with meaningful values for all parameters. It makes no sense to have a different overload of Foo in production - it will never be used.
    – Vegar
    Aug 11, 2011 at 19:44
  • If you are doing the same test with different parameters, see if the framework can provide any leverage for you there ... in the form of [RowTest] attribute maybe?
    – Job
    Aug 11, 2011 at 19:54
  • Unfortunately, the default test framework coming with visual studio do not have this feature. In this case, it would reduce the amount of tests to some degree, but not much. Even though all tests call foo, they test different parts. Some tests the cases of failure, some tests the returned value, some verifies that calls to underlying services are invoked with the right parameters and so on.
    – Vegar
    Aug 11, 2011 at 20:06
  • I see ... though MsTest sort of has this feature: codeclimber.net.nz/archive/2008/01/18/…
    – Job
    Aug 11, 2011 at 21:37

3 Answers 3

2
//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)=>{});
4
  • Yes, this is what I am afraid of - that it hides to much, making it hard to see that it is Foo that really gets called. But then again, by doing the call directly, fooObject.Foo("", 0, (bar)=>{}, (bar)=>{});, how do you choose values that you don't care about? Does passing a 0 for the second parameter mean something? Is a empty string important for the test? In one test it may be, in another it may not, and I struggle to communicate that in a good way. By only passing the parameters that really matters for this test, it's clear that the test cares for those values.
    – Vegar
    Aug 30, 2011 at 18:04
  • I don't think Act is the first place to look at when trying to determine which variables are important for the test. According to some definitions of Arrange Act Assert, all preconditions and input parameters should be declared in the Arrange section. Thus you can be sure that all anonymous variables passed as parameters in the Act part are unimportant to the test whereas parameters that were declared in Arrange do matter. (See c2.com/cgi/wiki?ArrangeActAssert) Test method name is also often a good place to look at to get an idea of the relevant context (..._When_<Some_Conditions>) Aug 31, 2011 at 8:12
  • So what your're saying, is that if e.g. the first callback was important for this test, it should be assigned to a local variable in the arrange-section first? Since it's used literals for the first two parameters in your example, that indicates that the values are not important? Hum... I disagree. I can't see that this would be a clear and predictable pattern. I think only complex input needs to be initialized in the arrange-section. Or if the input is reused in the assert-section.
    – Vegar
    Aug 31, 2011 at 13:12
  • Yes, my example was to show a case where we don't care about any of the parameters. If you think that only complex input needs to be initalized in the Arrange section, then there's probably no solution to your problem since you're doomed to have an Act that is as muddled as the sum of your least-complex inputs. I don't find it to be a big deal or something that would be worth the burden of a complicated workaround, though. Aug 31, 2011 at 14:05
0

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()
   {
      //...
   }
}
5
  • So basically, you have chopped all the tests into subclasses and hid the relevant parts of the method call under test in an class initializer and the call itself in a base class. How will that do anything for the test, but make it even harder to understand whats being tested and how the class works?
    – Vegar
    Aug 15, 2011 at 21:07
  • It makes it clear exactly what you are testing and you hide the mess. You write less code, have less duplication, and spec style tests with good names are almost always easier to follow.
    – Travis
    Aug 16, 2011 at 11:40
  • how do you find it more clear? When I look at it, I think you have hidden what you are testing completely by setting the relevant values in the classinitializer instead of in the test and replaced the tested method with an Act()-statement. By dividing the tests into subclasses based on what input parameters it uses instead of what area is tested, it makes it even harder to get an complete picture of what the method does and how it works. Please tell me why you find this more clear and why it should result in less code.
    – Vegar
    Aug 16, 2011 at 11:53
  • Well, that's a reasonable question. I find it easier to really understand what you are testing when there's less to read. An example of this played out in more depth: github.com/MassTransit/MassTransit/blob/master/src/… . A simple setup for each test (though for details you do have to dig, but you don't care about the complexity most of the time when changing stuff), each test is small and discrete. And lastly, it results in less code because you don't have to write most of the setup and act portions of the code for each test.
    – Travis
    Aug 16, 2011 at 13:45
  • I have browsed through a couple of tests from MassTransit, and what I miss, is a more clear 'Arrange-Act-Assert'-pattern. I found it hard to see what part of the code that was boilerplating and what was the actual use of the class under test and what was the check to see if it really worked. It may be that the spec pattern has a different form and purpose.
    – Vegar
    Aug 18, 2011 at 10:20
0

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.

4
  • [ExpectedException] want do any good in this case. The errorcallback is part of the interface of the class under test. The Foo method will trap any exception, and pass it back through the error callback. This is a common pattern when dealing with async service calls, where exceptions can be thrown in a different thread, or even a different machine all together.
    – Vegar
    Aug 30, 2011 at 18:12
  • Regarding commens - I usually don't have Arrange-Act-Assert-comments, or any comments at all, but put them in here to take away any doubt of whats going on.
    – Vegar
    Aug 30, 2011 at 18:21
  • I can't see your point on 'PerformBar being too general', though. Can you clarify? How is it too general? It can't be used for anything but invoking Bar with the given parameters.
    – Vegar
    Aug 30, 2011 at 18:24
  • Btw, being ignorant of the [Combinatorial]-feature of nunit, I find it really hard to understand what your tests actually tests. Guess I have to add this to my learning-list.
    – Vegar
    Aug 30, 2011 at 18:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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