5

Imagine I have a function like this (written in Java):

void sayHello(String firstName, String lastName) {
    if (firstName == null) {
        throw new IllegalArgumentException("first name is null");
    }
    if (lastName == null) {
        throw new IllegalArgumentException("last name is null");
    }
    System.out.println("Hello " + firstName + " " + lastName);
}

I want to write unit tests that verify that exceptions are thrown in the correct scenarios, so I write a unit test like this:

@Test
void sayHello_last_name_null_Test() {
    IllegalArgumentException exception = Assertions.assertThrows(
            IllegalArgumentException.class,
            () -> sayHello("Tom", null),
            "Exception was not thrown"
    );
    Assertions.assertEquals("last name is null", exception.getMessage());
}

Testing that the exception's message matches precisely seems brittle, since this string is meant to be human readable. A developer could later make a trivial change to the message, such as "last name must not be null" and this would break the unit tests, even though the message's meaning is still correct.

You could argue that you just shouldn't worry about the contents of the messages in the test (and only check that an exception was thrown), but if sayHello("Tom", null) returns an IllegalArgumentException with message "first name is null" I would consider that a bug, and I would like a unit test to catch this.

What's best practice for these scenarios?

Improve this question
7
  • 2
    Maybe you shouldn’t be doing null checks to begin with; that’s fallacy number 40. kalzumeus.com/2010/06/17/… Commented Nov 14 at 0:13
  • 3
    @RikD: feel free to write an answer how to solve the OPs problem without null checks and without comprimising readable error messages. Commented Nov 14 at 10:01
  • @DocBrown there would be no checks or errors, because nameless users would be accepted as valid. RkiD specifically did not write the answer, because the question could be generalized to any parameter verification, not just names. Commented Nov 14 at 10:04
  • To close voter: this question is focused. Try harder! Commented Nov 14 at 10:15
  • 1
    @Basilevs: I am sure I know who the close voter is, and it seems that person has lost any interest in trying to think hard whether a question could be kept open or not, or why. As soon as the question contains a buzzword like "what's best practice", it gets a close vote "needs more focus" automatically. Commented Nov 14 at 10:49

5 Answers 5

Reset to default
10

From my own personal experience, you should only be building unit tests for things that you do not want anyone to change without also modifying the unit tests. That might seem tautological so I'll give an example. If a function produces a list of items, should I test that the items are in a list? Should I:

  1. Should I test that the items are in a list. That is, check the type of the returned collection?
  2. Check that they are in a specific order?
  3. Should I check what items are in the list?

And the answer is, it depends. Is it important that the type be a list or a specific list implementation? Probably not, but if I know that the type of the returned list is important for the overall design to work, it should be part of a unit test. If the order matters, then yes, that should be tested. And it's likely that you would want to check what items are returned in a unit test but there might be times you don't care.

In a nutshell, you want to be testing for invariant constraints. That is, things that you are assuming to be true elsewhere should be covered in unit tests. Or, stated more weakly, only those things should be unit tested.

So to the specific question. Does anything depend on the text of the error message? In the normal course of things, generally not. However, if you have something that depends on the content of the error message, you may want to check it in your code. Now, the obvious reason that it might matter is because of your requirements e.g., a specific message should be displayed to the user. I don't think that's an especially good reason to add that kind of check to a unit test. That's probably better handled in a different kind of test.

But there are other scenarios where it might matter. For example, say that you have code that needs to execute different actions based on the content of the exception message. That would be a really good reason to add a unit test. I would even consider that a priority unit test because most devs would not expect the message to be used in that way. And yes, it's probably not a great design but perhaps you have some legacy code and you can't do anything about it. Even more reason to make it hard to change the text. If you were using something like numeric codes instead, you would want to check that the correct code is returned depending on the issue. It's essentially the same situation.

On the other hand, if this is just informational and the exact content of the error message is cosmetic, I would not add that as a unit test.

Improve this answer
3
  • To tie this in with the answer by @ArseniMourZenko, if you are typing the same string at three places (when throwing the exception, in the test and in the logic that uses the exception string) that should be a trigger to DRY-out your code and refactor that string into a named constant. Commented Nov 14 at 7:39
  • This answer seem to miss the existence and application of Throwable.getLocalizedMessage() Commented Nov 14 at 9:57
  • @Basilevs It sure does. Why is that relevant? Commented Nov 14 at 15:17
4

A string is equal to itself

A developer could later make a trivial change to the message, such as “last name must not be null” and this would break the unit tests, even though the message's meaning is still correct.

Exactly. The goal of unit tests is to test the actual logic. Testing that a string is equal to itself is useless. I would even argue that as soon as you repeat the message in your test, you introduce code duplication, that you should solve by moving the string to a constant (and reuse the constant in the test). As soon as you do this, it would become obvious that the test has zero value, as it would simply assert that the constant is equal to itself.

Testing actual messages becomes valuable when the messages are not simple constants, but are built based on a logic. This logic, indeed, deserves tests, as any other logic. It's up to you to evaluate the benefits of such tests, versus the cost of adding a test.

Note that in some circumstances,¹ exception messages are also localized. That is, on a German PC, the exception will be in German, whereas on a Japanese machine, they'll show in Japanese. Usually, you won't test every message in every supported locale. However, it is not unusual to test the formatting, when placeholders are used, as a message that has placeholders {0} and {1} in one language may have them inverted, i.e. {1} and {0}, in another language, or may require some very specific logic in another culture (such as when pluralizing things).

Enrich your exceptions

if sayHello("Tom", null) returns an IllegalArgumentException with message “first name is null,” I would consider that a bug, and I would like a unit test to catch this.

Then test exactly that. Asserting contents of an exception message is a rather convoluted way to do the test.

The exception message should contain all it needs to construct the actual message later, if needed. The documentation seems to imply that IllegalArgumentException takes only one argument—the actual message. This makes it inconvenient to use. By comparison, in .NET, you have an ArgumentNullException. While it can take an optional custom message as an argument, the usual way to use it is to pass just the name of the actual argument:

if (firstName == null)
{
    throw new ArgumentNullException(nameof(firstName));
}

When the name is null, the message would be generated automatically, looking like this:

Value cannot be null. (Parameter 'firstName')

When testing, you'll assert that ex.ParamName is equal to a given string. How the parameter name is then translated into an actual exception message is the job of .NET, not yours.

Doing this has an additional benefit of saving memory. You don't need the full string to be stored here: as the logic is idempotent, the actual message can be built on demand, when and if the message is needed. It might happen that the exception will be swallowed anyway by a try/catch block.

If Java doesn't have an exception type that let you do that, you may create one.

Additional things to think about

When designing exception messages, take your time to do several other things:

  1. Make absolutely sure that the exception is clear and complete. Imagine yourself on Friday evening in front of a log file that tells you that (1) the application crashed, and (2) it's because “first name is null.” By the way, there is an ersatz of stack trace that you can't easily use, the application being built in release mode. Does such message help you in your debugging? Does it make your life easier?

  2. Exception messages are messages, and so they should use proper English, proper punctuation, start with a capital letter, etc.

  3. Exception messages are for developers, and developers only (and support persons). Not for the end users. This means that if your application has an actual interface, you should take great care to avoid the situation where the value ends up being null in the first place—and if you don't, you should present the user with a clear, readable message. It's at this level that you'll have quite a lot of tests, as making such messages is all but easy.

  4. It's fine to have a check that the argument is not null. However, one should note that other languages have better alternatives. Some have richer types, where you can tell that you don't want a null value to be passed. And if the code that calls the method tries to pass a null, it won't compile (as your Java code won't compile if you would call your method and try to pass a float and a boolean as arguments). Others have code contracts—contracts that are part of the public interface of the method and that can be statically checked.

¹ For instance, .NET localizes the messages of the exceptions thrown by .NET itself. IMHO, this is a mistake, as exceptions have no purpose being shown to the end users—Microsoft's own products are a good example of why, when they occasionally crash with absolutely unhelpful exception messages. Nevertheless, the practice exists.

Improve this answer
4
  • I agree with the part of refactoring the string into a constant. But not with the second part. For example in refactoring, a developer might copy paste the first exception twice, so it shows First name is null when the second name is null. A unit test checking the exception message would catch that. Commented Nov 14 at 8:23
  • @infinitezero the test developer can also copy the constant twice Commented Nov 14 at 10:00
  • +1 for discussion of localization concern. Java localizes exceptions too, so it applies. Commented Nov 14 at 10:02
  • @Caleth the test developer can mess up in many ways. However, that should be notice quickly when you run the test. The test is meant to catch things that happen way later down the line. Once in place, tests usually don't change Commented Nov 14 at 10:18
3

There is a hierarchy of value in testing.

  1. Do the cases you expect to work, work correctly
  2. Do the cases you expect to fail, fail without mucking stuff up
  10. When you throw an exception for a failed case, is the exception message correct.

Sure its best practice to check that things work as per the spec. But you are pretty far down the list of things you should do by the time you get to this.

Improve this answer
0

In your case, the dynamic and important part of the message is a parameter name:

Assertions.assertContains("last", exception.getMessage());

This does not require the extremes Arseni suggests and covers a point missed by Jimmy (only getLocalizedMessage() is shown to user)

Improve this answer
0

If it is necessary for a function to fail in some case, it is a good idea to test that. However, since the specific text should not affect the behavior of the caller of the function, it is not a good idea to test the text itself.

The issue is solved by not using exception text at all. Instead, use unique error codes for each type of error. Have an exception class that contains the code, and test for the specific code. The exception text could contain more specific information about the error and could be logged for debugging.

You can use error codes to convert each exception to the message that you show to the user. This way each error message in the user interface is well planned, its solution/workaround can be easily documented, and it can be easily translated to other languages. For exceptions that do not contain a code (from third party libraries), have the error handler create them a code based on the class.

Improve this answer
New contributor
VLL is a new contributor to this site. Take care in asking for clarification, commenting, and answering. Check out our Code of Conduct.
6
  • IllegalArgumentTwoException? IllegalLastNameException? Commented Nov 14 at 11:29
  • @Basilevs In this case the error should be about the invalid last name. It is a value that the customer inputs (or understands), while the invalid argument two is irrelevant (customer does not know anything about arguments or their order). Even if this is a programming error (the function is incorrectly passed a null value), the customer might be able to work around it by entering a different last name or not doing the action that requires the last name. Commented Nov 14 at 12:25
  • @Basilevs You can use custom exception classes and error codes together. Just set the code automatically in the constructor. Error codes can be easily documented, but the customer does not know the name of the class. Even if you report the name, they might not be able to read the it if they are not a native English speaker. Commented Nov 14 at 12:25
  • 1
    This will lead for an exception type for every argument of every boundary method. Surely that's a bit too much? Commented Nov 14 at 15:06
  • @Basilevs If you reuse error codes, it will not lead to many exception types. For example, some function might check multiple boundaries, but it does not make sense to have multiple error types for each boundary. Or maybe there are multiple functions that check that "last name" is not null; these checks could be considered the same error. Commented Nov 14 at 20:02

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.