Concurrent execution of JUnit 5 tests leading to incorrect coverage computations and improper TIA

[pjmartos]

When using skippy for test impact analysis together with JUnit 5 tests, we're observing unpredictable coverage, wrong test classes being marked for execution, and test classes being skipped when they shouldn't, when either of the following happen:

• Test classes are running in concurrent parallel mode, and/or
• There are test methods scattered across two or more @Nested classes, or across a @Nested class and the top-level class itself, or both

I believe this is due to the way coverage computations are being leveraged to determine the mapping of production classes vs test classes that visit them: https://github.com/skippy-io/skippy/blob/main/skippy-core/src/main/java/io/skippy/core/SkippyTestApi.java#L115,L131

Lines 115 and 131 assume that test classes run fully isolated and that the coverage computations observed since the last reset (i.e. in scope of a beforeAll callback) must undoubtedly and unambiguously correspond to production code exercised by the test class that has just finished running and for which we're running an afterAll callback.

[fmck3516]

Thanks for the report.

It would be great if you could provide minimal examples for both bullet points (but I will try to re-produce the issue myself).

Your observation is spot on: Skippy currently assumes that tests within a JVM execute serially. I have to wrap my head around how to solve that.

[fmck3516]

I played around with this yesterday. My sample project:

junit-platform.properties:

junit.jupiter.execution.parallel.enabled=true
junit.jupiter.execution.parallel.mode.default=concurrent

Test case:

package com.example;

import io.skippy.junit5.PredictWithSkippy;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;

import java.util.concurrent.*;

import static org.junit.jupiter.api.Assertions.assertEquals;

class Foo {

    static String getFoo() {
        return "foo";
    }

}

class Bar {
    static String getBar() {
        return "bar";
    }
}

@PredictWithSkippy
public class ParallelTestsTest {

    static CountDownLatch entryLatch = new CountDownLatch(2);
    static CountDownLatch exitLatch = new CountDownLatch(2);

    static ExecutorService executorService = Executors.newSingleThreadExecutor();

    private Future<String> invokeMethod(String className, String methodName) {
        return executorService.submit(() -> {
            try {
                var clazz = Class.forName(className);
                var method = clazz.getDeclaredMethod(methodName);
                return (String) method.invoke(null);
            } catch (Exception e) {
                throw new RuntimeException(e);
            }
        });
    }

    @Nested
    class FooTest {

        @Test
        public void first() throws Exception {
            entryLatch.countDown();
            entryLatch.await();
            assertEquals("foo", invokeMethod("com.example.Foo", "getFoo").get());
            exitLatch.countDown();
            exitLatch.await();
        }

    }

    @Nested
    class BarTest {

        @Test
        void testSomething() throws Exception {
            entryLatch.countDown();
            entryLatch.await();
            assertEquals("bar", invokeMethod("com.example.Bar", "getBar").get());
            exitLatch.countDown();
            exitLatch.await();
        }

    }

}

I don't see a way to track coverage for overlapping tests like that based on byte code instrumentation. Even if Jacoco offered a way to track coverage on a per-thread basis, I could not map this back to the individual test cases:

The only things that come to my mind:

• cover that this is not supported in the documentation
• disable Skippy if junit.jupiter.execution.parallel.enabled is set to true

@pjmartos Thoughts?

[pjmartos]

@fmck3516 what about adding this to the documentation and suggesting that people disable concurrent test class execution if they want to use skippy?

I honestly think that's a fair trade-off; for cold starts, some tests that used to be run in parallel would start running sequentially, which is maybe not ideal, but on the other hand skippy would nevertheless reduce the amount of tests to execute going forward, so the impact of "sequentialization" should be somewhat minimised.

Thanks.

[fmck3516]

This is now covered in the documentation: https://www.skippy.io/docs/#unsupported-features-junit-5

Thanks for the report!