Fixing review comments

Author: krmahadevan

testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java

@@ -1036,10 +1036,8 @@ public int invoke(int invCount) {
         // See GITHUB-3238.
         if (!isInvokingListeners.get()) {
           runTestResultListener(r);
-        } else {
-          // Reset the flag since we're handling the listener exception here
-          isInvokingListeners.set(false);
         }
+        isInvokingListeners.set(false);
         m_notifier.addFailedTest(arguments.getTestMethod(), r);
       } // catch
       return invocationCount.get();

testng-core/src/main/java/org/testng/internal/thread/graph/GraphOrchestrator.java

@@ -3,6 +3,7 @@
 import java.util.Comparator;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import java.util.concurrent.*;
 import org.testng.IDynamicGraph;
 import org.testng.collections.Maps;
@@ -73,7 +74,8 @@ private void mapNodeToParent(List<T> freeNodes) {
     }
   }
 
-  private void afterExecute(IWorker<T> r, Throwable t) {
+  private void afterExecute(IWorker<T> original, IWorker<T> result) {
+    IWorker<T> r = Optional.ofNullable(result).orElse(original);
     try (AutoCloseableLock ignore = internalLock.lock()) {
       setStatus(r, computeStatus(r));
       if (graph.getNodeCount() == graph.getNodeCountWithStatus(IDynamicGraph.Status.FINISHED)) {

testng-core/src/main/java/org/testng/internal/thread/graph/TestNGFutureTask.java

@@ -9,9 +9,9 @@
 public class TestNGFutureTask<T> extends FutureTask<IWorker<T>> implements IWorker<T> {
 
   private final IWorker<T> worker;
-  private final BiConsumer<IWorker<T>, Throwable> callback;
+  private final BiConsumer<IWorker<T>, IWorker<T>> callback;
 
-  public TestNGFutureTask(IWorker<T> worker, BiConsumer<IWorker<T>, Throwable> callback) {
+  public TestNGFutureTask(IWorker<T> worker, BiConsumer<IWorker<T>, IWorker<T>> callback) {
     super(worker, worker);
     this.callback = callback;
     this.worker = worker;
@@ -24,14 +24,13 @@ public void run() {
 
   @Override
   protected void done() {
-    Throwable throwable = null;
     IWorker<T> result = null;
     try {
       result = super.get();
     } catch (InterruptedException | ExecutionException e) {
-      throwable = e;
+      // Gobble exception and do nothing.
     }
-    callback.accept(result == null ? worker : result, throwable);
+    callback.accept(worker, result);
   }
 
   @Override

testng-core/src/test/java/test/listeners/ListenersTest.java

@@ -691,12 +691,16 @@ public void ensureInheritanceIsHandledWhenDealingWithListeners() {
     assertThat(testng.getStatus()).isZero();
   }
 
-  @Test(description = "GITHUB-3238")
-  public void ensureListenerFailureDoesNotBreakTestNG() {
-    TestNG testng =
-        create(
-            TestClassWithFailingTestMethodSample.class, TestClassWithPassingTestMethodSample.class);
-    testng.setParallel(ParallelMode.CLASSES);
+  @Test(description = "GITHUB-3238", dataProvider = "dp-3238")
+  public void ensureListenerFailureDoesNotBreakTestNG(ParallelMode parallelMode) {
+    XmlSuite xmlSuite =
+        createXmlSuite(
+            "3238_suite",
+            "3238_test",
+            TestClassWithFailingTestMethodSample.class,
+            TestClassWithPassingTestMethodSample.class);
+    xmlSuite.setParallel(parallelMode);
+    TestNG testng = create(xmlSuite);
     AtomicInteger failingCount = new AtomicInteger(0);
     AtomicInteger passingCount = new AtomicInteger(0);
     testng.addListener(
@@ -713,7 +717,12 @@ public void onTestSuccess(ITestResult result) {
         });
     testng.run();
     assertThat(failingCount.get()).isEqualTo(1);
-    assertThat(passingCount.get()).isEqualTo(1);
+    assertThat(passingCount.get()).isEqualTo(5);
+  }
+
+  @DataProvider(name = "dp-3238")
+  public Object[][] getTestData() {
+    return new Object[][] {{ParallelMode.CLASSES}, {ParallelMode.METHODS}};
   }
 
   private void setupTest(boolean addExplicitListener) {

testng-core/src/test/java/test/listeners/issue3238/TestClassWithFailingTestMethodSample.java

@@ -9,7 +9,6 @@ public class TestClassWithFailingTestMethodSample {
 
   @Test
   public void failingTest() {
-    System.err.println(":::::");
     Assert.fail();
   }
 }

testng-core/src/test/java/test/listeners/issue3238/TestClassWithPassingTestMethodSample.java

@@ -1,8 +1,20 @@
 package test.listeners.issue3238;
 
+import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
 
 public class TestClassWithPassingTestMethodSample {
   @Test
   public void testMethod() {}
+
+  @Test
+  public void anotherTestMethod() {}
+
+  @Test(dataProvider = "dp")
+  public void dataDrivenTest(int ignored) {}
+
+  @DataProvider(name = "dp", parallel = true)
+  public Object[][] dp() {
+    return new Object[][] {{1}, {2}, {3}};
+  }
 }