CodeSOD: Collect Your Garbage Test

Alex L works for a company with a bunch of data warehousing tools. One of the primary drivers for new features is adding bits which make the sales demos shinier, or in marketing speak, "delivers a value add for a prospective customer".

One of those "shinies" was a lightweight server which would gather stats from the data warehousing engine and provide a dashboard with those stats, and notifications about them. Prospective customers really love it when the machine goes "bing" and shows them an interesting notification.

As the notifications were becoming a bigger feature, the system needed a way to remember notification configuration between sessions, and that meant there needed to be a better persistence layer.

To add the feature, Alex started by looking at the existing tests for the notification system.

public class NotificationFeatureTest extends TestBase {@BeforeClasspublic static void init(){start(new File(TestUtil.TEST_DIR, "notification"));}@Testpublic void testSetNotificationLevel() {NotifyClient client = new NotifyClient(connectionUrl);client.setNotifyAbove(0); //notify at 0% usedclient.publish(TestUtils.randomRow());Assert.assertTrue(client.isNotified());}/* and about 20 more simple such tests */}

Pretty reasonable. The init method calls the base class's start method, which spins up a notification server and a notification client.

Using that as a model, Alex whipped up a test which would shut down the server, then restart it, and confirm that the data is still there.

@Testpublic void testPersistNotificationLevel() {NotifyClient client = new NotifyClient(connectionUrl);client.setNotifyAbove(32); //notify at 32% usedtestInst.shutdown();testInst.start();client = new NotifyClient(connectionUrl);Assert.assertEquals(32, client.getNotifyAbove());}

testInst is the service instance created during the start. Unfortunately, when Alex ran this test, it exploded on testInst.start(), complaining: "Configuration not specified". But wasn't the point of the base-class start method to do that configuration step?

public abstract class TestBase {static int port = 8080;static String connectionUrl = null;static DashboardServer testInst =null;public static void start(File config) {testInst = new DashboardServer(port);testInst.setConfigurationFile(config.getAbsolutePath());testInst.start();connectionUrl = testInst.getUrl();testInst = new DashboardServer(port);}/* stripped unnecessary variables and methods */}

Well, sort of. The start method creates a testInst, configures it, and then starts it. So far so good. It then stuffs the URL of that instance into connectionUrl, which is also the url the NotifyClient uses to connect to the server in all of the tests.

And then they replace the started testInst with an entirely new, and unconfigured, testInst.

Alex went back through the history here, and found that this line had always been part of the TestBase class. The fact that any of the 400 tests in their test suite actually worked was basically an accident. It would start a server, release the reference to the server, and since the unit tests didn't live long enough for garbage collection to hit them, the server kept running even though there were no live references to it. Since NotifyClient was initialized with the connectionUrl from that living instance, everything worked.

Alex's test was the first one to directly touch testInst as a test step. It was easy for Alex to fix that line and get the test to pass. It was much harder to understand how and why that line got there in the first place, or why no one had ever noticed it before.

[Advertisement] ProGet supports your applications, Docker containers, and third-party packages, allowing you to enforce quality standards across all components. Download and see how!