37

I started writing some unit tests for my current project. I don't really have experience with it though. I first want to completely "get it", so I am currently using neither my IoC framework nor a mocking library.

I was wondering if there is anything wrong with providing null arguments to objects' constructors in unit tests. Let me provide some example code:

public class CarRadio
{...}


public class Motor
{
    public void SetSpeed(float speed){...}
}


public class Car
{
    public Car(CarRadio carRadio, Motor motor){...}
}


public class SpeedLimit
{
    public bool IsViolatedBy(Car car){...}
}

Yet Another Car Code Example(TM), reduced to only the parts important to the question. I now wrote a test something like this:

public class SpeedLimitTest
{
    public void TestSpeedLimit()
    {
        Motor motor = new Motor();
        motor.SetSpeed(10f);
        Car car = new Car(null, motor);

        SpeedLimit speedLimit = new SpeedLimit();
        Assert.IsTrue(speedLimit.IsViolatedBy(car));
    }
}

The test runs fine. SpeedLimit needs a Car with a Motor in order to do its thing. It is not interested in a CarRadio at all, so I provided null for that.

I am wondering if an object providing correct functionality without being fully constructed is a violation of SRP or a code smell. I have this nagging feeling that it does, but speedLimit.IsViolatedBy(motor) doesn't feel right either - a speed limit is violated by a car, not a motor. Maybe I just need a different perspective for unit tests vs. working code, because the whole intention is to test only a part of the whole.

Is constructing objects with null in unit tests a code smell?

  • 24
    A bit off-topic, but Motor probably shouldn't have a speed at all. It should have a throttle and compute a torque based on the current rpm and throttle. It's the car's job to use a Transmission to integrate that into a current speed, and to turn that into an rpm to signal back to the Motor... But I guess, you weren't in it for the realism anyway, were you? – cmaster Mar 20 '18 at 14:34
  • 17
    The code smell is that your constructor takes null without complaining in the first place. If you think this is acceptable (you might have your reasons for this): go ahead, test it! – Tommy Mar 20 '18 at 16:02
  • 4
    Consider the Null-Object pattern. It often simplifies code, while also making it NPE-safe. – Bakuriu Mar 20 '18 at 18:21
  • 17
    Congratulations: you have tested that with a null radio, the speed limit is correctly computed. Now you may want to create a test to validate the speed limit with a radio; just in case the behavior differs... – Matthieu M. Mar 21 '18 at 8:16
  • 3
    Not sure about this example, but sometimes the fact that you only want to test half a class is a clue that something should be broken up – Owen Mar 21 '18 at 14:08
70

In the case of the example above, it is reasonable that a Car can exist without a CarRadio. In which case, I'd say that not only is it acceptable to pass in a null CarRadio sometimes, I'd say that it's obligatory. Your tests need to ensure that the implementation of the Car class is robust, and does not throw null pointer exceptions when no CarRadio is present.

However, let's take a different example - let's consider a SteeringWheel. Let's assume that a Car has to have a SteeringWheel, but the speed test doesn't really care about it. In this case, I wouldn't pass a null SteeringWheel as this is then pushing the Car class into places where it isn't designed to go. In this case, you'd be better off creating some sort of DefaultSteeringWheel which (to continue the metaphor) is locked in a straight line.

  • 44
    And don't forget to write a unit test to check that a Car cannot be constructed without a SteeringWheel... – cmaster Mar 20 '18 at 14:28
  • 13
    I might be missing something, but if it's possible and even common to create a Car without a CarRadio, wouldn't it make sense to create a constructor that doesn't require one to be passed in and not have to worry about an explicit null at all? – an earwig Mar 20 '18 at 16:34
  • 16
    Self driving cars might not have a steering wheel. Just saying. ☺ – BlackJack Mar 20 '18 at 16:54
  • 1
    @James_Parsons I forgot to add that it was about a class which didn't have null checks because it was only used internally and always received non-null parameters. Other methods of Car would have thrown an NRE with a null CarRadio, but the relevant code for SpeedLimit wouldn't. In the case of the question as posted now you would be correct and I would indeed do that. – R. Schmitz Mar 20 '18 at 18:40
  • 2
    @mtraceur The way everybody assumed that the class allowing construction with a null argument means null is a valid option made me realize that I should probably have null checks there. The actual issue I would have then is covered by a lot of good, interesting, well-written answers here which I don't want to ruin and which did help me. Also accepting this answer now because I don't like to leave things unfinished and it's the most upvoted (although they all were helpful). – R. Schmitz Mar 21 '18 at 11:19
23

Is constructing objects with null in unit tests a code smell?

Yes. Note the C2 definition of code smell: "a CodeSmell is a hint that something might be wrong, not a certainty."

The test runs fine. SpeedLimit needs a Car with a Motor in order to do its thing. It is not interested in a CarRadio at all, so I provided null for that.

If you are trying to demonstrate that speedLimit.IsViolatedBy(car) does not depend on the CarRadio argument passed to the constructor, then it would probably be clearer to the future maintainer to make that constraint explicit by introducing a test double that fails the test if it is invoked.

If, as is more likely, you are just trying to save yourself the work of creating a CarRadio that you know isn't used in this case, you should notice that

  • this is a violation of encapsulation (you are letting the fact that CarRadio isn't used leak out into the test)
  • you have discovered at least one case where you want to create a Car without specifying a CarRadio

I strongly suspect that the code is trying to tell you that you want a constructor that looks like

public Car(IMotor motor) {...}

and then that constructor can decide what to do with the fact that there is no CarRadio

public Car(IMotor motor) {
    this(null, motor);
}

or

public Car(IMotor motor) {
    this( new ICarRadio() {...} , motor);
}

or

public Car(IMotor motor) {
    this( new DefaultRadio(), motor);
}

etc.

Its about if passing null to something else while testing SpeedLimit. You can see that the test is called "SpeedLimitTest" and the only Assert is checking a method of SpeedLimit.

That's a second interesting smell - in order to test SpeedLimit, you have to build an entire car around a radio, even though the only thing that the SpeedLimit check probably cares about is the speed.

This might be a hint that SpeedLimit.isViolatedBy should be accepting a role interface that car implements, rather than requiring an entire car.

interface IHaveSpeed {
    float speed();
}

class Car implements IHaveSpeed {...}

IHaveSpeed is a really lousy name; hopefully your domain language will have an improvement.

With that interface in place, your test for SpeedLimit could be much simpler

public void TestSpeedLimit()
{
    IHaveSpeed somethingTravelingQuickly = new IHaveSpeed {
        float speed() { return 10f; }
    }

    SpeedLimit speedLimit = new SpeedLimit();
    Assert.IsTrue(speedLimit.IsViolatedBy(somethingTravelingQuickly));
}
  • In your last example I presume you'd have to create a mock class that implements the IHaveSpeed interface as (at least in c#) you wouldn't be able to instantiate an interface itself. – Ryan Searle Mar 22 '18 at 20:16
  • 1
    That's right - the effective spelling will depend upon which flavor of Blub you are using for your tests. – VoiceOfUnreason Mar 22 '18 at 20:25
18

Is constructing objects with null in unit tests a code smell?

No

Not at all. To the contrary, if NULL is a value that is available as an argument (some languages may have constructs that disallow passing of a null pointer), you should test it.

Another question is what happens when you pass NULL. But that is exactly what a unit test is about: making sure a defined result is happening when for each possible input. And NULL is a potential input, so you should have a defined result and you should have a test for that.

So if your Car is supposed to be constructable without a radio, you should write a test for that. If it's not and is supposed to throw an exception, you should write a test for that.

Either way, yes you should write a test for NULL. It would be a code smell if you left it out. That would mean you have an untested branch of code that you just assumed would magically be bug-free.


Please note that I do agree with the other answers that your code under test could be improved. Nonetheless, if the improved code has parameters that are pointers, passing NULL even for the improved code is a good test. I would want a test to show what happens when I pass NULL as a motor. That's not a code smell, that's the opposite of a code smell. It's testing.

  • It seems like you are writing bout testing Car here. While I don't see anything I'd consider a wrong statement, the question is about testing SpeedLimit. – R. Schmitz Mar 20 '18 at 18:32
  • @R.Schmitz Not sure what you mean. I have quoted the question, it's about passing NULL to the constructor of Car. – nvoigt Mar 21 '18 at 15:39
  • 1
    Its about if passing null to something else while testing SpeedLimit. You can see that the test is called "SpeedLimitTest" and the only Assert is checking a method of SpeedLimit. Testing Car - for example "if your Car is supposed to be constructable without a radio, you should write a test for that" - would happen in a different test. – R. Schmitz Mar 21 '18 at 16:15
7

I personally would be hesitant to inject nulls. In fact, whenever I inject dependencies into a constructor, one of the first things I do is raise an ArgumentNullException if those injections are null. That way I can safely use them inside my class, without dozens of null-checks spread around. Here's a very common pattern. Note the use of readonly to ensure those dependencies set in constructor can not be set to null (or anything else) afterwards:

class Car
{
   private readonly ICarRadio _carRadio;
   private readonly IMotor _motor;

   public Car(ICarRadio carRadio, IMotor motor)
   {
      if (carRadio == null)
         throw new ArgumentNullException(nameof(carRadio));

      if (motor == null)
         throw new ArgumentNullException(nameof(motor));

      _carRadio = carRadio;
      _motor = motor;
   }
}

With the above, I could perhaps have a unit test or two, where I inject nulls, just to make sure my null checks work as expected.

Having said that, this becomes a non-issue, once you do two things:

  1. Program to interfaces instead of classes.
  2. Use a mocking framework (like Moq for C#) to inject mocked dependencies instead of nulls.

So for your example you'd have:

interface ICarRadio
{
   bool Tune(float frequency);
}

interface Motor
{
   bool SetSpeed(float speed);
}

...
var car = new Car(new Moq<ICarRadio>().Object, new Moq<IMotor>().Object);

More details on using Moq can be found here.

Of course, if you want to understand it without mocking first, you can still do it, as long as you are using interfaces. Simply create a dummy CarRadio and Motor as following, and inject those instead:

class DummyCarRadio : ICarRadio
{
   ...
}
class DummyMotor : IMotor
{
   ...
}

...
var car = new Car(new DummyCarRadio(), new DummyMotor())
  • 3
    This is the answer I would have written – Liath Mar 20 '18 at 14:17
  • 1
    I would go even so far to say that the dummy objects also have to fulfill their contract. If IMotor had a getSpeed() method, the DummyMotor would need to return a modified speed after a successful setSpeed as well. – ComFreek Mar 20 '18 at 14:41
1

It’s not just okay, it’s a well known dependency breaking technique for getting legacy code into a test harness. (See “Working Effectively with Legacy Code” by Michael Feathers.)

Does it smell? Well...

The test doesn’t smell. The test is doing its job. It’s declaring “this object can be null and the code under test still works correctly”.

The smell is in the implementation. By declaring a constructor argument, you’re declaring “this class needs this thing in order to properly function”. Obviously, that’s untrue. Anything untrue is a bit smelly.

1

Let's take a step back to first principles.

A unit test tests some aspect of a single class.

There will however be constructors or methods that take other classes as parameters. How you handle these will vary from case to case but setup should be minimal since they're tangential to the objective. There may be scenarios where passing a NULL parameter is perfectly valid - such as a car having no radio.

I'm assuming here, that we're just testing the racing line to start with (to continue the car theme), but you may also want to pass NULL to other parameters to check that any defensive code and exception handing mechanisms are functioning as required.

So far so good. However, what if NULL isn't a valid value. Well, here you're into the murky world of mocks, stubs, dummies and fakes.

If all this seems a bit much to take on, you're actually already using a dummy by passing NULL in the Car constructor since it a value that potentially won't be used.

What should become clear pretty quickly, is that you're potentially shotgunning your code with a lot of NULL handling. If you replace class parameters with interfaces, then you also pave the way for mocking and DI etc which make these far more straightforward to handle.

  • 1
    "Unit tests are for testing the code of a single class." Sigh. That is not what unit tests are and following that guideline is either going to lead you into bloated classes or counterproductive, hindering tests. – Ant P Mar 21 '18 at 22:32
  • 3
    treating "unit" as a euphemism for "class" is unfortunately a very common way of thinking but in reality all this does is (a) encourage "garbage-in, garbage-out" tests, which can totally undermine the validity of entire test suites and (b) enforce boundaries that should be free to change, which can cripple productivity. I wish more people would listen to their pain and challenge this preconception. – Ant P Mar 21 '18 at 22:40
  • 3
    No such confusion. Test units of behaviour, not units of code. There is nothing about the concept of a unit test - except in the widespread cargo cult hive mind of software testing - that implies that a test unit is coupled to the distinctly OOP concept of a class. And a very damaging misconception that is, too. – Ant P Mar 21 '18 at 22:48
  • 1
    I'm not sure precisely what you mean - I'm arguing the exact opposite of what you're implying. Stub/mock hard boundaries (if you absolutely have to) and test a behavioural unit. Through the public API. What that API is depends on what you're building but the footprint should be minimal. Adding abstraction, polymorphism or restructuring code should not cause tests to fail - "unit per class" contradicts this and utterly undermines the value of the tests in the first place. – Ant P Mar 21 '18 at 22:54
  • 1
    RobbieDee - I'm talking about the precise opposite. Consider that you may be the one harbouring common misconceptions, revisit what you consider a "unit" and maybe dig a little deeper into what Kent Beck was actually really talking about when he talked about TDD. What most people call TDD now is a cargo cult monstrosity. youtube.com/watch?v=EZ05e7EMOLM – Ant P Mar 22 '18 at 8:29
1

Taking a look at your design, I would suggest that a Car is composed of a set of Features. A Feature maybe a CarRadio, or EntertainmentSystem which may consist of things like a CarRadio, DvdPlayer etc.

So, at a minimum, you would have to construct a Car with a set of Features. EntertainmentSystem and CarRadio would be implementers of the interface (Java, C# etc) of the public [I|i]nterface Feature and this would be an instance of the Composite design pattern.

Therefore, you would always construct a Car with Features and a unit test would never instantiate a Car without any Features. Although you might consider mocking a BasicTestCarFeatureSet that has a minimal set of functionality required for your most basic test.

Just some thoughts.

John

protected by gnat Mar 22 '18 at 19:33

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

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