30

I have a unit test called TestMakeAValidCall(). It tests my phone app making a valid call.

I am about to write another test called TestShowCallMessage() that needs to have a valid call made for the test. Is it bad form to just call TestMakeAValidCall() in that test?

For reference this is my TestMakeAValidCall() test.

    [TestMethod]
    public void TestMakeAValidCall()
    {
       //Arrange
        phone.InCall = false;
        phone.CurrentNumber = "";
        // Stub the call to the database
        data.Expect(x => x.GetWhiteListData()).
            Return(FillTestObjects.GetSingleEntryWhiteList());
        // Get some bogus data
        string phoneNumber = FillTestObjects.GetSingleEntryWhiteList().
            First().PhoneNumber;
        // Stub th call to MakeCall() so that it looks as if a call was made.
        phone.Expect(x => x.MakeCall(phoneNumber)).
            WhenCalled(invocation =>
                       {
                           phone.CurrentNumber = phoneNumber;
                           phone.InCall = true;
                       });

       //Act
        // Select the phone number
        deviceControlForm.SelectedNumber = phoneNumber;
        // Press the call button to make a call.
        deviceMediator.CallButtonPressed();

       //Assert
        Assert.IsTrue(phone.InCall);
        Assert.IsTrue(phone.CurrentNumber == phoneNumber);
    }
Vaccano
  • 78,325
  • 149
  • 468
  • 850
  • Thanks for all the great answers. I refactored out the duplicate code to a separate call. I picked the answer with the most votes. – Vaccano Sep 02 '09 at 17:17

8 Answers8

64

Refactor the setup to another method and call that method from both tests. Tests should not call other tests.

tvanfosson
  • 524,688
  • 99
  • 697
  • 795
  • 4
    Dont test another test with test – Tom Smykowski Feb 11 '13 at 11:05
  • Could someone ellaborate on how the refactoring would look like in this case? – Philip Nov 28 '15 at 15:33
  • 1
    @Nakata - hard to show the exact code for this example without knowing how the dependencies are structured, but this gist shows the pattern I use to put set up code in a test context and invoke it from there in multiple tests. https://gist.github.com/tvanfosson/1ca6dd3bb0b796de65b0 – tvanfosson Nov 28 '15 at 16:10
  • Thank you, is it a good convention to put the SetupCall-method in a nested class? – Philip Nov 28 '15 at 16:23
  • 1
    @Nakata - using the inner class restricts access to its public methods to the containing class. I do this to limit coupling between the test classes, removing the temptation to over-engineer them to meet the needs of unrelated test classes. If you truly have code that can be reused, move it to a base test context that is outside the test class(es) and extend the inner test contexts from that base class. – tvanfosson Nov 28 '15 at 16:27
11

IMHO, you should do one of the following:

  • Create a method that returns a valid call, and use it separately for both tests (not one calling the other)
  • Mock the valid call for the ShowCallMessageTest
Samuel Carrijo
  • 17,449
  • 12
  • 49
  • 59
7

To offer a counter point:

I strongly believe that well designed unit test should depend on one another!

Of course, that makes sense only if the testing framework is aware of these dependencies such that it can stop running dependent test when a dependency fails. Even better, such a framework can pass the fixture from test to test, such that can build upon a growing and extending fixture instead of rebuilding it from scratch for each single test. Of course, caching is done to take care no side-effects are introduced when more than one test depends from the same example.

We implemented this idea in the JExample extension for JUnit. There is no C# port yet, though there are ports for Ruby and Smalltalk and ... the most recent release of PHPUnit picked up both our ideas: dependencies and fixture reuse.

PS: folks are also using it for Groovy.

akuhn
  • 27,477
  • 2
  • 76
  • 91
6

I think its a bad idea. You want your unit test to test one thing and one thing only. Instead of creating a call through your other test, mock out a call and pass it in as an argument.

Rob Allen
  • 17,381
  • 5
  • 52
  • 70
4

A unit test should test one unit/function of your code by definition. Having it call other unit tests makes it test more than one unit. I break it up in to individual tests.

Vince
  • 7,608
  • 3
  • 41
  • 46
2

Yes - unit tests should be separate and should aim to test only one thing (or at least a small number of closely-related things). As an aside, the calls to data.Expect and phone.Expect in your test method are creating expectations rather than stub calls, which can make your tests brittle if you refactor...

Lee
  • 142,018
  • 20
  • 234
  • 287
1

A unit vs. module....we also think tests should depend on reusable methods as well and should test at an api level testing integration of classes. Many just tests a single class but many bugs are at that integration between the class level. We also use verifydesign to guarantee the api does not depend on implementation. This allows you to refactor the whole component/module without touching a test(and we went through that once actually and it worked great). Of course, any architectural changes force you to refactor the tests but at least design changes in the module don't cause test refactor work(unless you change the behavior of the api of course implicitly like firing more events than you used to but that "would" be an api change anyways).

Dean Hiller
  • 19,235
  • 25
  • 129
  • 212
0

"Could someone ellaborate on how the refactoring would look like in this case? – Philip Bergström Nov 28 '15 at 15:33"

I am currently doing something like this and this is what i came up with:

Notice that ProcessorType and BuildProcessors both call TestLevels

the actual content besides that fact is unimportant

its using XUnit, and Shouldly NuGet package

private static void TestLevels(ArgProcessor incomingProcessor)
{
    Action<ProcessorLevel, int> currentLevelIteration = null;
    currentLevelIteration = (currentProcessor, currentLevel) =>
    {
        currentProcessor.CurrentLevel.ShouldBeEquivalentTo(currentLevel);
        ProcessorLevel nextProcessor = currentProcessor.CurrentProcessor;
        if (nextProcessor != null)
            currentLevelIteration(nextProcessor, currentLevel + 1);
    };

    currentLevelIteration(incomingProcessor, 0);
}

[Theory]
[InlineData(typeof(Build), "Build")]
public void ProcessorType(Type ProcessorType, params string[] args)
{
    ArgProcessor newCLI = new OriWeb_CLI.ArgProcessor(args);

    IncomingArgumentsTests.TestLevels(newCLI);

    newCLI.CurrentProcessor.ShouldBeOfType(ProcessorType);
}

[Theory]
[InlineData(typeof(Build.TypeScript), "TypeScript")]
[InlineData(typeof(Build.CSharp), "CSharp")]
public void BuildProcessors(Type ProcessorType, params string[] args)
{
    List<string> newArgs = new List<string> {"Build"};
    foreach(string arg in args) newArgs.Add(arg);

    ArgProcessor newCLI = new OriWeb_CLI.ArgProcessor(newArgs.ToArray());

    IncomingArgumentsTests.TestLevels(newCLI);

    newCLI.CurrentProcessor.CurrentProcessor.ShouldBeOfType(ProcessorType);
}
MatthewOrionos
  • 29
  • 4