refactor: settle the Error Prone checks that are contract decisions

CHANGES.txt

@@ -1,6 +1,11 @@
 Current (7.13.0)
 Fixed: GITHUB-3418: EmailableReporter2 no longer writes an empty invisible filler row under a result that already listed factory parameters. dumpParametersInfo for method parameters overwrote the flag the factory-parameter dump had just set, so a @Factory instance with no method parameters still got <tr><th class="invisible"/></tr> under those factory columns (Burak Kalaycı)
 Fixed: GITHUB-447: A configuration method that passed now reports in the XML reports the values it ran with, where it reported their final state. testng-results.xml lists the configurations that passed and is written once every invocation of the run is over, but the rendering taken as such a method started was dropped the moment it succeeded -- on the premise that only failed and skipped configurations are listed, which was true until the XML reports started reading those renderings. So a @BeforeMethod handed the row its test method will run with reported what it left behind: VerboseReporter printed prepare([Ljava.lang.Object;)(value(s): [before-configuration]) and the file said [mutated], for the same invocation of the same run. The store now knows whether a reporter will read it after the invocations are over, and holds everything until the last one has, which also means the value is rendered once rather than captured, dropped, and rendered a second time by the fallback -- on @BeforeMethod and @AfterMethod, the most frequent invocations of a run. A run whose only readers sit in the invocation lifecycle, which is what TextReporter and VerboseReporter do, still drops what they are finished with, so it retains nothing it has no use for. What is retained otherwise is bounded by what TestNG already holds: a configuration declaring no parameter stores nothing at all, and the results themselves are kept for the whole run either way, since that is where the report reads them (Julien Herr)
+Fixed: org.testng.internal.ClassHelper.forName no longer risks a ConcurrentModificationException when a class loader is registered while a suite is running. The list behind addClassLoader was a Vector, whose synchronised add says nothing about the iteration forName does over it from the runner's threads; it is a CopyOnWriteArrayList, which iterates a snapshot. addClassLoader is public static and reachable from a user thread, so the two really can overlap (Julien Herr)
+New: org.testng.ITestContext.getStartInstant() and getEndInstant() answer when a <test> started and stopped as java.time.Instant, and getStartDate() and getEndDate() are deprecated in their favour. Both new methods are default and built on the old pair, so an existing implementation of ITestContext keeps working and keeps compiling; the old pair stays abstract for now, because two mutually defaulting methods would compile for an implementation overriding neither and then recurse until the stack ends. org.testng.internal.Utils.requireEndInstantOf is new, and a second AbstractXmlReporter.setDurationAttributes overload takes instants beside the deprecated one that takes dates, and TestNG reads the instants everywhere it used to read the dates. As a consequence getStartDate() answers a fresh Date on every call rather than the runner's own field, so a caller that writes to the answer no longer moves the moment the run reports (Julien Herr)
+Changed: the comma separated command line values -testclass, -testnames and -spilistenerstoskip now have each element trimmed, and their empty elements dropped. "-testclass a.B, a.C" used to ask the class loader for a class called " a.C" and fail, and "-testclass ''" used to ask it for a class called "". The first now runs both classes. The second, and any value that names no class once trimmed such as "-testclass ' , ,'", is rejected by the command line validation with "You need to specify at least one testng.xml, one class or one method" rather than starting a run that selects nothing. A value with no space and no empty element is split exactly as before. The two implementations of the command line -- CliConfigurer and the deprecated TestNG.configure(CommandLineArgs) -- change together, which is what CliConfigurerParityTest requires and cannot itself detect (Julien Herr)
+Changed: three internal types whose equals compares getClass are now final: org.testng.internal.collections.Pair, org.testng.internal.IObject.IdentifiableObject and org.testng.internal.KeyAwareAutoCloseableLock.AutoReleasable. None has a subclass in TestNG, and a getClass-based equals already meant a subclass could never be equal to its base, so sealing them takes away nothing that worked. They are listed under Possible backward incompatible changes below (Julien Herr)
+Changed: org.testng.xml.XmlTest.getMetaGroups() and org.testng.TestNG.runSuitesLocally() answer a mutable collection from every path. Both answered Collections.emptyMap()/emptyList() from one branch -- a test that declares no <groups>, a run that found no suite -- and a fresh HashMap or ArrayList from the others, so whether the answer could be written to depended on which branch ran, and a caller that got it wrong found out at runtime. Nothing in TestNG writes to either, and widening what a caller may do cannot break one that already worked. Two internal methods, IAnnotationFinder.findInheritedAnnotations and ITestInvoker.cancelRemainingInvocations, had the same split and now answer a mutable empty list from that branch too (Julien Herr)
 Fixed: GITHUB-3243: The throwable that ended a worker is no longer discarded. Since GITHUB-3238 such a worker still has its nodes marked finished, so the graph moves on and the run comes back -- but that also makes it indistinguishable from a worker that ran cleanly, and GraphOrchestrator was the last place holding the cause. A listener whose class failed to initialise, for instance, took its ExceptionInInitializerError with it and the test simply failed with no explanation; the error was reachable only under a debugger. The orchestrator now keeps those throwables and TestTaskExecutor and SuiteTaskExecutor log them once the graph is done. The scheduling is deliberately unchanged: skipping the status update for a failed worker is what makes the run hang (Laszlo Kalina)
 Fixed: GITHUB-447: testng-results.xml now reports the values an invocation ran with as they were when it started. The file is written once every invocation of the run is over, so a data provider that hands the same mutable row to every invocation, or a test that changes what it was given, left every <test-method> of that method carrying the value's final state. Only a parameter implementing Cloneable escaped it, and only because ITestResult had kept a reflective clone of it; the XML reports now read the rendering TestNG takes as each invocation starts, which asks nothing of the parameter's type. XMLReporter and PerSuiteXMLReporter are both covered, and a value is still rendered once per invocation however many built-in reports read it (Julien Herr)
 Changed: An array parameter is written to testng-results.xml by its contents rather than by its identity: <value><![CDATA[[1, 2]]]></value> where the file used to hold [I@1b6d3586. GITHUB-2315 made the console reports print the contents of a native array, but the XML report went on calling toString() on the array itself, which answers an identity hash that differs from run to run -- so a tool parsing those values read something it could not compare between two runs of the same suite. Every other value is written exactly as before: a String unquoted, an empty one as empty CDATA, and an absent one as <value is-null="true"/> (Julien Herr)
@@ -15,7 +20,7 @@ Changed: org.testng.ITestContext.getInjectorFactory() answers the new IInjectorF
 Changed: org.testng.ITestResult.getMethod(), getName() and getInstanceName() no longer answer null, and setTestName no longer accepts null. The only result that had no method was the parameter carrier described in the entry below. Thirty-eight places in TestNG asserted the method was there through org.testng.internal.Utils.requireMethodOf and not one of them tested it; that helper is removed, its assertion having become the compiler's job (Julien Herr)
 Fixed: The result an invocation starts from carries its test method from the moment it is created, rather than being handed one by ConfigInvoker a few statements later. A @BeforeMethod that declares an ITestResult parameter is given that carrier, and used to see getName() and getInstanceName() answer null; both now answer the method and class names. The carrier is named when it is built, which is before the configuration method runs, so a class whose ITest.getTestName() is only set in that configuration method still reports the older name on the carrier and the newer one on the reported result. The carrier still knows nothing about the outcome: its status stays CREATED, its millis stay zero and it carries no test context. In memory friendly mode it keeps holding the live method rather than the lightweight snapshot, because that is the method a configuration method reaches through it to mutate (Julien Herr)
 Changed: A <test> built without going through a constructor -- which is how the YAML parser builds one -- now carries the same "Default XmlTest name <uuid>" the XML and the programmatic paths have always had, so org.testng.xml.XmlTest.getName() and org.testng.ITestContext.getName() no longer answer null. Nineteen places dereferenced that name: two of them keyed a ConcurrentHashMap in FailedReporter, which throws on a null key, and XmlSuite.toXml() fed it to Properties.setProperty, which rejects a null value. Two nameless tests in one YAML suite used to be rejected as duplicates named "null"; they now carry different names and both run (Julien Herr)
-New: org.testng.internal.Utils.durationOf(ITestContext) answers how long a <test> ran in milliseconds, folding the end-minus-start expression that four reporters each spelled out. It asserts the context has finished, which is what all four already did through requireEndDateOf (Julien Herr)
+New: org.testng.internal.Utils.durationOf(ITestContext) answers how long a <test> ran in milliseconds, folding the end-minus-start expression that four reporters each spelled out. It asserts the context has finished, which is what all four already did for themselves (Julien Herr)
 Fixed: GITHUB-3358, GITHUB-3359: every applicable @BeforeMethod(firstTimeOnly = true) now runs once, including a child method after a parent one, under a parallel data provider, and for overloaded @Test methods that share a name. The previous per-test-method token skipped later firstTimeOnly configurations and hid their failures. Parallel workers now wait for that firstTimeOnly method to finish before they continue, so a later data-provider row cannot start while the configuration is still running. After-configuration listeners now fire for a firstTimeOnly method that actually ran (Burak Kalaycı)
 Fixed: GITHUB-3385: XmlTest.addIncludedGroup now creates a missing <run> the same way addExcludedGroup and XmlSuite already do, so setGroups(new XmlGroups()) or addMetaGroup no longer throw NullPointerException. XmlTest.equals also compares a missing <run> instead of throwing when only the other side lacks one (Burak Kalaycı)
 Fixed: GITHUB-3377: setListenerClasses (and the CLI -listener flag) now supply a GuiceContext when instantiating listeners, matching suite XML <listeners>, so a @Guice-annotated listener no longer throws NullPointerException. CLI listeners are instantiated after the suite file is parsed, so they inherit that suite's Guice parent-module (Burak Kalaycı)
@@ -41,6 +46,13 @@ Fixed: A <package> tag that carries no name attribute is now reported the way an
 
 Possible backward incompatible changes:
 
+- org.testng.internal.collections.Pair, org.testng.internal.IObject.IdentifiableObject and
+  org.testng.internal.KeyAwareAutoCloseableLock.AutoReleasable are final. All three are public
+  members of internal, OSGi exported packages, so a subclass outside TestNG compiled until now; it
+  stops compiling. Each of the three compares getClass in equals, which is to say an instance of
+  such a subclass was never equal to a Pair, an IdentifiableObject or an AutoReleasable, and never
+  matched one as a map key.
+
 - org.testng.ITestContext.getInjectorFactory() is declared non-null and its default answers
   IInjectorFactory.NONE. Code testing the result for null must test for the token instead. A custom
   ITestContext that never overrode it used to fail a @Guice test class with a NullPointerException

build-logic/code-quality/src/main/kotlin/testng.errorprone.gradle.kts

@@ -12,6 +12,12 @@ dependencies {
 }
 
 tasks.withType<JavaCompile>().configureEach {
+    // javac stops after a hundred warnings per task and caps the summary count with them, so
+    // warning 101 is invisible and the tally understates itself. Four tasks reported exactly
+    // "100 warnings" before this line: testng-core-api and testng-core, main and test. A build
+    // that hides diagnostics cannot answer whether a check is ready to be raised to an error.
+    options.compilerArgs.addAll(listOf("-Xmaxwarns", "100000", "-Xmaxerrs", "100000"))
+
     options.errorprone {
         disableWarningsInGeneratedCode.set(true)
 
@@ -36,21 +42,40 @@ tasks.withType<JavaCompile>().configureEach {
         // remain carry a @SuppressWarnings saying why. Finalize is here on new violations alone --
         // both of its sites are suppressed, because the finalizers are what the leak test watches.
         //
-        // Measure before adding one: javac caps at 100 warnings per compile task and several
-        // tasks here are past that, so an ordinary build undercounts. Nothing raises the cap, so
-        // count from a throwaway init script that adds -Xmaxwarns rather than from a plain build.
+        // Measure before adding one. javac caps at 100 warnings per compile task and several
+        // tasks here are past that, so an ordinary build used to undercount; the -Xmaxwarns above
+        // raises the cap, so a plain build now counts them all and no init script is needed.
         //
         // Promoting also takes a check out of disableWarningsInGeneratedCode above: Error Prone
         // only honours that exemption while the check is below ERROR. Nothing here generates Java
         // today, so the list costs nothing; a module that adds a processor pays for it.
+        //
+        // Six of them are contract questions rather than tidiness -- what equals means, what a
+        // caller may do with a returned collection, which separator is a pattern -- and none of
+        // them fails a test when it is answered wrongly. That is what makes stopping the build the
+        // only thing that would catch the next one: EqualsGetClass, JavaUtilDate, JdkObsolete,
+        // MixedMutabilityReturnType, ReferenceEquality and StringSplitter.
+        //
+        // StringSplitter reports less than it matches, so "zero sites" means less for it than for
+        // the rest. It stays silent unless it can build a Guava Splitter fix, which needs the
+        // split to be a variable initialiser, a for-each subject or an array access; the same call
+        // assigned to an existing variable, or passed straight to a method, is never reported. The
+        // source was swept by hand to close that gap, so what is left in main is one deliberate
+        // regular expression in ClassHelper. A new String.split can still enter this way.
         error(
             "BadImport",
             "BooleanLiteral",
+            "EqualsGetClass",
             "Finalize",
             "InconsistentCapitalization",
+            "JavaUtilDate",
+            "JdkObsolete",
             "MissingOverride",
+            "MixedMutabilityReturnType",
             "NotJavadoc",
+            "ReferenceEquality",
             "StringCaseLocaleUsage",
+            "StringSplitter",
             "TypeParameterUnusedInFormals",
             "UnnecessaryParentheses",
             "UnusedVariable",

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

@@ -1,10 +1,10 @@
 package org.testng.cli;
 
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.List;
 import java.util.Objects;
 import java.util.Optional;
+import org.jspecify.annotations.Nullable;
 import org.testng.IExecutorServiceFactory;
 import org.testng.IInjectorFactory;
 import org.testng.ITestNGListener;
@@ -56,7 +56,10 @@ private static <T> Class<? extends T> uncheckedSubclass(Class<?> clazz) {
    * @throws CliParseException when the combination of options cannot select anything to run.
    */
   public static void validate(CliOptions cli) {
-    String testClasses = cli.testClass;
+    // What configure() will make of it, not the raw text: -testclass "" and -testclass " , ,"
+    // name no class at all, and selecting nothing has to be rejected here rather than produce a
+    // run with no classes and no complaint.
+    String testClasses = selectedTestClasses(cli.testClass);
     List<String> testNgXml = cli.suiteFiles;
     String testJar = cli.testJar;
     List<String> methods = cli.commandLineMethods;
@@ -80,6 +83,17 @@ public static void validate(CliOptions cli) {
     }
   }
 
+  /**
+   * @return the option value when it names at least one class, {@code null} when it names none, so
+   *     that validation treats "names nothing" the same way it treats "was not given".
+   */
+  private static @Nullable String selectedTestClasses(@Nullable String testClass) {
+    if (testClass == null || Utils.splitCommaSeparated(testClass).isEmpty()) {
+      return null;
+    }
+    return testClass;
+  }
+
   /**
    * Applies the parsed command line onto a {@link TestNG} instance.
    *
@@ -135,17 +149,16 @@ public static void configure(TestNG testng, CliOptions cli) {
 
     String testClasses = cli.testClass;
     if (null != testClasses) {
-      String[] strClasses = testClasses.split(",");
       List<Class<?>> classes = new ArrayList<>();
-      for (String c : strClasses) {
+      for (String c : Utils.splitCommaSeparated(testClasses)) {
         classes.add(ClassHelper.fileToClass(c));
       }
 
       testng.setTestClasses(classes.toArray(new Class[0]));
     }
 
     if (cli.testNames != null) {
-      testng.setTestNames(Arrays.asList(cli.testNames.split(",")));
+      testng.setTestNames(Utils.splitCommaSeparated(cli.testNames));
       testng.setIgnoreMissedTestNames(cli.ignoreMissedTestNames);
     }
 
@@ -161,7 +174,8 @@ public static void configure(TestNG testng, CliOptions cli) {
     testng.setXmlPathInJar(cli.xmlPathInJar);
     testng.setSkipFailedInvocationCounts(cli.skipFailedInvocationCounts);
     testng.toggleFailureIfAllTestsWereSkipped(cli.failIfAllTestsSkipped);
-    testng.setListenersToSkipFromBeingWiredInViaServiceLoaders(cli.spiListenersToSkip.split(","));
+    testng.setListenersToSkipFromBeingWiredInViaServiceLoaders(
+        Utils.splitCommaSeparated(cli.spiListenersToSkip).toArray(new String[0]));
 
     testng.setOverrideIncludedMethods(cli.overrideIncludedMethods);