12

I have a bunch of classes which deal with validation of values. For instance, a RangeValidator class checks whether a value is within the specified range.

Every validator class contains two methods: is_valid(value), which returns True or False depending on the value, and ensure_valid(value) which checks for a specified value and either does nothing if the value is valid, or throws a specific exception if the value doesn't match the predefined rules.

There are currently two unit tests associated with this method:

  • The one which passes an invalid value, and ensures that the exception was thrown.

    def test_outside_range(self):
        with self.assertRaises(demo.ValidationException):
            demo.RangeValidator(0, 100).ensure_valid(-5)
    
  • The one which passes a valid value.

    def test_in_range(self):
        demo.RangeValidator(0, 100).ensure_valid(25)
    

Although the second test does its job—fails if the exception is thrown, and succeeds if ensure_valid doesn't throw anything—the fact that there is no asserts inside looks strange. Someone who reads such code would immediately ask himself why is there a test which appears to be doing nothing.

Is this a current practice when testing methods which don't return a value and don't have side effects? Or should I rewrite the test in a different way? Or simply put a comment explaining what I'm doing?

  • possibly related: How to not test implementation when method returns void? – gnat Jan 31 '17 at 21:31
  • 11
    Pedantic point, but if you have a function that takes no arguments (save for a self reference) and returns no result, it ain't a pure function. – David Arno Jan 31 '17 at 22:08
  • 10
    @DavidArno: This is not a pedantic point, it goes right to the heart of the question: the method is hard to test precisely because it is impure. – Jörg W Mittag Jan 31 '17 at 22:46
  • @DavidArno More pedantic point, you could have the "do nothing" method (assuming "returns no results" is interpreted as "returns a unit type" which is called void in many languages and has silly rules.) Alternatively, you could have an infinite loop (which works even when "returns no result" really means returns no results. – Derek Elkins Feb 1 '17 at 1:51
  • There is one pure function of type unit -> unit in any language which considers unit a standard datatype instead of something magically special-cased. Unfortunately, it just returns unit and does nothing else. – Phoshi Feb 1 '17 at 12:33
22

Most test frameworks have an explicit assertion for "Doesn't throw", e.g. Jasmine has expect(() => {}).not.toThrow(); and nUnit and friends also have one.

  • 1
    And if the test framework doesn't have such assertion, it's always possible to create a local method which does exactly the same thing, making the code self-explanatory. Good idea. – Arseni Mourzenko Jan 31 '17 at 22:20
7

This strongly depends on the language and framework used. Speaking in terms of NUnit, there are Assert.Throws(...) methods. You can pass them a lambda method:

Assert.Throws(() => rangeValidator.EnsureValid(-5))

which is executed within the Assert.Throws. The call to the lambda will most likely be wrapped by a try { } catch { } block and the assertion fails, if an exception is caught.

If your framework does not provide these means, you could work it around, by wrapping the call by yourself (I'm writing in C#):

// assert that the method does not fail
try
{
    rangeValidator.EnsureValid(50);
}
catch(Exception e)
{
    Assert.IsTrue(false, $"Exception: {e.Message}");
}

// assert that the method does fail
try
{
    rangeValidator.EnsureValid(50);
    Assert.IsTrue(false, $"Method is expected to throw an exception");
}
catch(Exception e)
{
}

This makes the intention more clear, but clutters the code to a certain extent. (Of course you can wrap all this stuff in a method.) At the end it will be up to you.

Edit

As Doc Brown pointed out in the comments, the issue has not been to indicate that the method throws, but that it does not throw. In NUnit there is also an assertion for that

Assert.DoesNotThrow(() => rangeValidator.EnsureValid(-5))
1

You could also assert that some methods are called (or not called) properly.

For instance:

public void SendEmail(User user)
     ... construct email ...
     _emailSender.Send(email);

In your test:

emailSenderMock.VerifyIgnoreArgs(service =>
    service.Send(It.IsAny<Email>()),
    Times.Once
);
0

Just add a comment to make it clear why no assert is needed and why you didn't forget it.

As you can see in the other answers, anything else makes the code more complicated and cluttered. With the comment there, other programmers will know the intent of the test.

That being said, this kind of test should be an exception (no pun intended). If you find yourself writing something like that regularly, then probably the tests are telling you that the design is not optimal.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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