Streamline TestResult management from listeners

Fixes #1632

CHANGES.txt

@@ -1,4 +1,5 @@
 Current
+Fixed: GITHUB-1632: throwing SkipException sets iTestResult status to Failure instead of Skip (Julien Herr & Krishnan Mahadevan)
 New  : GITHUB-2456: Add onDataProviderFailure listener (Krishnan Mahadevan)
 Fixed: GITHUB-2445: NPE in FailedReporter.java With Tests Created in Factory (Arham Jain)
 Fixed: GITHUB-2428: Configuration methods have the same test class instance when @Factory is being used (Nan Liang)

src/main/java/org/testng/internal/BaseInvoker.java

@@ -52,7 +52,17 @@ protected void runInvokedMethodListeners(
     InvokedMethodListenerInvoker invoker =
         new InvokedMethodListenerInvoker(listenerMethod, testResult, testResult.getTestContext());
     for (IInvokedMethodListener currentListener : m_invokedMethodListeners) {
-      invoker.invokeListener(currentListener, invokedMethod);
+      try {
+        invoker.invokeListener(currentListener, invokedMethod);
+      } catch (SkipException e) {
+        String msg = String.format(
+            "Caught a [%s] exception from one of listeners %s. Will mark [%s()] as SKIPPED.",
+            SkipException.class.getSimpleName(), currentListener.getClass().getName(),
+            invokedMethod.getTestMethod().getQualifiedName());
+        Utils.warn(msg);
+        testResult.setStatus(ITestResult.SKIP);
+        testResult.setThrowable(e);
+      }
     }
   }
 

src/main/java/org/testng/internal/ConfigInvoker.java

@@ -283,6 +283,7 @@ public void invokeConfigurations(ConfigMethodArguments arguments) {
           runInvokedMethodListeners(BEFORE_INVOCATION, invokedMethod, testResult);
           testResult.setStatus(ITestResult.SKIP);
           runInvokedMethodListeners(AFTER_INVOCATION, invokedMethod, testResult);
+
           handleConfigurationSkip(
               tm, testResult, configurationAnnotation,
               arguments.getTestMethod(),
@@ -322,6 +323,12 @@ public void invokeConfigurations(ConfigMethodArguments arguments) {
         copyAttributesFromNativelyInjectedTestResult(parameters,
             arguments.getTestMethodResult());
         runConfigurationListeners(testResult, arguments.getTestMethod(), false /* after */);
+        if (testResult.getStatus() == ITestResult.SKIP) {
+          Throwable t = testResult.getThrowable();
+          if (t != null) {
+            throw t;
+          }
+        }
       } catch (Throwable ex) {
         handleConfigurationFailure(
             ex, tm, testResult, configurationAnnotation,
@@ -349,9 +356,19 @@ private void invokeConfigurationMethod(
         new InvokedMethod(System.currentTimeMillis(), testResult);
 
     runInvokedMethodListeners(BEFORE_INVOCATION, invokedMethod, testResult);
+
     if (tm instanceof IInvocationStatus) {
       ((IInvocationStatus) tm).setInvokedAt(invokedMethod.getDate());
     }
+    if (testResult.getStatus() == ITestResult.SKIP) {
+      //There was a skip marked by the listener invocation.
+      testResult.setEndMillis(System.currentTimeMillis());
+      Reporter.setCurrentTestResult(testResult);
+      runInvokedMethodListeners(AFTER_INVOCATION, invokedMethod, testResult);
+
+      Reporter.setCurrentTestResult(null);
+      return ;
+    }
     try {
       Reporter.setCurrentTestResult(testResult);
       ConstructorOrMethod method = tm.getConstructorOrMethod();

src/main/java/org/testng/internal/Utils.java

@@ -228,6 +228,10 @@ public static void error(String errorMessage) {
     LOG.error("[Error] " + errorMessage);
   }
 
+  public static void warn(String warnMsg) {
+    LOG.warn("[Warning] " + warnMsg);
+  }
+
   /* Tokenize the string using the separator. */
   public static String[] split(String string, String sep) {
     if ((string == null) || (string.length() == 0)) {

src/test/java/test/skip/github1632/IssueTest.java

@@ -45,4 +45,10 @@ public void runNoConfigAfterListenerSample() {
     InvokedMethodNameListener listener = run(NoConfigAfterListenerSample.class);
     assertThat(listener.getResult("shouldNotBeExecuted").getStatus()).isEqualTo(ITestResult.SKIP);
   }
+
+  @Test
+  public void runBeforeListenerSkipSample2() {
+    InvokedMethodNameListener listener = run(ListenerMarksMethodAsSkippedSample.class);
+    assertThat(listener.getResult("shouldNotBeExecuted").getStatus()).isEqualTo(ITestResult.SKIP);
+  }
 }

src/test/java/test/skip/github1632/ListenerMarksMethodAsSkippedSample.java

@@ -0,0 +1,31 @@
+package test.skip.github1632;
+
+
+import org.testng.IInvokedMethod;
+import org.testng.IInvokedMethodListener;
+import org.testng.ITestResult;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Listeners;
+import org.testng.annotations.Test;
+
+@Listeners(ListenerMarksMethodAsSkippedSample.MySkipTestListener.class)
+public class ListenerMarksMethodAsSkippedSample {
+
+  @BeforeMethod
+  void beforeMethod() {
+  }
+
+  @Test
+  void shouldNotBeExecuted() {
+  }
+
+  public static class MySkipTestListener implements IInvokedMethodListener {
+
+    @Override
+    public void afterInvocation(IInvokedMethod method, ITestResult testResult) {
+      testResult.setStatus(ITestResult.SKIP);
+    }
+  }
+
+
+}

src/test/java/test_result/AfterListenerSample.java

@@ -7,7 +7,7 @@
 import org.testng.annotations.Listeners;
 import org.testng.annotations.Test;
 
-@Listeners(AfterListenerSample.MySkipTestListener.class)
+
 public class AfterListenerSample {
 
     @Test

src/test/java/test_result/OrderAbidingListener.java

@@ -0,0 +1,117 @@
+package test_result;
+
+import java.util.Arrays;
+import java.util.List;
+import org.testng.IInvokedMethod;
+import org.testng.IInvokedMethodListener;
+import org.testng.ITestContext;
+import org.testng.ITestListener;
+import org.testng.ITestNGListener;
+import org.testng.ITestResult;
+
+public class OrderAbidingListener implements IInvokedMethodListener, ITestListener {
+
+  private final List<ITestNGListener> listeners;
+
+  public OrderAbidingListener(ITestNGListener ...listeners) {
+    this.listeners = Arrays.asList(listeners);
+  }
+
+  @Override
+  public void beforeInvocation(IInvokedMethod method, ITestResult testResult) {
+    listeners.stream()
+        .filter(l-> l instanceof IInvokedMethodListener)
+        .map( l -> (IInvokedMethodListener) l )
+        .forEach(l-> l.beforeInvocation(method, testResult));
+  }
+
+  @Override
+  public void beforeInvocation(IInvokedMethod method, ITestResult testResult,
+      ITestContext context) {
+    listeners.stream()
+        .filter(l-> l instanceof IInvokedMethodListener)
+        .map( l -> (IInvokedMethodListener) l )
+        .forEach(l-> l.beforeInvocation(method, testResult, context));
+
+  }
+
+  @Override
+  public void afterInvocation(IInvokedMethod method, ITestResult testResult) {
+    listeners.stream()
+        .filter(l-> l instanceof IInvokedMethodListener)
+        .map( l -> (IInvokedMethodListener) l )
+        .forEach(l-> l.afterInvocation(method, testResult));
+  }
+
+  @Override
+  public void afterInvocation(IInvokedMethod method, ITestResult testResult, ITestContext context) {
+    listeners.stream()
+        .filter(l-> l instanceof IInvokedMethodListener)
+        .map( l -> (IInvokedMethodListener) l )
+        .forEach(l-> l.afterInvocation(method, testResult, context));
+  }
+
+  @Override
+  public void onStart(ITestContext context) {
+    listeners.stream()
+        .filter(l-> l instanceof ITestListener)
+        .map( l -> (ITestListener) l )
+        .forEach(l-> l.onStart(context));
+  }
+
+  @Override
+  public void onTestStart(ITestResult result) {
+    listeners.stream()
+        .filter(l-> l instanceof ITestListener)
+        .map( l -> (ITestListener) l )
+        .forEach(l-> l.onTestStart(result));
+  }
+
+  @Override
+  public void onTestSuccess(ITestResult result) {
+    listeners.stream()
+        .filter(l-> l instanceof ITestListener)
+        .map( l -> (ITestListener) l )
+        .forEach(l-> l.onTestSuccess(result));
+  }
+
+  @Override
+  public void onTestFailure(ITestResult result) {
+    listeners.stream()
+        .filter(l-> l instanceof ITestListener)
+        .map( l -> (ITestListener) l )
+        .forEach(l-> l.onTestFailure(result));
+  }
+
+  @Override
+  public void onTestSkipped(ITestResult result) {
+    listeners.stream()
+        .filter(l-> l instanceof ITestListener)
+        .map( l -> (ITestListener) l )
+        .forEach(l-> l.onTestSkipped(result));
+  }
+
+  @Override
+  public void onTestFailedButWithinSuccessPercentage(ITestResult result) {
+    listeners.stream()
+        .filter(l-> l instanceof ITestListener)
+        .map( l -> (ITestListener) l )
+        .forEach(l-> l.onTestFailedButWithinSuccessPercentage(result));
+  }
+
+  @Override
+  public void onTestFailedWithTimeout(ITestResult result) {
+    listeners.stream()
+        .filter(l-> l instanceof ITestListener)
+        .map( l -> (ITestListener) l )
+        .forEach(l-> l.onTestFailedWithTimeout(result));
+  }
+
+  @Override
+  public void onFinish(ITestContext context) {
+    listeners.stream()
+        .filter(l-> l instanceof ITestListener)
+        .map( l -> (ITestListener) l )
+        .forEach(l-> l.onFinish(context));
+  }
+}

src/test/java/test_result/ResultStatusTest.java

@@ -4,6 +4,7 @@
 import org.testng.annotations.Test;
 import test.InvokedMethodNameListener;
 import test.SimpleBaseTest;
+import test_result.AfterListenerSample.MySkipTestListener;
 
 import static org.assertj.core.api.Assertions.assertThat;
 
@@ -46,10 +47,12 @@ public void testBeforeListener() {
     @Test
     public void testAfterListener() {
         TestNG tng = create(AfterListenerSample.class);
+        AfterListenerSample.MySkipTestListener testCaseListener = new MySkipTestListener();
 
         // A skipped method is not supposed to be run but, here, it's the goal of the feature
         InvokedMethodNameListener listener = new InvokedMethodNameListener(true, true);
-        tng.addListener(listener);
+        OrderAbidingListener orderAbidingListener = new OrderAbidingListener(testCaseListener, listener);
+        tng.addListener(orderAbidingListener);
 
         tng.run();