Pin and document what configfailurepolicy actually does

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

@@ -101,7 +101,11 @@ public class CliOptions {
   /** Parallel mode (methods, tests or classes). */
   public XmlSuite.@Nullable ParallelMode parallelMode;
 
-  /** Configuration failure policy (skip or continue). */
+  /**
+   * Whether TestNG keeps attempting configuration methods after one of them has failed once ({@code
+   * continue}) or skips the remaining ones ({@code skip}). See {@link
+   * org.testng.xml.XmlSuite.FailurePolicy}.
+   */
   public @Nullable String configFailurePolicy;
 
   /** Number of threads to use when running tests in parallel. */

testng-core-api/src/main/java/org/testng/xml/XmlSuite.java

@@ -77,9 +77,37 @@ public String toString() {
     }
   }
 
-  /** Configuration failure policy options. */
+  /**
+   * Decides what happens to the configuration methods that would have run after one of them has
+   * already failed, and hence to the test methods they were setting up.
+   *
+   * <p>What each policy does depends on the level of the configuration method that failed:
+   *
+   * <table border="1">
+   *   <caption>Fate of a test method whose setup failed</caption>
+   *   <tr><th>Failing method</th><th>{@link #SKIP}</th><th>{@link #CONTINUE}</th></tr>
+   *   <tr><td>&#64;BeforeSuite</td><td>skipped</td><td>skipped</td></tr>
+   *   <tr><td>&#64;BeforeTest</td><td>skipped</td><td><b>runs</b></td></tr>
+   *   <tr><td>&#64;BeforeClass</td><td>skipped</td><td>skipped</td></tr>
+   *   <tr><td>&#64;BeforeMethod</td><td>skipped</td><td>skipped</td></tr>
+   * </table>
+   *
+   * <p>The failure itself is always reported, so neither policy turns a broken setup into a green
+   * run. A &#64;BeforeSuite failure stops the suite whatever the policy says.
+   */
   public enum FailurePolicy {
+    /**
+     * Once a configuration method has failed, stop attempting the remaining ones for the whole
+     * class (and, for a &#64;BeforeTest failure, for every class of the same {@code <test>}).
+     */
     SKIP("skip"),
+    /**
+     * Keep attempting a configuration method that has already failed, invalidating only the
+     * narrowest scope the failure belongs to: the failing instance for &#64;BeforeClass, the
+     * failing test-method invocation for &#64;BeforeMethod. Sibling instances and sibling test
+     * methods still get their configuration methods invoked, and a &#64;BeforeTest failure
+     * invalidates no instance at all, so the test methods of that {@code <test>} run.
+     */
     CONTINUE("continue");
 
     private final String name;
@@ -284,7 +312,8 @@ public void setGuiceStage(String guiceStage) {
   }
 
   /**
-   * Sets the configuration failure policy.
+   * Sets whether TestNG keeps attempting configuration methods after one of them has failed once,
+   * or skips the remaining ones. See {@link FailurePolicy} for what each value invalidates.
    *
    * @param configFailurePolicy The config failure policy.
    */
@@ -293,7 +322,8 @@ public void setConfigFailurePolicy(FailurePolicy configFailurePolicy) {
   }
 
   /**
-   * Returns the configuration failure policy.
+   * Returns whether TestNG keeps attempting configuration methods after one of them has failed
+   * once, or skips the remaining ones. See {@link FailurePolicy} for what each value invalidates.
    *
    * @return The configuration failure policy.
    */

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

@@ -175,7 +175,7 @@
 
   private @Nullable String m_jarPath;
   /** The path of the testng.xml file inside the jar file */
   private String m_xmlPathInJar = CommandLineArgs.XML_PATH_IN_JAR_DEFAULT;
 
   private List<String> m_stringSuites = new ArrayList<>();
   private final List<Class<? extends ITestNGListener>> m_listenerClasses = new ArrayList<>();
@@ -876,7 +876,7 @@
 
   private boolean m_ignoreMissedTestNames;
 
   private Integer m_suiteThreadPoolSize = CommandLineArgs.SUITE_THREAD_POOL_SIZE_DEFAULT;
 
   private boolean m_randomizeSuites = Boolean.FALSE;
 
@@ -2058,8 +2058,8 @@
 
   /**
    * Sets the policy for whether or not to ever invoke a configuration method again after it has
-   * failed once. Possible values are defined in {@link XmlSuite}. The default value is {@link
-   * org.testng.xml.XmlSuite.FailurePolicy#SKIP}
+   * failed once. See {@link org.testng.xml.XmlSuite.FailurePolicy} for what each value invalidates.
+   * The default value is {@link org.testng.xml.XmlSuite.FailurePolicy#SKIP}
    *
    * @param failurePolicy the configuration failure policy
    */

testng-core/src/test/java/test/configurationfailurepolicy/issue2731/FailedBeforeClassSample.java

@@ -0,0 +1,15 @@
+package test.configurationfailurepolicy.issue2731;
+
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+public class FailedBeforeClassSample {
+
+  @BeforeClass
+  public void setupClassFails() {
+    throw new RuntimeException("setup class fail");
+  }
+
+  @Test
+  public void test1() {}
+}

testng-core/src/test/java/test/configurationfailurepolicy/issue2731/FailedBeforeMethodSample.java

@@ -0,0 +1,15 @@
+package test.configurationfailurepolicy.issue2731;
+
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+public class FailedBeforeMethodSample {
+
+  @BeforeMethod
+  public void setupMethodFails() {
+    throw new RuntimeException("setup method fail");
+  }
+
+  @Test
+  public void test1() {}
+}

testng-core/src/test/java/test/configurationfailurepolicy/issue2731/FailedBeforeSuiteSample.java

@@ -0,0 +1,15 @@
+package test.configurationfailurepolicy.issue2731;
+
+import org.testng.annotations.BeforeSuite;
+import org.testng.annotations.Test;
+
+public class FailedBeforeSuiteSample {
+
+  @BeforeSuite
+  public void setupSuiteFails() {
+    throw new RuntimeException("setup suite fail");
+  }
+
+  @Test
+  public void test1() {}
+}

testng-core/src/test/java/test/configurationfailurepolicy/issue2731/FailedBeforeTestSample.java

@@ -0,0 +1,15 @@
+package test.configurationfailurepolicy.issue2731;
+
+import org.testng.annotations.BeforeTest;
+import org.testng.annotations.Test;
+
+public class FailedBeforeTestSample {
+
+  @BeforeTest
+  public void setupTestFails() {
+    throw new RuntimeException("setup test fail");
+  }
+
+  @Test
+  public void test1() {}
+}

testng-core/src/test/java/test/configurationfailurepolicy/issue2731/IssueTest.java

@@ -0,0 +1,66 @@
+package test.configurationfailurepolicy.issue2731;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import org.testng.TestListenerAdapter;
+import org.testng.TestNG;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+import org.testng.xml.XmlSuite;
+import test.SimpleBaseTest;
+
+/**
+ * Pins what "configfailurepolicy" does to a test method for each level of failing configuration.
+ *
+ * <p>This is a characterization test: it records the behaviour, it does not argue that the
+ * behaviour is right. The row that GITHUB-2731 is about is &#64;BeforeTest under CONTINUE, the only
+ * combination where the test method still runs. &#64;BeforeClass and &#64;BeforeMethod skip it
+ * under either policy -- CONTINUE only narrows which instance or which invocation the failure
+ * invalidates, and these samples have a single one of each -- while &#64;BeforeSuite stops the
+ * suite regardless of the policy. Whichever way that inconsistency is eventually settled, these
+ * expectations are the ones that have to move.
+ */
+public class IssueTest extends SimpleBaseTest {
+
+  @DataProvider(name = "dp")
+  public Object[][] getData() {
+    // params - sample, policy, passed, skipped, configuration failures
+    return new Object[][] {
+      new Object[] {FailedBeforeSuiteSample.class, XmlSuite.FailurePolicy.SKIP, 0, 1, 1},
+      new Object[] {FailedBeforeSuiteSample.class, XmlSuite.FailurePolicy.CONTINUE, 0, 1, 1},
+      new Object[] {FailedBeforeTestSample.class, XmlSuite.FailurePolicy.SKIP, 0, 1, 1},
+      // GITHUB-2731: the odd one out, the test method runs despite its @BeforeTest having failed
+      new Object[] {FailedBeforeTestSample.class, XmlSuite.FailurePolicy.CONTINUE, 1, 0, 1},
+      new Object[] {FailedBeforeClassSample.class, XmlSuite.FailurePolicy.SKIP, 0, 1, 1},
+      new Object[] {FailedBeforeClassSample.class, XmlSuite.FailurePolicy.CONTINUE, 0, 1, 1},
+      new Object[] {FailedBeforeMethodSample.class, XmlSuite.FailurePolicy.SKIP, 0, 1, 1},
+      new Object[] {FailedBeforeMethodSample.class, XmlSuite.FailurePolicy.CONTINUE, 0, 1, 1},
+    };
+  }
+
+  @Test(dataProvider = "dp", description = "GITHUB-2731")
+  public void configFailureDecidesTheFateOfTheTestMethod(
+      Class<?> sample,
+      XmlSuite.FailurePolicy policy,
+      int passedTests,
+      int skippedTests,
+      int configurationFailures) {
+
+    TestListenerAdapter tla = new TestListenerAdapter();
+    TestNG testng = create(sample);
+    testng.addListener(tla);
+    testng.setConfigFailurePolicy(policy);
+    testng.run();
+
+    String context = sample.getSimpleName() + " with configfailurepolicy=" + policy;
+    assertThat(tla.getPassedTests()).describedAs("passed tests, " + context).hasSize(passedTests);
+    assertThat(tla.getSkippedTests())
+        .describedAs("skipped tests, " + context)
+        .hasSize(skippedTests);
+    assertThat(tla.getFailedTests()).describedAs("failed tests, " + context).isEmpty();
+    // The failure is always reported, so "continue" never turns a broken setup into a green run.
+    assertThat(tla.getConfigurationFailures())
+        .describedAs("configuration failures, " + context)
+        .hasSize(configurationFailures);
+  }
+}

testng-core/src/test/resources/testng.xml

@@ -802,6 +802,7 @@
   <test name="ConfigFailurePolicy">
     <classes>
       <class name="test.configurationfailurepolicy.FailurePolicyTest" />
+      <class name="test.configurationfailurepolicy.issue2731.IssueTest" />
     </classes>
   </test>