BEbedobi·Jul 27, 2021
I'm irrationally passionate about testing. Shameless plug: (rendered version at https://gist.github.com/androidfred/501d276c7dc26a5db09e893b...)
# Test your tests
## Summary: test interfaces, not implementations
Tests that pass when something is actually broken, and fail when things actually do work, are worse than useless- they are positively harmful.
## The requirement
Let's say we've been tasked with returning `400` when `GET` `/users/<userId>` is called with a negative `userId`.
## The test
The requirement can be turned into a test that hits the endpoint with a negative `userId` and checks that a `400` is returned:
```java
@Test
public void getUser_InvalidUserId_400() {
expect().statusCode(400).when().get("/users/-1");
}
```
## Implementation
A ubiquitous style of implementation may look something like this: (ignor
Read the rest
e whether you like the style of implementation or not, it's just an example)
```java
public class UserResource {
@Inject
private UserService userService;
@GET
@Path("/users/{userId}")
public Response getUser(@PathParam("userId") final Long userId) {
try {
return Response.ok(userService.findById(userId)).build();
} catch (final UserException e) {
return Response.status(e.getErrorCode()).build();
}
}
}
```
```java
public UserService {
@Inject
private UserDao userDao;
public User findById(final Long userId) throws UserException {
if ((userId == null) || (userId <= 0)) {
throw new UserException("Invalid arguments", 400);
}
return userDao.findById(userId);
}
}
```
## Another test
Everyone knows a test hitting the endpoint is not enough- more tests are required. A ubiquitous style of additional test may look something like this:
```java
public class UserResourceTest {
@InjectMocks
private UserResource userResource = new UserResource();
@Mock
private UserService userService;
@Test
public void getUser_InvalidParams_400() throws UserException {
doThrow(new UserException(INVALID_ARGUMENTS, 400)).when(userService).findById(-1L);
assertThat(userResource.getUser(-1L).getStatus(), is(equalTo(400)));
}
}
```
## Test the tests
### Break something
Let's say the negative `userId` check is removed from the `UserService`. (by mistake or whatever)
The test that hits the endpoint will fail, because it will no longer get a `400` when `GET` `/users/<userId>` is called with a negative `userId`. This is exactly what we want out of a test!
The additional test however will pretend the negative `userId` check is still in the service and pass. *This is literally the exact opposite of what we want out of a test!*
### Refactor something
Or, instead, let's say the `UserResource` is refactored to use a `NewBetterUserService`. (the `NewBetterUserService` still throws an exception on negative `userId`)
The test that hits the endpoint will pass, because it will still get a `400` when `GET` `/users/<userId>` is called with a negative `userId`. This is exactly what we want out of a test!
The additional test however will fail because it expects the `UserResource` to call the (old) `UserService`. *This is literally the exact opposite of what we want out of a test!*
### But... The `UserServiceTest` would fail
Maybe there's a `UserServiceTest` that would fail if the negative `userId` check is removed from the `UserService`:
```java
public class UserServiceTest {
@Test
public void findById_InvalidParams_400() throws UserException {
expectedException.expect(UserException.class);
expectedException.expectMessage("Invalid arguments");
assertThat(expectedException.errorCode(), is(equalTo(400)));
userService.findById(-1L);
}
}
```
It's irrelevant. The `UserResourceTest` test is still a bad test, because it fails on refactoring. And even if there is a `UserServiceTest` that fails if the negative `userId` check is removed from the `UserService`, the `UserResourceTest` doesn't fail if there is no such `UserServiceTest`, or if there is but it's incorrectly implemented etc. It just pretends the negative `userId` check is in the `UserService` and passes.
If you were absolutely adamant about testing the implementation (which you shouldn't be, because implementation tests fail on refactoring, making them bad tests), the "correct" way of testing that would be:
```java
public class UserResourceTest {
private UserResource userResource = new UserResource(new UserService()); //not a mock
@Test
public void getUser_InvalidParams_400() throws UserException {
assertThat(userResource.getUser(-1L).getStatus(), is(equalTo(400)));
}
}
```
Because now, if the negative `userId` check is removed from the `UserService` the test will fail. But since it also fails on refactoring, it's still a bad test.
### But... Mocks are useful
Yes, mocks are useful, and they do have a place. Eg, instead of connecting to a real db, the DAO methods could be mocked, and REST calls to other services etc could be mocked too. They're external dependencies not part of the core of the system under test. The case made here isn't that mocks are never useful or always bad, it's that mock testing is often used excessively for internals of the system under test, which doesn't make sense.
Also, I'd still argue that, by default, there should still be a strong preference for, rather than mocking DAO calls to db and REST calls to other services, actually spinning up a real in memory db (eg [https://github.com/vorburger/MariaDB4j](https://github.com/v... lets you do this), and actually WireMocking calls to other services. Then, more of the system under test is being tested, with less effort.
Ie unlike with regular mock testing, it's verified that the app doesn't just call a DAO method but also that DAO method actually connects to the db, runs the expected query and returns the expected result based on the primed content in the db, and that the app actually hits the configured external service url with the expected request and handles the primed response based on the Wiremock stub etc etc.
While such tests take a bit longer to run, you write far fewer, higher quality, less brittle tests, because you're not having to write and maintain `nn` mock tests between every layer of your app internals to check every single call on the way through the stack. You can freely refactor literally all* the internals all you want without having to change the tests at all - they will keep passing as long as everything works and they will fail if something doesn't. (which, again, is exactly what you want)
## Terminology
The test that hits the endpoint is commonly referred to as a "integration test", and the other test is commonly referred to as a "unit test".
Kent Beck (the originator of test-driven development) defines a unit test as "a test that runs in isolation from other tests". This is very different from the definition of a unit test as "a test that tests a class/method in isolation from other classes/methods". *The test that hits the endpoint satisfies the original definition.*
But it doesn't really matter if you want to call a given test an "integration" test or a "unit" test. The point is the test fails when something breaks and passes when something is improved. If it does the opposite, it's not a good test.
## More on the topic
* http://googletesting.blogspot.com.au/2013/08/testing-on-toil...
* http://codebetter.com/iancooper/2011/10/06/avoid-testing-imp...
## Misc
There are other problems with the example, such as eg
* Primitive Obsession: using a Long to represent `userId` and using procedural inline checks in the service to check for negative `userId`. Instead, create a class `UserId` that encapsulates and enforces its own checks.
* Hidden Dependencies: `UserResource` has a hidden dependency on `UserService` and `UserService` has a hidden dependency on `UserDao`. Dependency Injection frameworks like Spring encourage such hidden dependencies, and the excessive use of mocks in unit tests.
* The service shouldn't know about HTTP error codes.
These are simplifications in order to keep the example short - the scope of this article is testing.