refactor(core): extract the command line front end out of testng-core

CHANGES.txt

@@ -1,6 +1,15 @@
 Current (7.13.0)
 New: Added OpenRewrite to the build with a hand-picked recipe list (see rewrite.yml), and applied it to the main sources (Julien Herr)
 Fixed: Remove leftover dead JUnit code: the deprecated unused ConversionUtils and orphaned JUnit test samples, following the removal of JUnit execution support in 7.10.0 (Julien Herr)
+New: Extract the command line front end out of testng-core into the new testng-cli and testng-jcommander modules, so that testng-core no longer depends on JCommander (Julien Herr)
+  - org.testng.TestNG.main now delegates to an org.testng.ITestNGCliRunner looked up through the ServiceLoader. The reference implementation is bundled inside the org.testng:testng jar, so "java -cp testng.jar org.testng.TestNG suite.xml" is unchanged. Repackaging testng.jar without META-INF/services/org.testng.ITestNGCliRunner now disables the command line.
+  - testng-core no longer brings org.jcommander:jcommander transitively. Embedders that drive TestNG through its Java API drop a dependency; those that relied on it being on the classpath have to declare it.
+  - Deprecated: TestNG.main, TestNG.privateMain, org.testng.CommandLineArgs (use org.testng.cli.CliOptions), TestNG.configure(CommandLineArgs) (use org.testng.cli.CliConfigurer) and TestNG.validateCommandLineParameters. They all remain available and are scheduled for removal in 8.0; see the behaviour changes listed below.
+  - TestNG.validateCommandLineParameters now reports failures as org.testng.TestNGException instead of JCommander's ParameterException. Both are unchecked, so the signature is unchanged, but a caller catching ParameterException no longer catches it.
+  - TestNG gained setListenerComparatorClass, setListenerFactoryClass, setExecutorServiceFactoryClass and setInjectorFactoryClass, which instantiate through the object factory in use. They are named rather than overloaded so that existing calls passing a bare null keep compiling.
+  - TestNG.setThreadCount now raises a TestNGException for a value below 1 instead of printing the usage banner and calling System.exit(1). The command line output is unchanged, since TestNG.main turns that exception into the same message and exit code; embedders get an exception they can handle.
+  - ITestNGCliRunner implementations never terminate the JVM: an unusable command line comes back as a TestNGException. Only TestNG.main turns that into a message, a usage banner and exit code 1, so the observable command line behaviour is unchanged. TestNG.privateMain now throws where it used to exit, which is what an embedder wants.
+  - Moved: org.testng.Converter is now org.testng.cli.jcommander.Converter. It is still bundled in testng.jar.
 Update: Dependency refresh: Guice 6.0.0, JCommander 2.0, snakeyaml 2.6, slf4j-api 2.0.18. Guice 7 and JCommander 3 were skipped: they require jakarta.inject and Java 17 respectively
 Fixed: GITHUB-3242: use-global-thread-pool no longer refuses to start a suite when the number of data-driven tests reaches thread-count (Krishnan Mahadevan)
 New: GITHUB-3290: Support data providers that return Stream<Object[]> or Stream<Object>; the stream is consumed lazily and closed once its rows have been consumed (Krishnan Mahadevan)

settings.gradle.kts

@@ -22,9 +22,11 @@ develocity {
 include(":testng")
 include(":testng-api")
 include(":testng-bom")
+include(":testng-cli")
 include(":testng-collections")
 include(":testng-core")
 include(":testng-core-api")
+include(":testng-jcommander")
 include(":testng-reflection-utils")
 include(":testng-runner-api")
 include(":testng-test-kit")

testng-cli/src/main/java/org/testng/cli/AbstractCliRunner.java

@@ -0,0 +1,50 @@
+package org.testng.cli;
+
+import org.testng.ITestListener;
+import org.testng.ITestNGCliRunner;
+import org.testng.TestNG;
+import org.testng.TestNGException;
+
+/**
+ * Base class for command line front ends. Subclasses only have to turn {@code argv} into {@link
+ * CliOptions} and print a usage banner; validation, configuration and the run itself are shared.
+ *
+ * <p>Nothing here terminates the JVM: a command line that cannot be honoured comes back as a {@link
+ * CliParseException}, which lets the same code drive a real command line, a test, or an embedding
+ * process.
+ *
+ * @since 7.13
+ */
+public abstract class AbstractCliRunner implements ITestNGCliRunner {
+
+  /**
+   * Parses the raw command line. This is the only thing a front end has to supply.
+   *
+   * @param argv the TestNG command line parameters.
+   * @return the parsed options.
+   * @throws CliParseException when {@code argv} is not a valid command line.
+   */
+  protected abstract CliOptions parse(String[] argv);
+
+  @Override
+  public TestNG run(String[] argv, ITestListener listener) {
+    TestNG result = new TestNG();
+
+    if (null != listener) {
+      result.addListener(listener);
+    }
+
+    CliOptions cli = parse(argv);
+    CliConfigurer.validate(cli);
+    CliConfigurer.configure(result, cli);
+
+    try {
+      result.run();
+    } catch (TestNGException ex) {
+      // A command line reports a broken run as a failing status, not as a propagating exception.
+      result.reportRunFailure(ex);
+    }
+
+    return result;
+  }
+}

testng-cli/src/main/java/org/testng/cli/CliConfigurer.java

@@ -0,0 +1,243 @@
+package org.testng.cli;
+
+import java.util.Arrays;
+import java.util.List;
+import java.util.Objects;
+import java.util.Optional;
+import org.testng.IExecutorServiceFactory;
+import org.testng.IInjectorFactory;
+import org.testng.ITestNGListener;
+import org.testng.ITestNGListenerFactory;
+import org.testng.ITestObjectFactory;
+import org.testng.ITestRunnerFactory;
+import org.testng.ListenerComparator;
+import org.testng.TestNG;
+import org.testng.collections.Lists;
+import org.testng.internal.ClassHelper;
+import org.testng.internal.Utils;
+import org.testng.log4testng.Logger;
+import org.testng.xml.XmlSuite;
+
+/**
+ * Turns the values a parser collected into {@link CliOptions} into a configured {@link TestNG}
+ * instance. This is where every "string to object" conversion a command line needs lives: class
+ * loading, listener wiring, method selector parsing and reporter configuration.
+ *
+ * <p>The deprecated {@code TestNG#configure(org.testng.CommandLineArgs)} keeps a frozen copy of
+ * this behaviour for the benefit of subclasses such as {@code RemoteTestNG}; both must stay in sync
+ * until that method is removed.
+ *
+ * @since 7.13
+ */
+public final class CliConfigurer {
+
+  // Deliberately keyed on TestNG so that the message reads the same as before the CLI extraction.
+  private static final Logger LOGGER = Logger.getLogger(TestNG.class);
+
+  private static final String BAD_METHOD_SELECTOR =
+      "Method selector value was not in the format org.example.Selector:4";
+
+  private CliConfigurer() {}
+
+  /**
+   * Narrows a class loaded by name without checking it, so that an unsuitable {@code
+   * -objectfactory} or {@code -testrunfactory} fails where it did before this logic left {@code
+   * testng-core}: when the instance is created, not here.
+   */
+  @SuppressWarnings("unchecked")
+  private static <T> Class<? extends T> uncheckedSubclass(Class<?> clazz) {
+    return (Class<? extends T>) clazz;
+  }
+
+  /**
+   * Double checks that the command line parameters are consistent.
+   *
+   * @param cli the parsed command line.
+   * @throws CliParseException when the combination of options cannot select anything to run.
+   */
+  public static void validate(CliOptions cli) {
+    String testClasses = cli.testClass;
+    List<String> testNgXml = cli.suiteFiles;
+    String testJar = cli.testJar;
+    List<String> methods = cli.commandLineMethods;
+
+    if (testClasses == null
+        && testJar == null
+        && (testNgXml == null || testNgXml.isEmpty())
+        && (methods == null || methods.isEmpty())) {
+      throw new CliParseException(
+          "You need to specify at least one testng.xml, one class or one method");
+    }
+
+    String groups = cli.groups;
+    String excludedGroups = cli.excludedGroups;
+
+    if (testJar == null
+        && (null != groups || null != excludedGroups)
+        && testClasses == null
+        && (testNgXml == null || testNgXml.isEmpty())) {
+      throw new CliParseException("Groups option should be used with testclass option");
+    }
+  }
+
+  /**
+   * Applies the parsed command line onto a {@link TestNG} instance.
+   *
+   * @param testng the instance to configure.
+   * @param cli the parsed command line.
+   */
+  public static void configure(TestNG testng, CliOptions cli) {
+    Objects.requireNonNull(
+        cli.spiListenersToSkip, CliOptions.LISTENERS_TO_SKIP_VIA_SPI + " must not be null");
+    Optional.ofNullable(cli.useGlobalThreadPool).ifPresent(testng::shouldUseGlobalThreadPool);
+    Optional.ofNullable(cli.shareThreadPoolForDataProviders)
+        .ifPresent(testng::shareThreadPoolForDataProviders);
+    // FIXME: the field defaults to FALSE rather than null, so this toggle is switched on for every
+    // command line run and -propagateDataProviderFailureAsTestFailure cannot be turned off. Kept
+    // verbatim for parity with the frozen TestNG#configure(CommandLineArgs); fixing it is a
+    // behaviour change that belongs in its own issue.
+    Optional.ofNullable(cli.propagateDataProviderFailureAsTestFailure)
+        .ifPresent(value -> testng.propagateDataProviderFailureAsTestFailure());
+    testng.setReportAllDataDrivenTestsAsSkipped(cli.includeAllDataDrivenTestsWhenSkipping);
+
+    Optional.ofNullable(cli.listenerFactory)
+        .map(ClassHelper::forName)
+        .filter(ITestNGListenerFactory.class::isAssignableFrom)
+        .map(it -> it.asSubclass(ITestNGListenerFactory.class))
+        .ifPresent(testng::setListenerFactoryClass);
+
+    Optional.ofNullable(cli.generateResultsPerSuite).ifPresent(testng::setGenerateResultsPerSuite);
+
+    Optional.ofNullable(cli.listenerComparator)
+        .map(ClassHelper::forName)
+        .filter(ListenerComparator.class::isAssignableFrom)
+        .map(it -> it.asSubclass(ListenerComparator.class))
+        .ifPresent(testng::setListenerComparatorClass);
+
+    if (cli.verbose != null) {
+      testng.setVerbose(cli.verbose);
+    }
+    if (cli.dependencyInjectorFactoryClass != null) {
+      Class<?> clazz = ClassHelper.forName(cli.dependencyInjectorFactoryClass);
+      if (clazz != null && IInjectorFactory.class.isAssignableFrom(clazz)) {
+        testng.setInjectorFactoryClass(clazz.asSubclass(IInjectorFactory.class));
+      }
+    }
+    Optional.ofNullable(cli.threadPoolFactoryClass)
+        .map(ClassHelper::forName)
+        .filter(IExecutorServiceFactory.class::isAssignableFrom)
+        .map(it -> it.asSubclass(IExecutorServiceFactory.class))
+        .ifPresent(testng::setExecutorServiceFactoryClass);
+
+    testng.setOutputDirectory(cli.outputDirectory);
+
+    String testClasses = cli.testClass;
+    if (null != testClasses) {
+      String[] strClasses = testClasses.split(",");
+      List<Class<?>> classes = Lists.newArrayList();
+      for (String c : strClasses) {
+        classes.add(ClassHelper.fileToClass(c));
+      }
+
+      testng.setTestClasses(classes.toArray(new Class[0]));
+    }
+
+    if (cli.testNames != null) {
+      testng.setTestNames(Arrays.asList(cli.testNames.split(",")));
+      testng.setIgnoreMissedTestNames(cli.ignoreMissedTestNames);
+    }
+
+    // Note: can't use a Boolean field here because we are allowing a boolean
+    // parameter with an arity of 1 ("-usedefaultlisteners false")
+    if (cli.useDefaultListeners != null) {
+      testng.setUseDefaultListeners("true".equalsIgnoreCase(cli.useDefaultListeners));
+    }
+
+    testng.setGroups(cli.groups);
+    testng.setExcludedGroups(cli.excludedGroups);
+    testng.setTestJar(cli.testJar);
+    testng.setXmlPathInJar(cli.xmlPathInJar);
+    testng.setSkipFailedInvocationCounts(cli.skipFailedInvocationCounts);
+    testng.toggleFailureIfAllTestsWereSkipped(cli.failIfAllTestsSkipped);
+    testng.setListenersToSkipFromBeingWiredInViaServiceLoaders(cli.spiListenersToSkip.split(","));
+
+    testng.setOverrideIncludedMethods(cli.overrideIncludedMethods);
+
+    if (cli.parallelMode != null) {
+      testng.setParallel(cli.parallelMode);
+    }
+    if (cli.configFailurePolicy != null) {
+      testng.setConfigFailurePolicy(XmlSuite.FailurePolicy.getValidPolicy(cli.configFailurePolicy));
+    }
+    if (cli.threadCount != null) {
+      testng.setThreadCount(cli.threadCount);
+    }
+    if (cli.dataProviderThreadCount != null) {
+      testng.setDataProviderThreadCount(cli.dataProviderThreadCount);
+    }
+    if (cli.suiteName != null) {
+      testng.setDefaultSuiteName(cli.suiteName);
+    }
+    if (cli.testName != null) {
+      testng.setDefaultTestName(cli.testName);
+    }
+    if (cli.listener != null) {
+      String sep = ";";
+      if (cli.listener.contains(",")) {
+        sep = ",";
+      }
+      String[] strs = Utils.split(cli.listener, sep);
+      List<Class<? extends ITestNGListener>> classes = Lists.newArrayList();
+
+      for (String cls : strs) {
+        Class<?> clazz = ClassHelper.fileToClass(cls);
+        if (ITestNGListener.class.isAssignableFrom(clazz)) {
+          classes.add(clazz.asSubclass(ITestNGListener.class));
+        }
+      }
+
+      testng.setListenerClasses(classes);
+    }
+
+    if (null != cli.methodSelectors) {
+      String[] strs = Utils.split(cli.methodSelectors, ",");
+      for (String cls : strs) {
+        String[] sel = Utils.split(cls, ":");
+        try {
+          if (sel.length == 2) {
+            testng.addMethodSelector(sel[0], Integer.parseInt(sel[1]));
+          } else {
+            LOGGER.error(BAD_METHOD_SELECTOR);
+          }
+        } catch (NumberFormatException nfe) {
+          LOGGER.error(BAD_METHOD_SELECTOR);
+        }
+      }
+    }
+
+    if (cli.objectFactory != null) {
+      testng.setObjectFactory(
+          CliConfigurer.<ITestObjectFactory>uncheckedSubclass(
+              ClassHelper.fileToClass(cli.objectFactory)));
+    }
+    if (cli.testRunnerFactory != null) {
+      testng.setTestRunnerFactoryClass(
+          CliConfigurer.<ITestRunnerFactory>uncheckedSubclass(
+              ClassHelper.fileToClass(cli.testRunnerFactory)));
+    }
+
+    testng.addReporter(cli.reporter);
+
+    if (!cli.commandLineMethods.isEmpty()) {
+      testng.setCommandLineMethods(cli.commandLineMethods);
+    }
+
+    if (cli.suiteFiles != null) {
+      testng.setTestSuites(cli.suiteFiles);
+    }
+
+    testng.setSuiteThreadPoolSize(cli.suiteThreadPoolSize);
+    testng.setRandomizeSuites(cli.randomizeSuites);
+    testng.alwaysRunListeners(cli.alwaysRunListeners);
+  }
+}