Big Test Set-Ups Don’t Necessarily Point to Design Problems

I was discussing what our test code can tell us about the design of our solutions this morning with a friend. It’s an interesting topic. The received wisdom is that big test set-ups mean that the class or module being tested has too many dependencies and is therefore almost certainly doing too much.

This is often the case, but not always. Let me illustrate with an example. Here’s an integration test for my Guitar Shack solution:

	package com.guitarshack.integrationtests;
	import com.guitarshack.*;
	import com.guitarshack.net.RESTClient;
	import com.guitarshack.net.RequestBuilder;
	import com.guitarshack.net.Web;
	import com.guitarshack.product.ProductData;
	import com.guitarshack.sales.SalesData;
	import com.guitarshack.sales.ThirtyDayAverageSalesRate;
	import org.junit.Test;
	import java.util.Calendar;
	import static org.mockito.Matchers.any;
	import static org.mockito.Mockito.mock;
	import static org.mockito.Mockito.verify;
	/*
	It's a good idea to have at least one test that wires together most or all
	of the implementations of our interfaces to check that we haven't missed anything
	*/
	public class StockMonitorIntegrationTest {
	@Test
	public void alertShouldBeTriggered(){
	Alert alert = mock(Alert.class);
	StockMonitor monitor = new StockMonitor(
	alert,
	new ProductData(
	new RESTClient(
	new Web(),
	new RequestBuilder())),
	new LeadTimeReorderLevel(
	new ThirtyDayAverageSalesRate(
	new SalesData(
	new RESTClient(
	new Web(),
	new RequestBuilder()
	),
	() –> {
	Calendar calendar = Calendar.getInstance();
	calendar.set(2019, Calendar.AUGUST, 1);
	return calendar.getTime();
	}
	)
	)
	)
	);
	monitor.productSold(811, 40);
	verify(alert).send(any());
	}
	}

view raw GuitarShackIntegrationTest.java hosted with ❤ by GitHub

The set-up for this test is pretty big. Does that mean my StockMonitor class has too many dependencies? Let’s take a look.

	public class StockMonitor {
	private final Alert alert;
	private final Warehouse warehouse;
	private final ReorderLevel reorderLevel;
	public StockMonitor(Alert alert, Warehouse warehouse, ReorderLevel reorderLevel) {
	this.alert = alert;
	this.warehouse = warehouse;
	this.reorderLevel = reorderLevel;
	}
	public void productSold(int productId, int quantity) {
	Product product = warehouse.fetchProduct(productId);
	if(needsReordering(product, quantity))
	alert.send(product);
	}
	private Boolean needsReordering(Product product, int quantitySold) {
	return product.getStock() – quantitySold <= reorderLevel.calculate(product);
	}
	}

view raw StockMonitor.java hosted with ❤ by GitHub

That actually looks fine to me. StockMonitor essentially does one job, and collaborates with three other classes in my solution. The rest of the design is hidden behind those interfaces.

In fact, the design is like that all the way through. Each class only does on job. Each class hides its internal workings behind small, client-specific interfaces. Each dependency is swappable by dependency injection. This code is highly modular.

When we look at the unit test for StockMonitor, we see a much smaller set-up.

	public class StockMonitorTest {
	@Test
	public void alertSentWhenProductNeedsReordering() {
	Alert alert = mock(Alert.class);
	ReorderLevel reorderLevel = product1 –> 10;
	Product product = new Product(811, 11, 14);
	Warehouse warehouse = productId –> product;
	StockMonitor monitor = new StockMonitor(alert, warehouse, reorderLevel);
	monitor.productSold(811, 1);
	verify(alert).send(product);
	}
	}

view raw StockMonitorTest.java hosted with ❤ by GitHub

The nesting in the set-up for the integration test is a bit of clue here.

	StockMonitor monitor = new StockMonitor(
	alert,
	new ProductData(
	new RESTClient(
	new Web(),
	new RequestBuilder())),
	new LeadTimeReorderLevel(
	new ThirtyDayAverageSalesRate(
	new SalesData(
	new RESTClient(
	new Web(),
	new RequestBuilder()
	),
	() –> {
	Calendar calendar = Calendar.getInstance();
	calendar.set(2019, Calendar.AUGUST, 1);
	return calendar.getTime();
	}
	)
	)
	)
	);

view raw GuitarShackIntegrationTest.java hosted with ❤ by GitHub

This style of object construction is what I call “Russian dolls”. The objects at the bottom of the call stack are injected into the objects one level up, which are injected into objects another level up, and so on. Each object only sees its direct collaborators, and the lower layers are hidden behind their interfaces.

This is a natural consequence of the way I test-drove my solution: from the outside in, solving one problem at a time and using stubs and mocks as placeholders for sub-solutions.

So the big set-up in my integration test is not a sign of a class that’s doing too much and a lack of separation of concerns, and that’s because it’s a “Russian dolls” set-up. if it was a “flat set-up”, where every object is passed in as a direct parameter of StockMonitor‘s constructor, then that would surely be a sign of StockMonitor doing too much.

So, big set-up != lack of modularity in certain cases. What about the other way around? Does a small set-up always mean no problems in the solution design?

Before Christmas I refuctored my Guitar Shack solution to create some practice “legacy code” for students to stretch their refactoring skills on.

	public class StockMonitor {
	private final Alert alert;
	public StockMonitor(Alert alert) {
	this.alert = alert;
	}
	public void productSold(int productId, int quantity) {
	String baseURL = "https://6hr1390c1j.execute-api.us-east-2.amazonaws.com/default/product";
	Map<String, Object> params = new HashMap<>() {{
	put("id", productId);
	}};
	String paramString = "?";
	for (String key : params.keySet()) {
	paramString += key + "=" + params.get(key).toString() + "&";
	}
	HttpRequest request = HttpRequest
	.newBuilder(URI.create(baseURL + paramString))
	.build();
	String result = "";
	HttpClient httpClient = HttpClient.newHttpClient();
	HttpResponse<String> response = null;
	try {
	response = httpClient.send(request, HttpResponse.BodyHandlers.ofString());
	result = response.body();
	} catch (IOException | InterruptedException e) {
	e.printStackTrace();
	}
	Product product = new Gson().fromJson(result, Product.class);
	Calendar calendar = Calendar.getInstance();
	calendar.setTime(Calendar.getInstance().getTime());
	Date endDate = calendar.getTime();
	calendar.add(Calendar.DATE, –30);
	Date startDate = calendar.getTime();
	DateFormat format = new SimpleDateFormat("M/d/yyyy");
	Map<String, Object> params1 = new HashMap<>(){{
	put("productId", product.getId());
	put("startDate", format.format(startDate));
	put("endDate", format.format(endDate));
	put("action", "total");
	}};
	String paramString1 = "?";
	for (String key : params1.keySet()) {
	paramString1 += key + "=" + params1.get(key).toString() + "&";
	}
	HttpRequest request1 = HttpRequest
	.newBuilder(URI.create("https://gjtvhjg8e9.execute-api.us-east-2.amazonaws.com/default/sales" + paramString1))
	.build();
	String result1 = "";
	HttpClient httpClient1 = HttpClient.newHttpClient();
	HttpResponse<String> response1 = null;
	try {
	response1 = httpClient1.send(request1, HttpResponse.BodyHandlers.ofString());
	result1 = response1.body();
	} catch (IOException | InterruptedException e) {
	e.printStackTrace();
	}
	SalesTotal total = new Gson().fromJson(result1, SalesTotal.class);
	if(product.getStock() – quantity <= (int) ((double) (total.getTotal() / 30) * product.getLeadTime()))
	alert.send(product);
	}
	}

view raw StockMonitor.java hosted with ❤ by GitHub

Yikes!

I think it’s beyond any reasonable doubt that this class does too much. There’s almost no separation of concerns in this design.

Now, I didn’t write any unit tests for this (because “legacy code”), but I do have a command line program I can use the StockMonitor with for manual or shell script testing. Take a look at the set-up.

	public class Program {
	private static StockMonitor monitor = new StockMonitor(product –> {
	// We are faking this for now
	System.out.println(
	"You need to reorder product " + product.getId() +
	". Only " + product.getStock() + " remaining in stock");
	});
	public static void main(String[] args) {
	int productId = Integer.parseInt(args[0]);
	int quantity = Integer.parseInt(args[1]);
	monitor.productSold(productId, quantity);
	}
	}

view raw Program.java hosted with ❤ by GitHub

It’s pretty small. And that’s because StockMonitor‘s dependencies are nearly all hard-wired inside it. Ironically, lack of separation of concerns in this case means a simple interface and a tiny set-up.

So, big set-ups don’t always point to a lack of modularity, and small set-ups don’t always mean that that we have modularity in our design.

Of course, what the big set-up in our integration test does mean is that this test could fail for many reasons, in many layers of our call stack. So if all our tests have big set-ups, that in itself could spell trouble.

Explore the Guitar Shack source code