2

Some time ago I had to code review a unit-test fixture like this one (C# code):

public class ServiceFixture : Service
{
    [Fact]
    public void Test_DoSomething()
    {
        Assert(DoSomething(), SomeExpectedResult);
    }
}

So Service is the SUT, the class under test, and ServiceFixture is the test fixture. And ServiceFixture inherits from Service.

This is, by the way, a really non-orthodox way of writing a test fixture (at least in C#, Java, Scala, languages that I am more acquainted with). Usually the SUT is instantiated by the the test fixture, as fair as my experience tells...

I had a hard time convincing the author that this should not be done this way though. I mean, the main reason I can think of against it is (besides "non usual") the unnecessary coupling between fixture and SUT (which is related to the old Inheritance vs. Aggregation discussion).

What else could be said about it? Are any arguments for this kind of practice, or can we agree it should be avoided?

rsenna
  • 348
  • 2
  • 11

1 Answers1

1

This is a problematic way to write tests because the test doesn't specify instantiation of the SUT. The Fixture and implicitly with it the SUT will be instantiated by the testing framework. This leads to the following problems:

  • You cannot provide constructor arguments.
  • Following from that, you cannot set up complicated object graphs, or do manual dependency injection.
  • This leads to problems if the SUT instance should only be created through a factory, or needs to be registered somewhere in your application in order to work correctly.
  • The same object is reused for all test cases (perhaps – depends on testing framework).
  • You cannot reset the SUT state for the next test unless it happens to be mutable.

To avoid such problems, I recommend setting up the SUT within each test case, and only rely on automatic fixtures if they simplify the tests noticeably.

As you already mentioned, the use of inheritance itself might be problematic.

  • The fragile base class problem: benign changes to the base class could break derived classes. C# makes this hard to do, but it's not impossible. In all languages: a class should be explicitly designed for inheritance if it is to be used as a base class.
  • The additions in the derived class might accidentally modify the object's behaviour. Again, C# makes this difficult but not impossible. If your service is subject to reflection or may implement optional interfaces, additions such as having your fixture inherit from IDisposable might have unintended consequences.

There are cases where inheritance might be the correct solution: when the SUT is intended as a base class and your tests need access to protected members. But otherwise, it's better to keep some distance between your production code and tests.

In addition, this is a non-obvious, and unnecessarily clever way to write tests. Surprises are bad. As discussed in How would you know if you've written readable and easily maintainable code?, there are a couple of indicators whether this approach is good or bad. An excerpt:

  • Your peer tells you after reviewing the code.

    You told your colleague this might not be ideal, which is a good indicator that it isn't.

  • It is:

    1. maintainable if you can maintain it.
    2. easily maintainable if someone else can maintain it without asking you for help
    3. readable if someone else, on reading it, correctly understands the design, layout and intent

    I do not understand why the test had to be written in this unorthodox way. Surely there was a special reason for this? I'd have to ask.

  • Watch what happens after handing it off to a reasonably competent person. If they don't need to ask many questions relative to the difficulty of the code, you've done a good job.

    This is very simple code, yet it raises questions.

By leaving this unnecessary code in the code base, you are also setting a bad example. By itself, this inheritance is not really harmful. But in other cases, the abovementioned drawbacks might impact the validity of the tests.

amon
  • 132,749
  • 27
  • 279
  • 375