Skip to content

Pin and document what configfailurepolicy actually does - #3426

Open
juherr wants to merge 2 commits into
testng-team:masterfrom
juherr:juherr/issue-2731-review-pr-2735
Open

Pin and document what configfailurepolicy actually does#3426
juherr wants to merge 2 commits into
testng-team:masterfrom
juherr:juherr/issue-2731-review-pr-2735

Conversation

@juherr

@juherr juherr commented Aug 25, 2026

Copy link
Copy Markdown
Member

configfailurepolicy is documented three different ways, and the three do not agree.

  • The shipped DTD (testng-1.0.dtd) and TestNG.setConfigFailurePolicy say it decides whether a configuration method is attempted again after it has failed once.
  • The website says continue makes TestNG "continue to execute the remaining tests in the suite".
  • XmlSuite.FailurePolicy and its accessors said nothing at all ("Sets the configuration failure policy.").

Running it settles which is true — and the answer is "both, depending on the level":

Failing method skip continue
@BeforeSuite test skipped test skipped
@BeforeTest test skipped test runs
@BeforeClass test skipped test skipped
@BeforeMethod test skipped test skipped

The configuration failure is reported in all eight cases, so continue never turns a broken setup into a green run.

That @BeforeTest row is exactly what #2731 reports, and its title ("only works for BeforeTest") is literally accurate. @BeforeClass and @BeforeMethod do honour continue, but only by narrowing which instance or which invocation the failure invalidates — with a single instance and a single invocation there is nothing left to narrow, so the test still skips. A failed @BeforeTest is recorded against no instance at all, so it invalidates nothing.

This PR does not change any of that behaviour. It makes it visible and writes it down:

  • test(config)FailurePolicyTest covers @BeforeClass and @BeforeMethod but neither @BeforeSuite nor @BeforeTest, under either policy. That is the gap configfailurepolicy=continue only works for BeforeTest when using TestNG XML file #2731 went through. The new characterization test records the whole 4×2 matrix, so that whichever way the inconsistency is settled, the expectations that have to move are named.
  • docs(config) — give XmlSuite.FailurePolicy, both accessors, TestNG.setConfigFailurePolicy and the CLI option a description that matches the matrix instead of either half-truth. The DTD text was already right and is untouched.

A companion PR against testng-team.github.io fixes the website row that promises the behaviour TestNG does not have.

What is deliberately left open

Whether @BeforeTest + continue is the bug (too permissive, should align with @BeforeClass) or the model (and the other levels are too strict) is a design call, not a documentation one — it is the question @krmahadevan raised on #2731 in 2022 and it is still unanswered. Either answer breaks somebody. #2731 should stay open until it is decided; this PR just makes deciding it cheap.

Did you remember to?

  • Add test case(s)
  • Update CHANGES.txt — no behaviour change, nothing user-visible to announce
  • Auto applied styling via ./gradlew autostyleApply

Summary by CodeRabbit

  • Documentation

    • Clarified configFailurePolicy behavior for SKIP and CONTINUE.
    • Documented how configuration failures affect tests at different setup levels.
    • Clarified the default policy and configuration retry behavior.
  • Tests

    • Added coverage for configuration failures during suite, test, class, and method setup.
    • Verified whether affected tests pass or skip under each failure policy.

juherr added 2 commits August 25, 2026 21:56
`FailurePolicyTest` covers @BeforeClass and @BeforeMethod, but neither
@BeforeSuite nor @BeforeTest, under either policy. That is the gap
GITHUB-2731 reports through: with `continue`, a failed @BeforeTest is
the one and only case where the test method still runs.

Record the whole 4x2 matrix so the inconsistency is visible in the test
suite rather than only in an issue, and so that whichever way it is
eventually settled, the expectations that have to move are named.

The samples are top-level classes rather than nested ones on purpose:
Gradle's TestNG runner collects the nested classes of a filtered class,
and a @BeforeSuite that fails would then poison the whole run.

Refs testng-team#2731
`XmlSuite.FailurePolicy` and its accessors carried no semantics at all
("Sets the configuration failure policy."), and the website documents
`continue` as "continue to execute the remaining tests in the suite" --
which only @BeforeTest actually does.

Write down the matrix the preceding commit pins, rather than either of
the two half-truths: `continue` narrows a config failure to the failing
instance for @BeforeClass and to the failing invocation for
@BeforeMethod, a failed @BeforeTest invalidates no instance at all so
those test methods run, and a @BeforeSuite failure stops the suite under
either policy. The failure is reported in every case, so `continue`
never turns a broken setup into a green run.

Refs testng-team#2731
@coderabbitai

coderabbitai Bot commented Aug 25, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: a7fbda3c-81db-43ab-b2b8-a0f241af377e

📥 Commits

Reviewing files that changed from the base of the PR and between 5b0746b and de5e38c.

📒 Files selected for processing (9)
  • testng-cli/src/main/java/org/testng/cli/CliOptions.java
  • testng-core-api/src/main/java/org/testng/xml/XmlSuite.java
  • testng-core/src/main/java/org/testng/TestNG.java
  • testng-core/src/test/java/test/configurationfailurepolicy/issue2731/FailedBeforeClassSample.java
  • testng-core/src/test/java/test/configurationfailurepolicy/issue2731/FailedBeforeMethodSample.java
  • testng-core/src/test/java/test/configurationfailurepolicy/issue2731/FailedBeforeSuiteSample.java
  • testng-core/src/test/java/test/configurationfailurepolicy/issue2731/FailedBeforeTestSample.java
  • testng-core/src/test/java/test/configurationfailurepolicy/issue2731/IssueTest.java
  • testng-core/src/test/resources/testng.xml

Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review.


📝 Walkthrough

Walkthrough

The change expands configuration failure policy documentation and adds characterization tests for SKIP and CONTINUE across suite, test, class, and method configuration levels.

Changes

Configuration failure policy

Layer / File(s) Summary
Policy documentation
testng-cli/src/main/java/org/testng/cli/CliOptions.java, testng-core-api/src/main/java/org/testng/xml/XmlSuite.java, testng-core/src/main/java/org/testng/TestNG.java
Javadocs describe SKIP and CONTINUE behavior, invalidation scope, and the default SKIP policy.
Policy characterization tests
testng-core/src/test/java/test/configurationfailurepolicy/issue2731/*, testng-core/src/test/resources/testng.xml
Added failing configuration fixtures and assertions for test results under both failure policies.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to de5e3

This PR documents existing configuration-failure behavior and adds characterization coverage without changing runtime behavior; no actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers: krmahadevan

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 7.69% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 8 files. (1 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main changes: it documents the existing configfailurepolicy behavior and adds tests to pin that behavior.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 7.69% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 8 files. (1 skipped: 1 unsupported.)

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant