fix(reports): keep a passed configuration's snapshot for the reports that list it

CHANGES.txt

@@ -1,5 +1,6 @@
 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: 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)

testng-core/src/main/java/org/testng/TestRunner.java

@@ -1178,7 +1178,12 @@ void addConfigurationListener(IConfigurationListener icl) {
    * <p>Being first here also means being last once {@code
    * TestListenerHelper#runPostConfigurationListeners} reverses the order, which is where it
    * belongs: whatever an internal listener drops about a finished configuration, it drops after the
-   * reporters have had it.
+   * reporters have had it. Last among these listeners, that is, and only while nothing reorders
+   * them -- so the guarantee is worth having but is not one to build on. {@code ListenerComparator}
+   * sorts this list before it is reversed and can move it anywhere; the preferential listeners are
+   * merged in after the regular ones; and the invoker appends {@link ConfigurationListener} after
+   * the reversal, which means the result is filed into {@code m_passedConfigurations} -- where the
+   * reports read it -- only once every listener here has been told.
    *
    * @param icl - An internal listener the other listeners depend on.
    */

testng-core/src/main/java/org/testng/internal/reporters/ParameterSnapshotReader.java

@@ -26,7 +26,12 @@
  * only ever sees the run's reporters, so a class that is not one -- {@code XMLSuiteResultWriter}
  * and the {@code jq} panels are the ones to watch, since they hold the reads but are not reporters
  * -- could otherwise wear this and be quietly passed over, leaving its report to fall back to
- * {@link org.testng.ITestResult#getParameters()} with nothing to say it had.
+ * {@link org.testng.ITestResult#getParameters()} with nothing to say it had. A read that arrives
+ * this late is worse off than the fallback suggests: the request below is also what tells the store
+ * to hold what the live reporters are finished with, so a reporter missing from this scan reads a
+ * store that dropped exactly the results it came for -- every configuration method that passed.
+ * That is what {@code Main} will have to declare when the {@code jq} report is migrated; a default
+ * run hides it, because {@code XMLReporter} has already asked.
  *
  * <p>Internal, and otherwise empty: implementing it is a statement about TestNG's own reporting,
  * not an extension point. A third party reporter that wants the snapshots implements {@link
@@ -50,7 +55,7 @@ public interface ParameterSnapshotReader extends IReporter {
   static void requestCaptureIfAnyReads(
       Collection<? extends IReporter> reporters, Collection<ISuite> suites) {
     if (reporters.stream().anyMatch(ParameterSnapshotReader.class::isInstance)) {
-      suites.forEach(ParameterSnapshots::requestCaptureFor);
+      suites.forEach(ParameterSnapshots::requestCaptureHeldUntilReportingFor);
     }
   }
 }

testng-core/src/main/java/org/testng/internal/reporters/ParameterSnapshotRecorder.java

@@ -46,9 +46,10 @@ public void beforeConfiguration(ITestResult result) {
 
   @Override
   public void onConfigurationSuccess(ITestResult result) {
-    // Only failed and skipped configurations are listed, so nothing will read this one again -- and
-    // the reporters that print a configuration as it passes already have: a configuration finishing
-    // is dispatched in reverse, which makes this listener, registered first, the last one told.
+    // Offers the snapshot back rather than dropping it: what this listener knows is that the
+    // reporters printing a configuration as it passes have been told -- a configuration finishing
+    // is dispatched in reverse, so this one, registered first, is told last among them. Whether the
+    // offer is taken is the store's to decide; see ParameterSnapshots#discard.
     snapshots.discard(result);
   }
 }

testng-core/src/main/java/org/testng/internal/reporters/ParameterSnapshots.java

@@ -28,7 +28,9 @@
  *
  * <p>Capture is opt-in: rendering a value runs the user's {@code toString()}, which is pure cost
  * when no reporter is verbose enough to print it. A consumer says so once with {@link
- * #requestCapture()}.
+ * #requestCapture()}, or with {@link #requestCaptureHeldUntilReporting()} when it will read the
+ * store after the invocations are over -- which is also what decides whether anything may be
+ * dropped along the way. See {@link #discard}.
  */
 public final class ParameterSnapshots {
 
@@ -40,6 +42,7 @@ public final class ParameterSnapshots {
 
   private final Map<ResultKey, ParameterSnapshot> snapshots = new ConcurrentHashMap<>();
   private volatile boolean captureRequested;
+  private volatile boolean heldUntilReporting;
 
   /**
    * Gives a suite the store its invocations will fill and its reporters will read.
@@ -80,14 +83,41 @@ public static void detachFrom(ISuite suite) {
     return attribute instanceof ParameterSnapshots ? (ParameterSnapshots) attribute : null;
   }
 
-  /** Declares that a reporter will read these snapshots, so they are worth taking. */
+  /**
+   * Declares that a reporter will read these snapshots, so they are worth taking, and that it can
+   * live with {@link #discard} dropping a configuration method that passed.
+   *
+   * <p>Which is not the same as reading from inside the invocation lifecycle, tempting as that
+   * shorthand is: {@code TextReporter} reads at {@code onFinish}, once every configuration of its
+   * context has already succeeded. What makes both callers of this safe is narrower -- neither
+   * lists {@link org.testng.ITestContext#getPassedConfigurations()}, which is the only thing {@code
+   * discard} is ever offered. A reporter that starts listing them needs {@link
+   * #requestCaptureHeldUntilReporting()} instead, whenever it reads.
+   */
   public void requestCapture() {
     captureRequested = true;
   }
 
   /**
-   * The same, for a reporter that has a suite rather than a store: a suite without one is a suite
-   * whose snapshots nobody is going to take, which is not the reporter's business.
+   * The same, for a reporter that reads the store once the invocations of the run are over: nothing
+   * is dropped for such a run, since a snapshot the live reporters are finished with is one it has
+   * not seen yet.
+   *
+   * <p>Kept as a second flag rather than as one ordered value on purpose. Both are monotone writes
+   * of {@code true} and neither clears the other, so a run that has both kinds of reader -- the
+   * default one above {@code -verbose 4} -- gets the same answer whichever asks first. Folding them
+   * into one field updated to a maximum would turn two race-free writes into a read-modify-write,
+   * and {@code <suite parallel="tests">} makes those calls from two runners at once.
+   */
+  public void requestCaptureHeldUntilReporting() {
+    captureRequested = true;
+    heldUntilReporting = true;
+  }
+
+  /**
+   * The {@link #requestCapture()} of a reporter that has a suite rather than a store: a suite
+   * without one is a suite whose snapshots nobody is going to take, which is not the reporter's
+   * business.
    *
    * <p>Call it from {@link org.testng.ITestListener#onStart(org.testng.ITestContext)}, which is
    * early enough: a context starts before its own {@code @BeforeTest} configurations and before any
@@ -104,6 +134,20 @@ public static void requestCaptureFor(@Nullable ISuite suite) {
     }
   }
 
+  /**
+   * The same, for a reporter that will read the store once the invocations of the run are over. It
+   * has no invocation lifecycle to ask from, so the run asks on its behalf; see {@link
+   * ParameterSnapshotReader#requestCaptureIfAnyReads}.
+   *
+   * @param suite - A suite about to run, none of whose invocations has started.
+   */
+  public static void requestCaptureHeldUntilReportingFor(@Nullable ISuite suite) {
+    ParameterSnapshots snapshots = of(suite);
+    if (snapshots != null) {
+      snapshots.requestCaptureHeldUntilReporting();
+    }
+  }
+
   /**
    * Captures what {@code result} was invoked with, unless it already has been. Must be called from
    * a lifecycle point the invocation has not run past yet.
@@ -180,22 +224,34 @@ public void captureIfAbsent(ITestResult result) {
   }
 
   /**
-   * Drops what was captured for a result nothing will be reported about anymore -- a configuration
-   * method that succeeded, which no reporter lists once the run is over. It is the caller's
-   * business to know that the live reporters are done with it; see {@link
-   * ParameterSnapshotRecorder#onConfigurationSuccess}. Guarded like {@link #captureIfAbsent}:
-   * {@code @BeforeMethod} / {@code @AfterMethod} invocations are the most frequent events in a run,
-   * and there is nothing to drop when nothing is being captured.
-   *
-   * @param result - The result nothing will be reported about anymore.
+   * Offers back what was captured for a result every live reporter is done with -- a configuration
+   * method that succeeded, which each of them prints as it passes and none of them lists again.
+   *
+   * <p>Half of that decision is the caller's and half is not, which is why it is taken here. The
+   * caller knows the reporters that sit in the invocation lifecycle have had it; only the store
+   * knows whether a reporter that has not run yet still wants it, and one that reads at {@code
+   * generateReport} does: {@code testng-results.xml} lists the configurations that passed. So a run
+   * that asked through {@link #requestCaptureHeldUntilReporting()} keeps everything until {@link
+   * #detachFrom} releases it, and a run whose readers are all live ones drops as it goes.
+   *
+   * <p>Guarded like {@link #captureIfAbsent} for the same reason: {@code @BeforeMethod} /
+   * {@code @AfterMethod} invocations are the most frequent events in a run, and there is nothing to
+   * drop when nothing is being captured.
+   *
+   * @param result - The result the live reporters have finished with.
    */
   public void discard(ITestResult result) {
-    if (!captureRequested) {
+    if (!discardsWhatIsDone()) {
       return;
     }
     snapshots.remove(new ResultKey(result));
   }
 
+  /** Named so that {@code discard} not discarding does not read as a bug. */
+  private boolean discardsWhatIsDone() {
+    return captureRequested && !heldUntilReporting;
+  }
+
   /** @return - Whether anything is still held, which is what {@link #detachFrom} leaves behind. */
   public boolean isEmpty() {
     return snapshots.isEmpty();

testng-core/src/test/java/org/testng/internal/reporters/ParameterSnapshotsTest.java

@@ -8,6 +8,7 @@
 import java.lang.reflect.Proxy;
 import java.util.Arrays;
 import java.util.concurrent.atomic.AtomicInteger;
+import java.util.function.Consumer;
 import org.testng.ITestClass;
 import org.testng.ITestNGMethod;
 import org.testng.ITestResult;
@@ -83,6 +84,51 @@ public void discardedResultsAreForgotten() {
     assertThat(snapshots.find(result)).isNull();
   }
 
+  @Test(
+      description =
+          "A store something reads once the invocations are over keeps what the live reporters are"
+              + " done with")
+  public void aStoreReadAfterTheInvocationsKeepsWhatIsOfferedBack() {
+    ITestResult result = resultOf(new CountingParameter("value"));
+    ParameterSnapshots snapshots = new ParameterSnapshots();
+    snapshots.requestCaptureHeldUntilReporting();
+    snapshots.captureIfAbsent(result);
+
+    snapshots.discard(result);
+
+    assertThat(requireNonNull(snapshots.find(result)).renderedValues()).containsExactly("value");
+  }
+
+  @Test(description = "A live reader arriving after a late one does not make the store drop again")
+  public void aLiveRequestDoesNotCancelALateOne() {
+    assertKeptAfterDiscard(
+        snapshots -> {
+          snapshots.requestCaptureHeldUntilReporting();
+          snapshots.requestCapture();
+        });
+  }
+
+  @Test(description = "A late reader arriving after a live one stops the store dropping")
+  public void aLateRequestDoesNotCancelALiveOne() {
+    assertKeptAfterDiscard(
+        snapshots -> {
+          snapshots.requestCapture();
+          snapshots.requestCaptureHeldUntilReporting();
+        });
+  }
+
+  /** Asks in the given order, then offers a captured result back and expects it kept. */
+  private static void assertKeptAfterDiscard(Consumer<ParameterSnapshots> requests) {
+    ITestResult result = resultOf(new CountingParameter("value"));
+    ParameterSnapshots snapshots = new ParameterSnapshots();
+    requests.accept(snapshots);
+    snapshots.captureIfAbsent(result);
+
+    snapshots.discard(result);
+
+    assertThat(requireNonNull(snapshots.find(result)).renderedValues()).containsExactly("value");
+  }
+
   @Test(
       description =
           "A data provider that supplied the wrong number of values leaves the counts to report"