[TestNG] Fix concurrent modification of events (#1919)

When a TestNG suite with `parallel="methods"` is executed TestNG will invoke
all `@DataProvider` annotated methods. Because these data providers also emit
the `TestRunStarted` event this event is emitted twice concurrently. These
events collide in the `CanonicalOrderEventPublisher` resulting in a concurrent
modification that inserts a null element rather then either of the events.

By moving the start of the test execution to the constructor we ensure that
test execution is always started serially. Additionally this ensures that it is
impossible to emit a `TestRunFinished` event without also emitting a
`TestRunStarted` event.

This comes at the cost of starting the execution as soon as the `@BeforeClass`
method is invoked. This will result in some apparent overhead as `TestNG` is
still preparing for execution.

Fixes: #1917

Author: mpkorstanje

testng/src/main/java/io/cucumber/testng/TestNGCucumberRunner.java

@@ -60,9 +60,7 @@ public final class TestNGCucumberRunner {
     private final Predicate<Pickle> filters;
     private final ThreadLocalRunnerSupplier runnerSupplier;
     private final RuntimeOptions runtimeOptions;
-    private final Plugins plugins;
-    private final FeaturePathFeatureSupplier featureSupplier;
-    private List<Feature> features = null;
+    private final List<Feature> features;
 
     /**
      * Bootstrap the cucumber runtime
@@ -101,38 +99,49 @@ public TestNGCucumberRunner(Class<?> clazz) {
 
         Supplier<ClassLoader> classLoader = ClassLoaders::getDefaultClassLoader;
         FeatureParser parser = new FeatureParser(bus::generateId);
-        this.featureSupplier = new FeaturePathFeatureSupplier(classLoader, runtimeOptions, parser);
+        FeaturePathFeatureSupplier featureSupplier = new FeaturePathFeatureSupplier(classLoader, runtimeOptions, parser);
 
-        this.plugins = new Plugins(new PluginFactory(), runtimeOptions);
+        Plugins plugins = new Plugins(new PluginFactory(), runtimeOptions);
         ObjectFactoryServiceLoader objectFactoryServiceLoader = new ObjectFactoryServiceLoader(runtimeOptions);
         ObjectFactorySupplier objectFactorySupplier = new ThreadLocalObjectFactorySupplier(objectFactoryServiceLoader);
         BackendServiceLoader backendSupplier = new BackendServiceLoader(clazz::getClassLoader, objectFactorySupplier);
         this.filters = new Filters(runtimeOptions);
         TypeRegistryConfigurerSupplier typeRegistryConfigurerSupplier = new ScanningTypeRegistryConfigurerSupplier(classLoader, runtimeOptions);
         this.runnerSupplier = new ThreadLocalRunnerSupplier(runtimeOptions, bus, backendSupplier, objectFactorySupplier, typeRegistryConfigurerSupplier);
+
+        // Start test execution now.
+        plugins.setSerialEventBusOnEventListenerPlugins(bus);
+        features = featureSupplier.get();
+        bus.send(new TestRunStarted(bus.getInstant()));
+        features.forEach(feature -> bus.send(new TestSourceRead(bus.getInstant(), feature.getUri(), feature.getSource())));
     }
 
     public void runScenario(io.cucumber.testng.Pickle pickle) throws Throwable {
         //Possibly invoked in a multi-threaded context
         Runner runner = runnerSupplier.get();
-        try(TestCaseResultObserver observer = TestCaseResultObserver.observe(runner.getBus(), runtimeOptions.isStrict())){
+        try (TestCaseResultObserver observer = TestCaseResultObserver.observe(runner.getBus(), runtimeOptions.isStrict())) {
             Pickle cucumberPickle = pickle.getPickle();
             runner.runPickle(cucumberPickle);
             observer.assertTestCasePassed();
         }
     }
 
+    /**
+     * Finishes test execution by Cucumber.
+     */
     public void finish() {
         bus.send(new TestRunFinished(bus.getInstant()));
     }
 
     /**
-     * @return returns the cucumber scenarios as a two dimensional array of {@link PickleWrapper}
-     * scenarios combined with their {@link FeatureWrapper} feature.
+     * @return returns the cucumber scenarios as a two dimensional array of
+     * {@link PickleWrapper} scenarios combined with their
+     * {@link FeatureWrapper} feature.
      */
     public Object[][] provideScenarios() {
+        //Possibly invoked in a multi-threaded context
         try {
-            return getFeatures().stream()
+            return features.stream()
                 .flatMap(feature -> feature.getPickles().stream()
                     .filter(filters)
                     .map(cucumberPickle -> new Object[]{
@@ -144,20 +153,4 @@ public Object[][] provideScenarios() {
             return new Object[][]{new Object[]{new CucumberExceptionWrapper(e), null}};
         }
     }
-
-    /**
-     * Gets features found on the feature path and sends {@link TestRunStarted} and {@link TestSourceRead} events. The method is
-     * idempotent.
-     *
-     * @return a list of {@link Feature} features found on the feature path.
-     */
-    private List<Feature> getFeatures() {
-        if (features == null) {
-            plugins.setSerialEventBusOnEventListenerPlugins(bus);
-            features = featureSupplier.get();
-            bus.send(new TestRunStarted(bus.getInstant()));
-            features.forEach(feature -> bus.send(new TestSourceRead(bus.getInstant(), feature.getUri(), feature.getSource())));
-        }
-        return features;
-    }
 }

testng/src/test/java/io/cucumber/testng/TestNGCucumberRunnerTest.java

@@ -48,11 +48,8 @@ public void runScenarioWithUndefinedStepsStrict() {
 
     @Test
     public void parse_error_propagated_to_testng_test_execution() {
-        testNGCucumberRunner = new TestNGCucumberRunner(ParseError.class);
         try {
-            Object[][] scenarios = testNGCucumberRunner.provideScenarios(); // a CucumberException is caught
-            PickleWrapper pickleWrapper = (PickleWrapper) scenarios[0][0];
-            pickleWrapper.getPickle();
+            testNGCucumberRunner = new TestNGCucumberRunner(ParseError.class);
             Assert.fail("CucumberException not thrown");
         } catch (FeatureParserException e) {
             assertEquals(e.getMessage(), "Failed to parse resource at: classpath:io/cucumber/error/parse-error.feature");