Thomas Klambauer About the Author

Thomas Klambauer has been active in the Software Engineering field since 2002, with a strong background in Java and C++. At Compuware, he is part of the server platform team for an application performance management product, where he focuses on multi-threading and concurrency topics.

How Stable are Your Unit Tests? Best Practices to Raise Test Automation Quality

Most of our 10k+ unit tests are written in Java using JUnit and run with the gradle build automation tool. The more tests we added over time, the more often we ran into the problem that unit test executions became unstable. New tests impacted the execution of existing tests. Our “failed test” metric for tests that ran fine for months started to increase. It was tempting to blame bad application code, but through careful analysis we found the real reason for these unstable test results. Many of these problems were caused by new tests that adversely impacted the test environment, and therefore also the execution of other tests.

In this blog post I will show you how we identified the root cause of these specific test failures as well as our derived best practices for unit test design with respect to its interaction with the test environment.

It all Began with Rare Unit Test Timeouts

Last week we identified a group of tests that failed for what seemed to be no specific reason. I volunteered to investigate.

A group of unit tests verifies important required behavior of a ThreadPool implementation. In particular, scheduled periodic tasks must continue to execute even after certain exceptions are thrown. To test this, a task is scheduled which throws the target exception. The test then waits for a second execution after the first one was aborted due to the exception.

On some machines and test runs, these tests would time out waiting for re-execution. No log output would be written, although various exception handlers were in place to log any exception. Only the message “Exception: java.lang.NullPointerException thrown from the UncaughtExceptionHandler in thread “pool-unit-test-thread-1″ was printed. However, this was never reproducible when executing the tests in Eclipse, only when running the test suite in gradle.

Next steps: I instructed gradle to open a debug port and then connected Eclipse to determine the cause. This revealed that the NullPointerException was generated somewhere in gradle code. I downloaded the source code and discovered that System.getProperty(“line.separator”)returned null and was dereferenced.

With this information, I searched and quickly found another test that verifies the string formatting on different platforms that had the side effect of changing the line.separator property. By calling System.clearProperty(“line.separator”)after the test, it inadvertently set that property to null.

In this context, when the next test was run, log information would be written to the console and gradle code would be invoked because it uses System.setOut to redirect console output. This in turn caused the NullPointerException as gradle accessed the line.separator property value. Note that the execution order is important, and by chance it was sometimes during the ThreadPool test.

Usually a set of handlers would catch that, but as they too wrote to the console, they encountered a NullPointerException and thus propagated the failure.

The quick fix for the failure is to remove the clearProperty call and instead use the previous value of the “line.separator” property. But can we do better?

Side Effects in Unit Tests

Part of the above failure is that for test automation to perform within expectations, JVM instances are usually reused, at least within the same project. Therefore ideally unit tests should have no side effects on their test environment, to prevent such failures.

The test environment includes various resources that can influence tests executed afterwards, such as creating files on the file system or as in the case above, altering Java system properties. For files, JUnit offers the TemporaryFolder rule to create temporary files, so we built a similar mechanism for properties.

Solution: Proper Use of System Properties in Unit Tests

In general, suppose we want to develop a test which requires a Java System property to be set to a specific value, writing something like this:

Test.java

@Before
public void setup() {
  System.setProperty("prop", "true");
}
@Test
// use that prop..

But, when other tests are run in the same JVM after this test, the property is also set and may influence the behavior of the following tests! So let’s improve that to:

Test.java

private String oldValue;
@Before
public void setup() {
  // setProperty returns the old value of that property.
  oldValue = System.setProperty("prop", "true");
}

@After
public void teardown() {
  System.setProperty("prop", oldValue);
}
@Test
// use that prop..

Looks fine, doesn’t it? But there still is a problem: If the property wasn’t set, setProperty in setup returns null and teardown throws a NullPointerException. So the correct version for a unit test to leave its system property environment unchanged would be:

Test.java

private String oldValue;
@Before
public void setup() {
  // setProperty returns the old value of that property.
  oldValue = System.setProperty("prop", "true");
}

@After
public void teardown() {
  if( oldValue == null ) {
    System.clearProperty("prop");
  } else {
    System.setProperty("prop", oldValue);
  }
}
@Test
// Use that property..

This is a bit verbose.. A helper based on the Java 7 try-with-resources statement accomplishes the same thing:

Test.java

private ScopedProperty property;
@Before
public void setup() {
  property = new ScopedProperty("prop", "true");
}
@After
public void teardown() {
  // Does same things like above.
  property.close();
}
@Test
// use that prop..

or also:

Test.java

@Test
public void test() {
  try(ScopedProperty prop = new ScopedProperty("prop", "true")) {
    // use that prop..
  }
}

In the first case it’s even better to implement a JUnit rule, where you can’t forget to write the @After close. For instance:

Test.java

@ClassRule
public static ScopedPropertyRule prop = new ScopedPropertyRule("prop", "true");
@Test
public void test1() {
 // use that prop..
}
@Test
public void test2() {
// use that prop..}

Where the property is set in the @BeforeClass and the original state restored in @AfterClass.

Conclusion: Side-Effect Free Tests increases test automation quality

To avoid test failures based on the order of execution unit tests should be side effect-free. The real world example demonstrated the relevance of this topic. To further this practice, we developed helper classes for the case of system properties. The same principle is also applicable to other environment objects, such as Thread priorities, the Java security manager and classes similar to ScopedProperty and ScopedPropertyRule, and are easily implemented. As we always strive to improve our test automation quality I would be interested in other best practices. Leave us a comment on what you are doing in our development teams. (Thanks to Reinhold Füreder for the input on JUnit rules!)

Appendix

 

ScopedProperty.java

 

/**
  * A helper to switch a system property value and restore the previous one.
  * 
  * When used in try-with-resources, restores the values automatically.
  */
public class ScopedProperty implements AutoCloseable {

    private final String key;
    private final String oldValue;

    /**
     * 
     * @param key The System.setProperty key
     * @param value The System.setProperty value to switch to.
     */
    public ScopedProperty(final String key, final String value) {
        this.key = key;
        oldValue = System.setProperty(key, value);
    }

    @Override
    public void close() {
        // Can't use setProperty(key, null) -> Throws NullPointerException.
        if( oldValue == null ) {
            // Previously there was no entry.
            System.clearProperty(key);
        } else {
            System.setProperty(key, oldValue);
        }
    }
}

Here, ScopedProperty implements AutoCloseable to function in the try-with-resources statement.

ScopedPropertyRule.java

/**
 * A JUnit test rule, which changes a property value within a test
 * and restores the original one afterwards.
 * See {@link ScopedProperty}.
 */
public class ScopedPropertyRule extends ExternalResource {

    private final String key;
    private final String value;
    private ScopedProperty scopedProperty;

    public ScopedPropertyRule(final String key, final String value) {
        this.key =key;
        this.value = value;
    }

    @Override
    protected void before() throws Throwable {
        scopedProperty = new ScopedProperty(key, value);
    }

    @Override
    protected void after() {
        scopedProperty.close();
    }
}

This class uses ExternalResource to implement a JUnit test rule.

Comments

  1. Richard Uttenthaler says:

    Hi Thomas,

    we in the acceptance team also need a well known state before every test execution. Our test are executed via a special JUnit suite runner (extension of class org.junit.runners.Suite one that comes with SilkTest). We do some cleanup in killing certain processes after a test failure occurred and of course when the testsuite is finished.

    public class SuiteRunner extends SilkTestSuite { // SilktestSuite extends org.junit.runners.Suite

    @Override
    public void run(RunNotifier notifier) {
    notifier.addListener(new RunListener() {

    @Override
    public void testFailure(Failure failure) throws Exception {
    // terminate some processes
    }

    @Override
    public void testRunFinished(Result result){
    // terminate some processes
    }
    });
    super.run(notifier);
    }

    • It is interesting to see this testing facet also regarding other processes. In some tests we start other Java runtimes (for instance to test termination), or server/client setups are tested and then it really becomes an issue.

      I also had tests failing because of other processes (still) running in the background (orphaned gradle build/test processes), so I would definitely add that consideration into test design and cleanup, a good point!

  2. ultimape says:

    Interesting.

    Can we go deeper and write a unit-test-test that verifiies that there are no sideffects on the JVM after a test is ran? Perhaps a quick answer would be to use some type of lint like checker to verify that side-effects aren’t accidentally overlooked. Stuff like this really needs to have some type of compiler support or meta-test to catch them all.

    • Verification could be a second, additional approach to the problem, if you really have unit tests (small). But when the components under tests become bigger, I think static verification would meet its limits. We have some static checking tools in use: FindBugs for null pointer checking, and these are very limited in their use: just a call into a library and the tool can’t be sure what to get back unless there are annotations or additional information available. So you can only get some hits if you are lucky and I think you would have the same problem with such unit-test side-effect analysis. You would need a lot of declarations like @SideEffectFree, etc.

      Verification at runtime (for instance instrumenting the File API) might help more, but it also would be a lot of work.

      I see the best chances in the provider of the environment: rolling back changes (writes to files, creation of threads, etc..) and that being done by JUnit between tests. But that is no easy task to implement either.

  3. You can add a org.junit.runner.notification.RunListener to the test run. This can be done in Maven, and probably Gradle too.
    In this verify that the state of the environment before/after is Ok, and fail the test if not.

    We do a lot of testing that involves changing time (using jodatime) or timezones. Sometime tests also start executors/threadpools. By asserting that the environment is Ok after a test has run in a standard way we can quickly find the tests that haven’t done the cleanup. This comes on top of having standard junit @Rules, in case somebody bypasses them for whatever reason.

    • Thanks for the tip with RunListener, that makes sense. I was also having a look at org.junit.rules .Verifier that might be useful to some folks.

      I have to say that I was already thinking of implementing a similar rule for leftover threads, because I found exactly the same situation, where someone forgets to clean-up the thread pools (call shutdown after tests), which might impact other tests, or could at least worsen performance.

      Regarding time-dependent components, we try to base those on an artificial TimelineProvider that is typically passed to them during construction, and components then use it for instance instead of System.currentTimeMillis(). This allows us during tests to manipulate / fake a timestamp and thereby simulate passing time / changing time.

      It would be useful to have a common JUnit rule library or test helpers to that end, seeing that similar issues seem to arise for a lot of JUnit users.

  4. Gareth Hurley says:

    Hi Thomas,

    I encountered exactly the same problem when executing a large volume of legacy JUnit tests through Maven in continuous integration. In my case restoring the System properties to the original state after test execution (as in your examples) was not enough to eliminate the ‘randomness’.

    The reason for this is that our maven-surefire-plugin was configured to run JUnit tests in multiple threads (for performance reasons) but within the same JVM (to reduce memory overheads).

    See;
    http://maven.apache.org/surefire/maven-surefire-plugin/examples/fork-options-and-parallel-execution.html

    As a result two test threads could read/write from/to the same System property or static variable around the same time, leading to a race condition and non-deterministic test results. This is true even if each test correctly ran its teardown() after test execution to restore resources to their initial state.

    An obvious workaround is to change the forking policy so that either JVMs are not reused across tests and/or to avoid parallel test execution within a single JVM.

    However tests that execute differently depending on valid Maven settings and/or the underlying hardware do not, in my opinion, qualify as true, independent ‘unit’ tests.

    I think that this problem would occur even with your helpful modifications? What do you suggest as an alternative?

    One solution I used was to pass the Properties into the constructor of the class under test and use a static creator method to create instances of the Object. That way I could pass mocked Properties into the object during my test and avoid System.getProperties/System.setProperty entirely.

    Example:

    package mypackage;

    public class MyClass {

    private static MyClass instance; //typical Singleton usage

    private Object property1, property2; //typed and better named in real code

    /*
    * Explicitly pass the values read from the properties into the constructor.
    * These values are ‘read-only’ JVM properties so no need to hold the Properties after object creation.
    * Alternatively the Properties could be held as an instance variable.
    */
    private MyClass(Object property1, Object property2){
    this.property1 = property1;
    this.property2 = property2;
    }

    //The method called by the actual production code…
    public synchronized static getInstance()(
    if(instance==null){
    return newMyClass(System.getProperties());
    }
    return instance;
    }

    /*
    * Package-friendly/default access so that the JUnit test can directly call this…
    */
    static newMyClass(Properties properties){
    Object property1 = System.getProperty(somePropertyKey1); //somePropertyKey1 & 2 defined elsewhere…
    Object property2 = System.getProperty(somePropertyKey2);
    return new MyClass(property1, property2);
    }
    }

    ———-

    package mypackage;

    public class MyClassTest {

    private Properties dummyProperties;

    @Before
    private void createProperties(){
    dummyProperties = new Properties(); //or use Mockito etc. if preferred
    .. //Build the properties as required
    ..
    }

    @Test
    public void someTestUsingMyClass {
    MyClass testInstance = MyClass.newMyClass(dummyProperties);
    ..
    ..
    }

    //No @After needed since there is no link to shared resources.

    }

    Unfortunately my unit tests do not directly call MyClass.getInstance() as used in the production code. On the other hand I can test the logic of MyClass without worrying about System properties.

    • Hello Gareth,

      I like that solution a lot, introducing another layer of indirection, such that the code under test does not directly rely on the System properties interface. One could pass a customer “ConfigurationForX” class and then mock that during tests, while using system properties in production. I found it often helpful to have such an indirection layer in place, also for other test issues, like System.currentTimeMillis().

      Of course the solution proposed in the article would not work when tests are executed in parallel on the same JVM. For us it was okay, because we execute tests only sequentially within the same JVM and achieving parallelism by using multiple JVMs for tests.

      I think we can deduce that is important to define how unit tests are run, if one test must assume that other tests run in parallel in the same JVM you are more restricted than assuming you run alone. Imagine also other cases, where you might run checks whether all created threads have been cleanly shutdown after a test: This would never works in such a situation.

      But you might however get better performance using same-JVM test parallelism.

  5. Gareth Hurley says:

    Small typo above…

    //The method called by the actual production code… The static creator method should be:

    public synchronized static MyClass getInstance()(
    if(instance==null){
    return newMyClass(System.getProperties());
    }
    return instance;
    }

Comments

*


5 − = two