When I’m writing production ready code I find that I spend 90% of my time writing test cases and 10% of the time writing the actual production code. There is a great temptation to take shortcuts when writing tests but there is one in particular I try to avoid.
In this article I’m going to try and demonstrate the flaw when asserting that objects are equivalent via the assertEquals method. This is a risky method of asserting that can miss potential null pointer exceptions and the inevitable bug hunt to trace the cause.
I’m going to be demonstrating a few different assertion methods and a few ways of modelling our data that can be used to avoid using the assertEquals method. In particular, I’m going to show how tests behave around Lombok, Java records, and Java classes. To do this I’m going to be using JUnit Jupiter with AssertJ in place of Jupiter’s default assertions.
For the sake of this example, I’m going to be working with some simple data model classes. These are solely for demonstration purposes, so let’s put to one side that the mapping is a basic 1:1 relationship and thus confers little benefit to the system. Let’s start by looking at our mapper class doing the object conversion:
public class EmployeeMapper {
public Employee map(Person input) {
return new Employee(input.getFirstName(), input.getLastName());
}
}
In the code above we have an object of type Person that needs to be converted to an object of type Employee. To do this, we use an object called the EmployeeMapper which encapsulates how it should convert from one object to another.
The test looks like the following:
private final EmployeeMapper mapper = new EmployeeMapper();
@Test
void shouldConvert() {
var input = new Person("Kieran", "Rafferty");
Employee result = mapper.map(input);
var expected = new Employee("Kieran", "Rafferty");
// equivalent of org.junit.jupiter.api.Assertions.assertEquals(expected, result);
assertThat(result).isEqualTo(expected);
}
Let’s review the test:
Given a Person object;
When I convert it to an Employee using the EmployeeMapper.map method;
Then assert that the resulting Employee object should be equal to the Employee found in the expected variable.
As mentioned, I’m using AssertJ which, in my opinion, provides a slightly nicer syntax over JUnit’s default assertion library (and avoids the common mistake regarding the order of the ‘expected’ parameter and the ‘actual’ parameter).
Testing for equality in Java is taught early in a Java engineers career because by default, it is not what you expect. The default behaviour being that we check whether the memory reference of one object is equal to the memory reference of another object. If they correspond, the objects are considered equal. The assertion framework essentially delegates to this equality check. Effectively:
assertTrue(expected.equals(result));
This means that if we have not overridden the equals method in the Employee class then the test above will fail because there are two separate Employee objects each residing in a separate section of the JVM.
Now, this is easily fixable. Because the engineer writing this code can make the test pass by overriding the equals method on the Employee class. And fortunately most IDEs will help us with this. IntelliJ by default, will generate the following equals method for the Employee class:
@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}
if (o == null || getClass() != o.getClass()) {
return false;
}
Employee employee = (Employee) o;
if (firstName != null ? !firstName.equals(employee.firstName) : employee.firstName != null) {
return false;
}
return lastName != null ? lastName.equals(employee.lastName) : employee.lastName == null;
}
If the test were now run, it will pass.
I have a problem with this strategy though. I’m making changes to my production code in order to aid the writing of my test. The only purpose of the equals method I have defined in the production code is for testing. If I have no intention of putting this object in a set or asserting equality in the production code, then why am I adding code to do so. This feels wrong to me. It’s like making a private field public so I can assert on the internal state of the object from within a test. We’re reminded again and again to test behaviours. Not state.
In this case, the object is capturing the state of the system, so I’m prepared to put this personal gripe aside. However, I believe there is a more fundamental problem. Does our test capture the intention of the developer?
Whenever I see assertions like this I’m almost certain that the intention of the developer was to make sure that the EmployeeMapper was working. i.e. That the EmployeeMapper is correctly setting every field on the Employeeobject with the relevant information from the Person object.
Whether this test passes or fails depends entirely on the implementation of the Employee class. What is actually being asserted on here is that the equals method on the Employee works. And we’ve not even tested that particularly well. Let’s show how bad our original test is by adding a new field to both the Person and Employee objects.
public class Person {
private final String firstName;
private final String lastName;
private final LocalDate dateOfBirth;
// Constructor and getters
}
public class Employee {
private final String firstName;
private final String lastName;
private final String dateOfBirth;
// Constructor and getters
}
Having added the fields, and updated the constructor I shall update the test to resolve the compile errors I will have introduced:
@Test
void shouldConvert() {
var input = new Person("Kieran", "Rafferty", LocalDate.of(1970, 1, 30));
Employee result = mapper.map(input);
var expected = new Employee("Kieran", "Rafferty", "1970-01-30");
assertThat(result).isEqualTo(expected);
}
If I run this test, it will pass. I’ve made no changes to the EmployeeMapper and yet the test still passes. Given the EmployeeMapper is not yet setting the date on the Employee object I would expect this test to fail as our expected object has a date.
What I’ve failed to update is the equals method on the Employee object. Therefore, our test still only checks whether the firstName and lastName are the same on the result object and the expected object and ignores the date field.
Perhaps the reason for this was glaringly obvious. But regardless of whether this was obvious; since it is an error, I would expect a test to fail because of it. And since no test failed, our test should be considered poor.
A reminder that just because something is glaringly obvious to one developer does not mean they will be caught by another developer. Furthermore, It should be emphasised that this does not imply anything about the developer’s skill – anyone can miss things when their focus is elsewhere.
Now at this point, I expect the proponents of Lombok to step forward and pronounce Lombok the saviour to this dilemma. So let’s show how Lombok solves this problem. We’re going to add the following dependencies:
dependencies {
compileOnly 'org.projectlombok:lombok:1.18.20'
annotationProcessor 'org.projectlombok:lombok:1.18.20'
}
I’m not going to dig into Lombok much but whats important is that by declaring just our fields, we can get Lombok to automatically provide an equals method. Let’s return to our Employee class and replace our constructor and equals method with Lombok.
@Getter
@EqualsAndHashCode
@RequiredArgsConstructor
public class Employee {
private final String firstName;
private final String lastName;
private final String dateOfBirth;
}
Lombok is now taking care of adding the getters, the constructor and the equals method automatically. Because of this, our maintenance overhead has been drastically reduced. We no longer have to remember to update the equals method because Lombok will take care of it for us.
Running the test proves this point. The test above will now fail as expected because Lombok’s auto-generated equals method will always check against every field with no additional changes.
As an alternative, I can also use Java records instead of Lombok to get this test to pass:
public record Employee(String firstName, String lastName, String dateOfBirth) {
}
Using Java records also gives us a more useful version of the equals method by default. With our Employee now a record instead of class I will also, correctly, see a test failure until I update the EmployeeMapper. (As an aside, I personally don’t really want to switch all my POJOs over to records. I think the record is a nice feature to have for lambdas where I want my functions to have multiple return types without using a tuple or declaring a class explicitly. Maybe I’ll see what comes from records as they become more widely used).
At this point, I’d like to emphasise that neither solution addresses the underlying problem here. The test is still poor. Let me demonstrate this with the following statements about what a test should be:
- A test should pass if the code it is testing works regardless of implementation.
- Whether code ‘works’ should be explicitly asserted on within the test.
Statement two is more of a clarification on the word ‘works’ in the first statement. As an example, if something produces the result we want but executes in minutes and the requirement is milliseconds, then that should be asserted on within the test. Without that assertion, the underlying production code ‘works’.
Statement one is the important point for this discussion and insists that my test should give me confidence of whether my production code works regardless of how the production code is implemented. Seeing as the use of a Java record or use of Lombok are implementation details… our test should give us confidence of whether the code works or not without the knowledge that these features auto-generate the equals method. Times change. Production code shifts hands. Lombok may be superseded by the next Java release or another library. If the next team chooses to migrate from Lombok back to raw Java then the test should still offer the same value. As it is, the test does not – since it allows certain implementations of the Employee class to fall into the original trap.
There are solutions to this problem. My preferred solution is sure to frustrate many a developer because it means more lines of code (and therefore more work).
Here is my preferred way:
@Test
void shouldConvert() {
var input = new Person("Kieran", "Rafferty", LocalDate.of(1970, 1, 30));
Employee result = mapper.map(input);
assertThat(result.firstName()).isEqualTo("Kieran");
assertThat(result.lastName()).isEqualTo("Rafferty");
assertThat(result.dateOfBirth()).isEqualTo("1970-01-30");
}
Explicitly checking each field will guarantee that each field is set correctly. If something is wrong then our stacktrace will include the particular line (and therefore field) which has been set incorrectly.
The other option is to take advantage of the extra assertions in AssertJ. Here is an alternative that AssertJ provides:
@Test
void shouldConvert() {
var input = new Person("Kieran", "Rafferty", LocalDate.of(1970, 1, 30));
Employee result = mapper.map(input);
var expected = new Employee("Kieran", "Rafferty", "1970-01-29");
assertThat(result).isEqualToComparingFieldByField(expected);
}
This can probably be thought of as an enhanced version of the isEqualTo assertion. AssertJ uses reflection to perform an equals check on each field in the object. The output of the above when there is an error is still readable. For example, the above test will output the following error:
Expecting value <"1970-01-29"> in field <"dateOfBirth"> but was <"1970-01-30"> in <Employee[firstName=Kieran, lastName=Rafferty, dateOfBirth=1970-01-30]>. Comparison was performed on all fields
Also to note that if the Employee class has a field that is a complex type then the assertion for that field falls back to equality (i.e. will check the memory reference). You would need to do the following to make sure the assertion library checks the fields of any nested objects:
assertThat(result).isEqualToComparingFieldByField(expected).usingRecursiveComparison();
So why do I prefer one over the other?
I find the former is better suited for Test Driven Development (TDD). In TDD I make changes to my test class first, which I should then run and observe a failure, before I make changes to the production code to make the test pass.
In the former, I add a new assertion specifically for my new field. In the latter, because I’m using the isEqualToComparingFieldByField assertion method, I need to add the new field to the expected Employee. This will work, but never feels right to me. Is this worth the verbosity – particularly with classes with many fields? I’ll let you decide. Just don’t assert that an object isEqualTo another unless you’re checking the equals method is working.
