IHookable and IConfigurable callback discrepancy

Closes testng-team#2704 Ensure that TestNG reports scenarios wherein User defines callbacks for configurations/test methods But fails to invoke those callbacks explicitly And also fails in shipping test status to a user recognized status (PASS|FAILURE|SKIP)
krmahadevan · Jan 12, 2022 · fbb04ef · fbb04ef
1 parent 55e8e52
commit fbb04ef
Show file tree

Hide file tree

Showing 14 changed files with 329 additions and 73 deletions.
diff --git a/CHANGES.txt b/CHANGES.txt
@@ -1,5 +1,6 @@
 Current
 7.6.0
+Fixed: GITHUB-2704: IHookable and IConfigurable callback discrepancy (Krishnan Mahadevan)
 Fixed: GITHUB-2637: Upgrade to JDK11 as the minimum JDK requirements(Krishnan Mahadevan)
 
 7.5

diff --git a/testng-core-api/src/main/java/org/testng/ConfigurationNotInvokedException.java b/testng-core-api/src/main/java/org/testng/ConfigurationNotInvokedException.java
@@ -0,0 +1,19 @@
+package org.testng;
+
+/**
+ * Represents an exception that is thrown when a configuration method is not invoked. One of the
+ * use-cases when this can happen is when the user does the following:
+ *
+ * <ul>
+ *   <li>User defines a configuration method
+ *   <li>The class that houses the configuration method defines support for callbacks via {@link
+ *       IConfigurable} implementation
+ *   <li>User willfully skips invoking the callback and also fails at altering the configuration
+ *       method's status via {@link ITestResult#setStatus(int)}
+ * </ul>
+ */
+public class ConfigurationNotInvokedException extends TestNGException {
+  public ConfigurationNotInvokedException(String string) {
+    super(string);
+  }
+}
diff --git a/testng-core-api/src/main/java/org/testng/ITestResult.java b/testng-core-api/src/main/java/org/testng/ITestResult.java
@@ -1,5 +1,6 @@
 package org.testng;
 
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
 import org.testng.internal.thread.ThreadTimeoutException;
@@ -114,6 +115,29 @@ default List<ITestNGMethod> getSkipCausedBy() {
    */
   String id();
 
+  /**
+   * @return - <code>true</code> if the current test result is either {@link ITestResult#STARTED} or
+   *     {@link ITestResult#CREATED}
+   */
+  default boolean isStatusCreatedOrStarted() {
+    return getStatus() == STARTED || getStatus() == CREATED;
+  }
+
+  /**
+   * @return - A <code>|</code> symbol separated values which includes all user facing statuses
+   *     viz.,
+   *     <ul>
+   *       <li>{@link ITestResult#SUCCESS}
+   *       <li>{@link ITestResult#SUCCESS_PERCENTAGE_FAILURE}
+   *       <li>{@link ITestResult#FAILURE}
+   *       <li>{@link ITestResult#SKIP}
+   *     </ul>
+   */
+  static String validUserFacingStatuses() {
+    return String.join(
+        "|", Arrays.asList("SUCCESS", "FAILURE", "SKIP", "SUCCESS_PERCENTAGE_FAILURE"));
+  }
+
   /**
    * @param result - The test result of a method
    * @return - <code>true</code> if the test failure was due to a timeout.

diff --git a/testng-core-api/src/main/java/org/testng/TestNotInvokedException.java b/testng-core-api/src/main/java/org/testng/TestNotInvokedException.java
@@ -0,0 +1,19 @@
+package org.testng;
+
+/**
+ * Represents an exception that is thrown when a test method is not invoked. One of the use-cases
+ * when this can happen is when the user does the following:
+ *
+ * <ul>
+ *   <li>User defines a test method
+ *   <li>The class that houses the test method defines support for callbacks via {@link IHookable}
+ *       implementation
+ *   <li>User willfully skips invoking the callback and also fails at altering the test method's
+ *       status via {@link ITestResult#setStatus(int)}
+ * </ul>
+ */
+public class TestNotInvokedException extends TestNGException {
+  public TestNotInvokedException(String string) {
+    super(string);
+  }
+}
diff --git a/testng-core/src/main/java/org/testng/internal/MethodHelper.java b/testng-core/src/main/java/org/testng/internal/MethodHelper.java
@@ -484,11 +484,8 @@ private static MatchResults matchMethod(ITestNGMethod[] methods, String regexp)
       String thisMethodName = thisMethod.getName();
       String methodName = usePackage ? calculateMethodCanonicalName(method) : thisMethodName;
       Pair<String, String> cacheKey = Pair.create(regexp, methodName);
-      Boolean match = MATCH_CACHE.get(cacheKey);
-      if (match == null) {
-        match = pattern.matcher(methodName).matches();
-        MATCH_CACHE.put(cacheKey, match);
-      }
+      boolean match =
+          MATCH_CACHE.computeIfAbsent(cacheKey, key -> pattern.matcher(methodName).matches());
       if (match) {
         results.matchedMethods.add(method);
         results.foundAtLeastAMethod = true;

diff --git a/testng-core/src/main/java/org/testng/internal/invokers/ConfigInvoker.java b/testng-core/src/main/java/org/testng/internal/invokers/ConfigInvoker.java
@@ -11,6 +11,7 @@
 import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
+import org.testng.ConfigurationNotInvokedException;
 import org.testng.IClass;
 import org.testng.IConfigurable;
 import org.testng.IInvokedMethodListener;
@@ -374,15 +375,30 @@ private void invokeConfigurationMethod(
         testResult.setStatus(ITestResult.SUCCESS);
         return;
       }
-      if (configurableInstance != null) {
-        MethodInvocationHelper.invokeConfigurable(
-            targetInstance, params, configurableInstance, method.getMethod(), testResult);
+      boolean willfullyIgnored = false;
+      boolean usesConfigurableInstance = configurableInstance != null;
+      if (usesConfigurableInstance) {
+        willfullyIgnored =
+            !MethodInvocationHelper.invokeConfigurable(
+                targetInstance, params, configurableInstance, method.getMethod(), testResult);
       } else {
         MethodInvocationHelper.invokeMethodConsideringTimeout(
             tm, method, targetInstance, params, testResult);
       }
-      testResult.setStatus(ITestResult.SUCCESS);
-    } catch (InvocationTargetException | IllegalAccessException ex) {
+      boolean testStatusRemainedUnchanged = testResult.isStatusCreatedOrStarted();
+      if (usesConfigurableInstance && willfullyIgnored && testStatusRemainedUnchanged) {
+        throw new ConfigurationNotInvokedException(
+            tm.getQualifiedName()
+                + " defines a callback via "
+                + IConfigurable.class.getName()
+                + " but neither the callback was invoked nor the status was altered to "
+                + ITestResult.validUserFacingStatuses());
+      } else {
+        testResult.setStatus(ITestResult.SUCCESS);
+      }
+    } catch (ConfigurationNotInvokedException
+        | InvocationTargetException
+        | IllegalAccessException ex) {
       throwConfigurationFailure(testResult, ex);
       testResult.setStatus(ITestResult.FAILURE);
       throw ex;

diff --git a/testng-core/src/main/java/org/testng/internal/invokers/InvokeMethodRunnable.java b/testng-core/src/main/java/org/testng/internal/invokers/InvokeMethodRunnable.java
@@ -1,13 +1,14 @@
 package org.testng.internal.invokers;
 
+import java.util.Optional;
 import java.util.concurrent.Callable;
 import org.testng.IHookable;
 import org.testng.ITestNGMethod;
 import org.testng.ITestResult;
 import org.testng.internal.ConstructorOrMethod;
 
 /** A Runnable Method invoker. */
-public class InvokeMethodRunnable implements Callable<Void> {
+public class InvokeMethodRunnable implements Callable<Boolean> {
   private final ITestNGMethod m_method;
   private final Object m_instance;
   private final Object[] m_parameters;
@@ -27,51 +28,54 @@ public InvokeMethodRunnable(
     m_testResult = testResult;
   }
 
-  public void run() throws TestNGRuntimeException {
+  public boolean run() throws TestNGRuntimeException {
     try {
-      call();
+      return call();
     } catch (Exception e) {
       throw new TestNGRuntimeException(e);
     }
   }
 
-  private void runOne() {
+  private boolean runOne() {
+    boolean invoked;
     try {
       RuntimeException t = null;
       try {
         ConstructorOrMethod m = m_method.getConstructorOrMethod();
         if (m_hookable == null) {
+          invoked = true;
           MethodInvocationHelper.invokeMethod(m.getMethod(), m_instance, m_parameters);
         } else {
-          MethodInvocationHelper.invokeHookable(
-              m_instance, m_parameters, m_hookable, m.getMethod(), m_testResult);
+          invoked =
+              MethodInvocationHelper.invokeHookable(
+                  m_instance, m_parameters, m_hookable, m.getMethod(), m_testResult);
         }
       } catch (Throwable e) {
-        Throwable cause = e.getCause();
-        if (cause == null) {
-          cause = e;
-        }
+        invoked = true;
+        Throwable cause = Optional.ofNullable(e.getCause()).orElse(e);
         t = new TestNGRuntimeException(cause);
       }
       if (null != t) {
         Thread.currentThread().interrupt();
         throw t;
       }
+      return invoked;
     } finally {
       m_method.incrementCurrentInvocationCount();
     }
   }
 
   @Override
-  public Void call() throws Exception {
+  public Boolean call() throws Exception {
+    boolean flag = true;
     if (m_method.getInvocationTimeOut() > 0) {
       for (int i = 0; i < m_method.getInvocationCount(); i++) {
-        runOne();
+        flag = flag && runOne();
       }
     } else {
-      runOne();
+      flag = runOne();
     }
-    return null;
+    return flag;
   }
 
   public static class TestNGRuntimeException extends RuntimeException {