refactor(core): extract the command line front end out of testng-core - #3307
Open
juherr wants to merge 1 commit into
Open
refactor(core): extract the command line front end out of testng-core#3307juherr wants to merge 1 commit into
juherr wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe CLI front end is split from ChangesCLI modularization
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
testng/testng-build.gradle.kts (1)
67-92: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winExport
org.testng.cli.jcommanderin the merged TestNG bundle.
TestNG#main/TestNG#privateMaindocument usingorg.testng.cli.jcommander.JCommanderCliRunneras the replacement for the deprecated command-line entry points, and this package is currently imported by the bundled JCommander frontend. Add it toExport-Packageso OSGi consumers can resolve the documented runner class without SPI-Fly provider-only access.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@testng/testng-build.gradle.kts` around lines 67 - 92, Add org.testng.cli.jcommander to the Export-Package list in the merged TestNG bundle configuration. Keep the existing package exports unchanged so OSGi consumers can resolve TestNG#main/privateMain’s documented JCommanderCliRunner.
🧹 Nitpick comments (2)
testng-cli/src/test/java/org/testng/cli/CliConfigurerParityTest.java (1)
54-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winParity test skips every class-loading field.
None of
listener,listenerFactory,listenerComparator,objectFactory,testRunnerFactory,threadPoolFactoryClass,dependencyInjectorFactoryClass,methodSelectors, orreporterare populated. These are precisely the fields whose conversion logic changed the most (raw casts +m_objectFactory.newInstancechains →asSubclass/uncheckedSubclass), so the parity test currently can't catch a regression in that logic.Consider adding concrete (test-fixture) class names for these fields so the class-loading/casting paths are actually exercised by the parity comparison.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@testng-cli/src/test/java/org/testng/cli/CliConfigurerParityTest.java` around lines 54 - 85, Extend populatedCliOptions() to assign concrete test-fixture class names to listener, listenerFactory, listenerComparator, objectFactory, testRunnerFactory, threadPoolFactoryClass, dependencyInjectorFactoryClass, methodSelectors, and reporter. Use compatible fixture implementations for each field so CliOptions conversion and parity comparison exercise the updated asSubclass/uncheckedSubclass class-loading paths.testng-jcommander/src/test/java/test/groups/issue2232/IssueCommandLineTest.java (1)
31-51: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a timeout guard around the forked-process wait.
process.waitFor()has no bound; if the forkedTestNGprocess hangs, this test (and the build) stalls indefinitely instead of failing.♻️ Proposed fix
- Process process = builder.inheritIO().start(); - process.waitFor(); - - return process.exitValue(); + Process process = builder.inheritIO().start(); + boolean finished = process.waitFor(60, TimeUnit.SECONDS); + if (!finished) { + process.destroyForcibly(); + throw new AssertionError("Forked TestNG process did not terminate within the timeout"); + } + + return process.exitValue();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@testng-jcommander/src/test/java/test/groups/issue2232/IssueCommandLineTest.java` around lines 31 - 51, Update the exec method’s forked-process wait to use a bounded timeout instead of unconditionally calling process.waitFor(). Fail the test when the timeout expires, and preserve returning the completed process’s exit value for normal termination.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGES.txt`:
- Around line 5-8: Update the deprecation entry in CHANGES.txt to state that the
listed APIs remain available but with the behavior changes described in the
subsequent entries, rather than claiming they work unchanged. Keep the existing
removal timeline and API list intact.
In `@testng-bom/testng-bom-build.gradle.kts`:
- Around line 11-15: Remove the projects.testngCli and projects.testngJcommander
entries from the BOM dependencies, while retaining the published
testngCollections, testngCoreApi, and testngCore modules. Do not expose the
deferred unpublished coordinates until issue `#3301` is resolved.
In `@testng-core/src/main/java/org/testng/CliRunners.java`:
- Around line 77-97: Update the load method’s duplicate-provider check after the
first successful it.next() so its subsequent it.hasNext() runs in a separate
guarded block; if that peek throws ServiceConfigurationError or
RuntimeException, ignore it and still return the already-loaded runner. Keep the
existing failure handling for initial ServiceLoader iteration and runner
loading, while preserving the warning when another provider is detected.
In `@testng-jcommander/src/test/java/test/methodselectors/CommandLineTest.java`:
- Around line 48-49: Replace every failed-result assertion in
CommandLineTest.java at lines 48-49, 58-59, 69-70, 80-81, 91-92, 103-104,
113-114, 124-125, 147-148, 167-168, and 188-190: calls passing
tla.getFailedTests() must use assertFailedTestNames instead of
assertPassedTestNames, while leaving passed-result assertions unchanged.
In
`@testng-jcommander/src/test/java/test/reports/EmailableReporterCommandLineTest.java`:
- Around line 50-61: Update the JVM property handling around TestNG.privateMain:
capture the original value before System.setProperty, then restore that value in
the finally block, calling System.clearProperty when the property was previously
absent. Preserve the existing jvm != null guard and test execution flow.
In
`@testng-jcommander/src/test/java/test/thread/CustomExecutorServiceFactoryCommandLineTest.java`:
- Around line 57-62: Update the generated-suite setup in
CustomExecutorServiceFactoryCommandLineTest so an IOException from
Files.writeString is propagated or causes the test to fail, rather than being
ignored. Preserve adding successfully written suite paths to suites, and ensure
write failures cannot silently reduce the test to a single-suite run.
In
`@testng-test-kit/src/main/java/test/commandline/issue341/LocalLogAggregator.java`:
- Around line 12-21: Update LocalLogAggregator’s static log state so each
TestNG.privateMain run starts with cleared logs, using an explicit reset or
equivalent per-run lifecycle hook. Change getLogs() to return a snapshot copy
rather than the live backing set, while preserving concurrent collection during
afterInvocation.
In
`@testng-test-kit/src/main/java/test/groups/issue2232/samples/SampleTest2.java`:
- Around line 13-19: Update the setUp method’s failure condition so it is
reachable during the single `@BeforeClass` invocation, such as triggering when
variable reaches 1; preserve the exception and existing setup behavior while
ensuring the failure-policy test exercises the configuration failure.
In `@testng-test-kit/src/main/java/test/TestHelper.java`:
- Around line 35-37: Update the temporary-resource helpers in TestHelper,
including the XML file creation and related directory creation paths, to
establish reliable cleanup: either expose the generated resources so each caller
explicitly deletes them during teardown or register guaranteed cleanup for every
created file and directory. Ensure cleanup also runs when the test or CLI
invocation fails.
In `@testng-test-kit/src/main/kotlin/test/SimpleBaseTest.kt`:
- Around line 341-345: Update the File-based grep overload to manage and close
the FileReader it creates before returning, while preserving delegation to the
existing grep(Reader, String, MutableList<String>) implementation and its
results.
---
Outside diff comments:
In `@testng/testng-build.gradle.kts`:
- Around line 67-92: Add org.testng.cli.jcommander to the Export-Package list in
the merged TestNG bundle configuration. Keep the existing package exports
unchanged so OSGi consumers can resolve TestNG#main/privateMain’s documented
JCommanderCliRunner.
---
Nitpick comments:
In `@testng-cli/src/test/java/org/testng/cli/CliConfigurerParityTest.java`:
- Around line 54-85: Extend populatedCliOptions() to assign concrete
test-fixture class names to listener, listenerFactory, listenerComparator,
objectFactory, testRunnerFactory, threadPoolFactoryClass,
dependencyInjectorFactoryClass, methodSelectors, and reporter. Use compatible
fixture implementations for each field so CliOptions conversion and parity
comparison exercise the updated asSubclass/uncheckedSubclass class-loading
paths.
In
`@testng-jcommander/src/test/java/test/groups/issue2232/IssueCommandLineTest.java`:
- Around line 31-51: Update the exec method’s forked-process wait to use a
bounded timeout instead of unconditionally calling process.waitFor(). Fail the
test when the timeout expires, and preserve returning the completed process’s
exit value for normal termination.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6bfc25c1-215f-4ea8-9e3c-8274509560d1
📒 Files selected for processing (97)
CHANGES.txtsettings.gradle.ktstestng-asserts/src/test/java/testhelper/PerformanceUtils.javatestng-asserts/testng-asserts-build.gradle.ktstestng-bom/testng-bom-build.gradle.ktstestng-cli/src/main/java/org/testng/cli/AbstractCliRunner.javatestng-cli/src/main/java/org/testng/cli/CliConfigurer.javatestng-cli/src/main/java/org/testng/cli/CliOptions.javatestng-cli/src/main/java/org/testng/cli/CliParseException.javatestng-cli/src/test/java/org/testng/cli/CliConfigurerParityTest.javatestng-cli/src/test/java/org/testng/cli/CliConfigurerValidateTest.javatestng-cli/src/test/java/org/testng/cli/CliOptionNamesTest.javatestng-cli/testng-cli-build.gradle.ktstestng-core/src/main/java/org/testng/CliRunners.javatestng-core/src/main/java/org/testng/CommandLineArgs.javatestng-core/src/main/java/org/testng/ITestNGCliRunner.javatestng-core/src/main/java/org/testng/TestNG.javatestng-core/src/test/java/test/TestHelper.javatestng-core/src/test/java/test/commandline/CommandLineOverridesXml.javatestng-core/src/test/java/test/configurationfailurepolicy/FailurePolicyTest.javatestng-core/src/test/java/test/github1417/YetAnotherTestClassSample.javatestng-core/src/test/java/test/groups/issue2232/IssueTest.javatestng-core/src/test/java/test/junitreports/JUnitReportsTest.javatestng-core/src/test/java/test/listeners/ListenersTest.javatestng-core/src/test/java/test/listeners/factory/TestNGFactoryTest.javatestng-core/src/test/java/test/methodselectors/MethodSelectorInSuiteTest.javatestng-core/src/test/java/test/reports/EmailableReporterTest.javatestng-core/src/test/java/test/testng1231/TestExecutionListenerInvocationOrder.javatestng-core/src/test/java/test/thread/CustomExecutorServiceFactoryTest.javatestng-core/src/test/resources/testng.xmltestng-core/testng-core-build.gradle.ktstestng-jcommander/src/main/java/org/testng/cli/jcommander/Converter.javatestng-jcommander/src/main/java/org/testng/cli/jcommander/JCommanderCliRunner.javatestng-jcommander/src/main/java/org/testng/cli/jcommander/JCommanderOptions.javatestng-jcommander/src/main/resources/META-INF/services/org.testng.ITestNGCliRunnertestng-jcommander/src/test/java/org/testng/cli/jcommander/JCommanderCliRunnerTest.javatestng-jcommander/src/test/java/test/commandline/CommandLineOverridesXmlCommandLineTest.javatestng-jcommander/src/test/java/test/configurationfailurepolicy/FailurePolicyCommandLineTest.javatestng-jcommander/src/test/java/test/groups/issue2232/IssueCommandLineTest.javatestng-jcommander/src/test/java/test/listeners/ListenerWiringCommandLineTest.javatestng-jcommander/src/test/java/test/listeners/factory/TestNGFactoryCommandLineTest.javatestng-jcommander/src/test/java/test/methodselectors/CommandLineTest.javatestng-jcommander/src/test/java/test/methodselectors/MethodSelectorInSuiteCommandLineTest.javatestng-jcommander/src/test/java/test/methodselectors/NoTest1MethodSelector.javatestng-jcommander/src/test/java/test/reports/EmailableReporterCommandLineTest.javatestng-jcommander/src/test/java/test/thread/CustomExecutorServiceFactoryCommandLineTest.javatestng-jcommander/src/test/resources/1332.xmltestng-jcommander/src/test/resources/methodselector-in-xml.xmltestng-jcommander/src/test/resources/test/methodselectors/sampleTest.xmltestng-jcommander/src/test/resources/test/methodselectors/sampleTestExclusions.xmltestng-jcommander/src/test/resources/testnames/main-suite.xmltestng-jcommander/src/test/resources/testng-configfailure.xmltestng-jcommander/src/test/resources/testng-methodselectors.xmltestng-jcommander/testng-jcommander-build.gradle.ktstestng-test-kit/src/main/java/org/testng/jarfileutils/org/testng/SampleTest1.javatestng-test-kit/src/main/java/org/testng/jarfileutils/org/testng/SampleTest2.javatestng-test-kit/src/main/java/org/testng/jarfileutils/org/testng/SampleTest3.javatestng-test-kit/src/main/java/org/testng/jarfileutils/org/testng/SampleTest4.javatestng-test-kit/src/main/java/org/testng/jarfileutils/org/testng/SampleTest5.javatestng-test-kit/src/main/java/org/testng/testhelper/JarCreator.javatestng-test-kit/src/main/java/org/testng/testhelper/OutputDirectoryPatch.javatestng-test-kit/src/main/java/test/InvokedMethodNameListener.javatestng-test-kit/src/main/java/test/TestHelper.javatestng-test-kit/src/main/java/test/commandline/issue341/LocalLogAggregator.javatestng-test-kit/src/main/java/test/commandline/issue341/TestSampleA.javatestng-test-kit/src/main/java/test/commandline/issue341/TestSampleB.javatestng-test-kit/src/main/java/test/configurationfailurepolicy/ClassWithFailedBeforeMethodAndMultipleTests.javatestng-test-kit/src/main/java/test/groups/issue2232/Issue2232Suites.javatestng-test-kit/src/main/java/test/groups/issue2232/samples/SampleTest.javatestng-test-kit/src/main/java/test/groups/issue2232/samples/SampleTest2.javatestng-test-kit/src/main/java/test/listeners/cliwiring/FirstWiringListener.javatestng-test-kit/src/main/java/test/listeners/cliwiring/ReverseNameListenerComparator.javatestng-test-kit/src/main/java/test/listeners/cliwiring/SecondWiringListener.javatestng-test-kit/src/main/java/test/listeners/cliwiring/WiringLog.javatestng-test-kit/src/main/java/test/listeners/cliwiring/WiringSampleTest.javatestng-test-kit/src/main/java/test/listeners/factory/ExampleListener.javatestng-test-kit/src/main/java/test/listeners/factory/SampleTestCase.javatestng-test-kit/src/main/java/test/listeners/factory/SampleTestFactory.javatestng-test-kit/src/main/java/test/methodselectors/AllTestsMethodSelector.javatestng-test-kit/src/main/java/test/methodselectors/NoTest.javatestng-test-kit/src/main/java/test/methodselectors/NoTestSelector.javatestng-test-kit/src/main/java/test/methodselectors/SampleTest.javatestng-test-kit/src/main/java/test/methodselectors/Test2MethodSelector.javatestng-test-kit/src/main/java/test/reports/ReporterSample.javatestng-test-kit/src/main/java/test/testnames/TestNamesFeature.javatestng-test-kit/src/main/java/test/thread/issue3066/Issue3066ExecutorServiceFactory.javatestng-test-kit/src/main/java/test/thread/issue3066/Issue3066ThreadPoolExecutor.javatestng-test-kit/src/main/java/test/thread/issue3066/TestClassSample.javatestng-test-kit/src/main/kotlin/test/SimpleBaseTest.kttestng-test-kit/src/main/resources/jarfileutils/child.xmltestng-test-kit/src/main/resources/jarfileutils/child/child.xmltestng-test-kit/src/main/resources/jarfileutils/child/childofchild/childofchild.xmltestng-test-kit/src/main/resources/jarfileutils/childofchild/childofchild.xmltestng-test-kit/src/main/resources/jarfileutils/testng-tests.xmltestng-test-kit/testng-test-kit-build.gradle.ktstestng-test-osgi/src/test/java/org/testng/test/osgi/PlainOsgiTest.javatestng/testng-build.gradle.kts
💤 Files with no reviewable changes (8)
- testng-asserts/testng-asserts-build.gradle.kts
- testng-core/src/test/java/test/TestHelper.java
- testng-core/src/test/resources/testng.xml
- testng-core/src/test/java/test/commandline/CommandLineOverridesXml.java
- testng-core/src/test/java/test/listeners/factory/TestNGFactoryTest.java
- testng-core/src/test/java/test/thread/CustomExecutorServiceFactoryTest.java
- testng-core/src/test/java/test/reports/EmailableReporterTest.java
- testng-core/src/test/java/test/listeners/ListenersTest.java
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
testng/testng-build.gradle.kts (1)
67-92: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winExport
org.testng.cli.jcommanderin the merged TestNG bundle.
TestNG#main/TestNG#privateMaindocument usingorg.testng.cli.jcommander.JCommanderCliRunneras the replacement for the deprecated command-line entry points, and this package is currently imported by the bundled JCommander frontend. Add it toExport-Packageso OSGi consumers can resolve the documented runner class without SPI-Fly provider-only access.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@testng/testng-build.gradle.kts` around lines 67 - 92, Add org.testng.cli.jcommander to the Export-Package list in the merged TestNG bundle configuration. Keep the existing package exports unchanged so OSGi consumers can resolve TestNG#main/privateMain’s documented JCommanderCliRunner.
🧹 Nitpick comments (2)
testng-cli/src/test/java/org/testng/cli/CliConfigurerParityTest.java (1)
54-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winParity test skips every class-loading field.
None of
listener,listenerFactory,listenerComparator,objectFactory,testRunnerFactory,threadPoolFactoryClass,dependencyInjectorFactoryClass,methodSelectors, orreporterare populated. These are precisely the fields whose conversion logic changed the most (raw casts +m_objectFactory.newInstancechains →asSubclass/uncheckedSubclass), so the parity test currently can't catch a regression in that logic.Consider adding concrete (test-fixture) class names for these fields so the class-loading/casting paths are actually exercised by the parity comparison.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@testng-cli/src/test/java/org/testng/cli/CliConfigurerParityTest.java` around lines 54 - 85, Extend populatedCliOptions() to assign concrete test-fixture class names to listener, listenerFactory, listenerComparator, objectFactory, testRunnerFactory, threadPoolFactoryClass, dependencyInjectorFactoryClass, methodSelectors, and reporter. Use compatible fixture implementations for each field so CliOptions conversion and parity comparison exercise the updated asSubclass/uncheckedSubclass class-loading paths.testng-jcommander/src/test/java/test/groups/issue2232/IssueCommandLineTest.java (1)
31-51: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a timeout guard around the forked-process wait.
process.waitFor()has no bound; if the forkedTestNGprocess hangs, this test (and the build) stalls indefinitely instead of failing.♻️ Proposed fix
- Process process = builder.inheritIO().start(); - process.waitFor(); - - return process.exitValue(); + Process process = builder.inheritIO().start(); + boolean finished = process.waitFor(60, TimeUnit.SECONDS); + if (!finished) { + process.destroyForcibly(); + throw new AssertionError("Forked TestNG process did not terminate within the timeout"); + } + + return process.exitValue();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@testng-jcommander/src/test/java/test/groups/issue2232/IssueCommandLineTest.java` around lines 31 - 51, Update the exec method’s forked-process wait to use a bounded timeout instead of unconditionally calling process.waitFor(). Fail the test when the timeout expires, and preserve returning the completed process’s exit value for normal termination.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGES.txt`:
- Around line 5-8: Update the deprecation entry in CHANGES.txt to state that the
listed APIs remain available but with the behavior changes described in the
subsequent entries, rather than claiming they work unchanged. Keep the existing
removal timeline and API list intact.
In `@testng-bom/testng-bom-build.gradle.kts`:
- Around line 11-15: Remove the projects.testngCli and projects.testngJcommander
entries from the BOM dependencies, while retaining the published
testngCollections, testngCoreApi, and testngCore modules. Do not expose the
deferred unpublished coordinates until issue `#3301` is resolved.
In `@testng-core/src/main/java/org/testng/CliRunners.java`:
- Around line 77-97: Update the load method’s duplicate-provider check after the
first successful it.next() so its subsequent it.hasNext() runs in a separate
guarded block; if that peek throws ServiceConfigurationError or
RuntimeException, ignore it and still return the already-loaded runner. Keep the
existing failure handling for initial ServiceLoader iteration and runner
loading, while preserving the warning when another provider is detected.
In `@testng-jcommander/src/test/java/test/methodselectors/CommandLineTest.java`:
- Around line 48-49: Replace every failed-result assertion in
CommandLineTest.java at lines 48-49, 58-59, 69-70, 80-81, 91-92, 103-104,
113-114, 124-125, 147-148, 167-168, and 188-190: calls passing
tla.getFailedTests() must use assertFailedTestNames instead of
assertPassedTestNames, while leaving passed-result assertions unchanged.
In
`@testng-jcommander/src/test/java/test/reports/EmailableReporterCommandLineTest.java`:
- Around line 50-61: Update the JVM property handling around TestNG.privateMain:
capture the original value before System.setProperty, then restore that value in
the finally block, calling System.clearProperty when the property was previously
absent. Preserve the existing jvm != null guard and test execution flow.
In
`@testng-jcommander/src/test/java/test/thread/CustomExecutorServiceFactoryCommandLineTest.java`:
- Around line 57-62: Update the generated-suite setup in
CustomExecutorServiceFactoryCommandLineTest so an IOException from
Files.writeString is propagated or causes the test to fail, rather than being
ignored. Preserve adding successfully written suite paths to suites, and ensure
write failures cannot silently reduce the test to a single-suite run.
In
`@testng-test-kit/src/main/java/test/commandline/issue341/LocalLogAggregator.java`:
- Around line 12-21: Update LocalLogAggregator’s static log state so each
TestNG.privateMain run starts with cleared logs, using an explicit reset or
equivalent per-run lifecycle hook. Change getLogs() to return a snapshot copy
rather than the live backing set, while preserving concurrent collection during
afterInvocation.
In
`@testng-test-kit/src/main/java/test/groups/issue2232/samples/SampleTest2.java`:
- Around line 13-19: Update the setUp method’s failure condition so it is
reachable during the single `@BeforeClass` invocation, such as triggering when
variable reaches 1; preserve the exception and existing setup behavior while
ensuring the failure-policy test exercises the configuration failure.
In `@testng-test-kit/src/main/java/test/TestHelper.java`:
- Around line 35-37: Update the temporary-resource helpers in TestHelper,
including the XML file creation and related directory creation paths, to
establish reliable cleanup: either expose the generated resources so each caller
explicitly deletes them during teardown or register guaranteed cleanup for every
created file and directory. Ensure cleanup also runs when the test or CLI
invocation fails.
In `@testng-test-kit/src/main/kotlin/test/SimpleBaseTest.kt`:
- Around line 341-345: Update the File-based grep overload to manage and close
the FileReader it creates before returning, while preserving delegation to the
existing grep(Reader, String, MutableList<String>) implementation and its
results.
---
Outside diff comments:
In `@testng/testng-build.gradle.kts`:
- Around line 67-92: Add org.testng.cli.jcommander to the Export-Package list in
the merged TestNG bundle configuration. Keep the existing package exports
unchanged so OSGi consumers can resolve TestNG#main/privateMain’s documented
JCommanderCliRunner.
---
Nitpick comments:
In `@testng-cli/src/test/java/org/testng/cli/CliConfigurerParityTest.java`:
- Around line 54-85: Extend populatedCliOptions() to assign concrete
test-fixture class names to listener, listenerFactory, listenerComparator,
objectFactory, testRunnerFactory, threadPoolFactoryClass,
dependencyInjectorFactoryClass, methodSelectors, and reporter. Use compatible
fixture implementations for each field so CliOptions conversion and parity
comparison exercise the updated asSubclass/uncheckedSubclass class-loading
paths.
In
`@testng-jcommander/src/test/java/test/groups/issue2232/IssueCommandLineTest.java`:
- Around line 31-51: Update the exec method’s forked-process wait to use a
bounded timeout instead of unconditionally calling process.waitFor(). Fail the
test when the timeout expires, and preserve returning the completed process’s
exit value for normal termination.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6bfc25c1-215f-4ea8-9e3c-8274509560d1
📒 Files selected for processing (97)
CHANGES.txtsettings.gradle.ktstestng-asserts/src/test/java/testhelper/PerformanceUtils.javatestng-asserts/testng-asserts-build.gradle.ktstestng-bom/testng-bom-build.gradle.ktstestng-cli/src/main/java/org/testng/cli/AbstractCliRunner.javatestng-cli/src/main/java/org/testng/cli/CliConfigurer.javatestng-cli/src/main/java/org/testng/cli/CliOptions.javatestng-cli/src/main/java/org/testng/cli/CliParseException.javatestng-cli/src/test/java/org/testng/cli/CliConfigurerParityTest.javatestng-cli/src/test/java/org/testng/cli/CliConfigurerValidateTest.javatestng-cli/src/test/java/org/testng/cli/CliOptionNamesTest.javatestng-cli/testng-cli-build.gradle.ktstestng-core/src/main/java/org/testng/CliRunners.javatestng-core/src/main/java/org/testng/CommandLineArgs.javatestng-core/src/main/java/org/testng/ITestNGCliRunner.javatestng-core/src/main/java/org/testng/TestNG.javatestng-core/src/test/java/test/TestHelper.javatestng-core/src/test/java/test/commandline/CommandLineOverridesXml.javatestng-core/src/test/java/test/configurationfailurepolicy/FailurePolicyTest.javatestng-core/src/test/java/test/github1417/YetAnotherTestClassSample.javatestng-core/src/test/java/test/groups/issue2232/IssueTest.javatestng-core/src/test/java/test/junitreports/JUnitReportsTest.javatestng-core/src/test/java/test/listeners/ListenersTest.javatestng-core/src/test/java/test/listeners/factory/TestNGFactoryTest.javatestng-core/src/test/java/test/methodselectors/MethodSelectorInSuiteTest.javatestng-core/src/test/java/test/reports/EmailableReporterTest.javatestng-core/src/test/java/test/testng1231/TestExecutionListenerInvocationOrder.javatestng-core/src/test/java/test/thread/CustomExecutorServiceFactoryTest.javatestng-core/src/test/resources/testng.xmltestng-core/testng-core-build.gradle.ktstestng-jcommander/src/main/java/org/testng/cli/jcommander/Converter.javatestng-jcommander/src/main/java/org/testng/cli/jcommander/JCommanderCliRunner.javatestng-jcommander/src/main/java/org/testng/cli/jcommander/JCommanderOptions.javatestng-jcommander/src/main/resources/META-INF/services/org.testng.ITestNGCliRunnertestng-jcommander/src/test/java/org/testng/cli/jcommander/JCommanderCliRunnerTest.javatestng-jcommander/src/test/java/test/commandline/CommandLineOverridesXmlCommandLineTest.javatestng-jcommander/src/test/java/test/configurationfailurepolicy/FailurePolicyCommandLineTest.javatestng-jcommander/src/test/java/test/groups/issue2232/IssueCommandLineTest.javatestng-jcommander/src/test/java/test/listeners/ListenerWiringCommandLineTest.javatestng-jcommander/src/test/java/test/listeners/factory/TestNGFactoryCommandLineTest.javatestng-jcommander/src/test/java/test/methodselectors/CommandLineTest.javatestng-jcommander/src/test/java/test/methodselectors/MethodSelectorInSuiteCommandLineTest.javatestng-jcommander/src/test/java/test/methodselectors/NoTest1MethodSelector.javatestng-jcommander/src/test/java/test/reports/EmailableReporterCommandLineTest.javatestng-jcommander/src/test/java/test/thread/CustomExecutorServiceFactoryCommandLineTest.javatestng-jcommander/src/test/resources/1332.xmltestng-jcommander/src/test/resources/methodselector-in-xml.xmltestng-jcommander/src/test/resources/test/methodselectors/sampleTest.xmltestng-jcommander/src/test/resources/test/methodselectors/sampleTestExclusions.xmltestng-jcommander/src/test/resources/testnames/main-suite.xmltestng-jcommander/src/test/resources/testng-configfailure.xmltestng-jcommander/src/test/resources/testng-methodselectors.xmltestng-jcommander/testng-jcommander-build.gradle.ktstestng-test-kit/src/main/java/org/testng/jarfileutils/org/testng/SampleTest1.javatestng-test-kit/src/main/java/org/testng/jarfileutils/org/testng/SampleTest2.javatestng-test-kit/src/main/java/org/testng/jarfileutils/org/testng/SampleTest3.javatestng-test-kit/src/main/java/org/testng/jarfileutils/org/testng/SampleTest4.javatestng-test-kit/src/main/java/org/testng/jarfileutils/org/testng/SampleTest5.javatestng-test-kit/src/main/java/org/testng/testhelper/JarCreator.javatestng-test-kit/src/main/java/org/testng/testhelper/OutputDirectoryPatch.javatestng-test-kit/src/main/java/test/InvokedMethodNameListener.javatestng-test-kit/src/main/java/test/TestHelper.javatestng-test-kit/src/main/java/test/commandline/issue341/LocalLogAggregator.javatestng-test-kit/src/main/java/test/commandline/issue341/TestSampleA.javatestng-test-kit/src/main/java/test/commandline/issue341/TestSampleB.javatestng-test-kit/src/main/java/test/configurationfailurepolicy/ClassWithFailedBeforeMethodAndMultipleTests.javatestng-test-kit/src/main/java/test/groups/issue2232/Issue2232Suites.javatestng-test-kit/src/main/java/test/groups/issue2232/samples/SampleTest.javatestng-test-kit/src/main/java/test/groups/issue2232/samples/SampleTest2.javatestng-test-kit/src/main/java/test/listeners/cliwiring/FirstWiringListener.javatestng-test-kit/src/main/java/test/listeners/cliwiring/ReverseNameListenerComparator.javatestng-test-kit/src/main/java/test/listeners/cliwiring/SecondWiringListener.javatestng-test-kit/src/main/java/test/listeners/cliwiring/WiringLog.javatestng-test-kit/src/main/java/test/listeners/cliwiring/WiringSampleTest.javatestng-test-kit/src/main/java/test/listeners/factory/ExampleListener.javatestng-test-kit/src/main/java/test/listeners/factory/SampleTestCase.javatestng-test-kit/src/main/java/test/listeners/factory/SampleTestFactory.javatestng-test-kit/src/main/java/test/methodselectors/AllTestsMethodSelector.javatestng-test-kit/src/main/java/test/methodselectors/NoTest.javatestng-test-kit/src/main/java/test/methodselectors/NoTestSelector.javatestng-test-kit/src/main/java/test/methodselectors/SampleTest.javatestng-test-kit/src/main/java/test/methodselectors/Test2MethodSelector.javatestng-test-kit/src/main/java/test/reports/ReporterSample.javatestng-test-kit/src/main/java/test/testnames/TestNamesFeature.javatestng-test-kit/src/main/java/test/thread/issue3066/Issue3066ExecutorServiceFactory.javatestng-test-kit/src/main/java/test/thread/issue3066/Issue3066ThreadPoolExecutor.javatestng-test-kit/src/main/java/test/thread/issue3066/TestClassSample.javatestng-test-kit/src/main/kotlin/test/SimpleBaseTest.kttestng-test-kit/src/main/resources/jarfileutils/child.xmltestng-test-kit/src/main/resources/jarfileutils/child/child.xmltestng-test-kit/src/main/resources/jarfileutils/child/childofchild/childofchild.xmltestng-test-kit/src/main/resources/jarfileutils/childofchild/childofchild.xmltestng-test-kit/src/main/resources/jarfileutils/testng-tests.xmltestng-test-kit/testng-test-kit-build.gradle.ktstestng-test-osgi/src/test/java/org/testng/test/osgi/PlainOsgiTest.javatestng/testng-build.gradle.kts
💤 Files with no reviewable changes (8)
- testng-asserts/testng-asserts-build.gradle.kts
- testng-core/src/test/java/test/TestHelper.java
- testng-core/src/test/resources/testng.xml
- testng-core/src/test/java/test/commandline/CommandLineOverridesXml.java
- testng-core/src/test/java/test/listeners/factory/TestNGFactoryTest.java
- testng-core/src/test/java/test/thread/CustomExecutorServiceFactoryTest.java
- testng-core/src/test/java/test/reports/EmailableReporterTest.java
- testng-core/src/test/java/test/listeners/ListenersTest.java
🛑 Comments failed to post (3)
testng-test-kit/src/main/java/test/commandline/issue341/LocalLogAggregator.java (1)
12-21: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Reset the static log set between CLI runs.
Because
logsis static andgetLogs()exposes the live set, laterTestNG.privateMaininvocations can observe prior results. Add per-run state or an explicit reset, and return a snapshot rather than the mutable backing set.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@testng-test-kit/src/main/java/test/commandline/issue341/LocalLogAggregator.java` around lines 12 - 21, Update LocalLogAggregator’s static log state so each TestNG.privateMain run starts with cleared logs, using an explicit reset or equivalent per-run lifecycle hook. Change getLogs() to return a snapshot copy rather than the live backing set, while preserving concurrent collection during afterInvocation.testng-test-kit/src/main/java/test/groups/issue2232/samples/SampleTest2.java (1)
13-19: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the failing setup reachable.
@BeforeClassruns once for this class, sovariablechanges from0to1; thevariable == 4branch can never execute during setup. The failure-policy integration test can therefore pass without exercising a configuration failure. Align the trigger with the lifecycle, such as failing on the first@BeforeClassinvocation or moving the condition to a per-test configuration method.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@testng-test-kit/src/main/java/test/groups/issue2232/samples/SampleTest2.java` around lines 13 - 19, Update the setUp method’s failure condition so it is reachable during the single `@BeforeClass` invocation, such as triggering when variable reaches 1; preserve the exception and existing setup behavior while ensuring the failure-policy test exercises the configuration failure.testng-test-kit/src/main/kotlin/test/SimpleBaseTest.kt (1)
341-345: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Close the
FileReaderowned by this overload.The delegated reader is never closed, so repeated file scans leak descriptors.
Proposed fix
protected fun grep( fileName: File, regexp: String, resultLines: MutableList<String> -) = grep(FileReader(fileName), regexp, resultLines) +) = FileReader(fileName).use { grep(it, regexp, resultLines) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.protected fun grep( fileName: File, regexp: String, resultLines: MutableList<String> ) = FileReader(fileName).use { grep(it, regexp, resultLines) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@testng-test-kit/src/main/kotlin/test/SimpleBaseTest.kt` around lines 341 - 345, Update the File-based grep overload to manage and close the FileReader it creates before returning, while preserving delegation to the existing grep(Reader, String, MutableList<String>) implementation and its results.
juherr
force-pushed
the
juherr/extract-jcommander-module
branch
from
July 29, 2026 12:53
df02482 to
924d410
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@testng-core/src/main/java/org/testng/TestNG.java`:
- Around line 275-281: Rename the Class-based overloads to distinct names to
preserve compilation for explicit null calls: update setListenerComparator to
setListenerComparatorClass at testng-core/src/main/java/org/testng/TestNG.java
lines 275-281, and apply the same Class-variant rename to the executor,
listener, and injector factory setters at lines 858-860, 870-872, and 2109-2111.
Update any internal references or callers to use the renamed methods while
leaving the object-based setters unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 610e15dd-3865-4943-93d2-b91b39521a74
📒 Files selected for processing (17)
CHANGES.txtsettings.gradle.ktstestng-cli/src/main/java/org/testng/cli/AbstractCliRunner.javatestng-cli/src/main/java/org/testng/cli/CliConfigurer.javatestng-cli/src/main/java/org/testng/cli/CliOptions.javatestng-cli/src/main/java/org/testng/cli/CliParseException.javatestng-cli/src/test/java/org/testng/cli/CliConfigurerParityTest.javatestng-cli/src/test/java/org/testng/cli/CliConfigurerValidateTest.javatestng-cli/src/test/java/org/testng/cli/CliOptionNamesTest.javatestng-cli/testng-cli-build.gradle.ktstestng-core/src/main/java/org/testng/CliRunners.javatestng-core/src/main/java/org/testng/CommandLineArgs.javatestng-core/src/main/java/org/testng/ITestNGCliRunner.javatestng-core/src/main/java/org/testng/TestNG.javatestng-core/src/test/java/test/TestHelper.javatestng-core/src/test/java/test/commandline/CommandLineOverridesXml.javatestng-core/src/test/java/test/configurationfailurepolicy/FailurePolicyTest.java
💤 Files with no reviewable changes (2)
- testng-core/src/test/java/test/TestHelper.java
- testng-core/src/test/java/test/commandline/CommandLineOverridesXml.java
🚧 Files skipped from review as they are similar to previous changes (14)
- testng-cli/src/main/java/org/testng/cli/CliParseException.java
- testng-core/src/main/java/org/testng/ITestNGCliRunner.java
- testng-core/src/main/java/org/testng/CliRunners.java
- testng-cli/src/main/java/org/testng/cli/AbstractCliRunner.java
- settings.gradle.kts
- testng-core/src/main/java/org/testng/CommandLineArgs.java
- testng-core/src/test/java/test/configurationfailurepolicy/FailurePolicyTest.java
- testng-cli/src/test/java/org/testng/cli/CliConfigurerParityTest.java
- testng-cli/src/main/java/org/testng/cli/CliConfigurer.java
- testng-cli/src/test/java/org/testng/cli/CliOptionNamesTest.java
- testng-cli/src/test/java/org/testng/cli/CliConfigurerValidateTest.java
- CHANGES.txt
- testng-cli/src/main/java/org/testng/cli/CliOptions.java
- testng-cli/testng-cli-build.gradle.kts
juherr
force-pushed
the
juherr/extract-jcommander-module
branch
from
July 29, 2026 13:41
924d410 to
700e9cb
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@testng-core/src/main/java/org/testng/TestNG.java`:
- Around line 1856-1869: Update the Javadoc for TestNG.addReporter(String) to
state that reporter resolution and instantiation occur synchronously, including
the warning for null or empty input and the exception/failure behavior when the
configured class is not an IReporter. Remove the inaccurate claim that invalid
reporters fail later during instantiation.
- Around line 1453-1462: Ensure the CLI path used by TestNG.privateMain()
rejects non-positive -threadcount values by throwing or reporting
TestNGException without invoking JVM termination. Update the relevant CLI
parsing/validation or TestNG.setThreadCount(int), while preserving normal
handling for positive thread counts and privateMain()’s non-termination
contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4e557936-754e-46e6-a220-bec3f93338fe
📒 Files selected for processing (94)
CHANGES.txtsettings.gradle.ktstestng-cli/src/main/java/org/testng/cli/AbstractCliRunner.javatestng-cli/src/main/java/org/testng/cli/CliConfigurer.javatestng-cli/src/main/java/org/testng/cli/CliOptions.javatestng-cli/src/main/java/org/testng/cli/CliParseException.javatestng-cli/src/test/java/org/testng/cli/CliConfigurerParityTest.javatestng-cli/src/test/java/org/testng/cli/CliConfigurerValidateTest.javatestng-cli/src/test/java/org/testng/cli/CliOptionNamesTest.javatestng-cli/testng-cli-build.gradle.ktstestng-core/src/main/java/org/testng/CliRunners.javatestng-core/src/main/java/org/testng/CommandLineArgs.javatestng-core/src/main/java/org/testng/ITestNGCliRunner.javatestng-core/src/main/java/org/testng/TestNG.javatestng-core/src/test/java/test/TestHelper.javatestng-core/src/test/java/test/commandline/CommandLineOverridesXml.javatestng-core/src/test/java/test/configurationfailurepolicy/FailurePolicyTest.javatestng-core/src/test/java/test/github1417/YetAnotherTestClassSample.javatestng-core/src/test/java/test/groups/issue2232/IssueTest.javatestng-core/src/test/java/test/junitreports/JUnitReportsTest.javatestng-core/src/test/java/test/listeners/ListenersTest.javatestng-core/src/test/java/test/listeners/factory/TestNGFactoryTest.javatestng-core/src/test/java/test/methodselectors/MethodSelectorInSuiteTest.javatestng-core/src/test/java/test/reports/EmailableReporterTest.javatestng-core/src/test/java/test/testng1231/TestExecutionListenerInvocationOrder.javatestng-core/src/test/java/test/thread/CustomExecutorServiceFactoryTest.javatestng-core/src/test/resources/testng.xmltestng-core/testng-core-build.gradle.ktstestng-jcommander/src/main/java/org/testng/cli/jcommander/Converter.javatestng-jcommander/src/main/java/org/testng/cli/jcommander/JCommanderCliRunner.javatestng-jcommander/src/main/java/org/testng/cli/jcommander/JCommanderOptions.javatestng-jcommander/src/main/resources/META-INF/services/org.testng.ITestNGCliRunnertestng-jcommander/src/test/java/org/testng/cli/jcommander/JCommanderCliRunnerTest.javatestng-jcommander/src/test/java/test/commandline/CommandLineOverridesXmlCommandLineTest.javatestng-jcommander/src/test/java/test/configurationfailurepolicy/FailurePolicyCommandLineTest.javatestng-jcommander/src/test/java/test/groups/issue2232/IssueCommandLineTest.javatestng-jcommander/src/test/java/test/listeners/ListenerWiringCommandLineTest.javatestng-jcommander/src/test/java/test/listeners/factory/TestNGFactoryCommandLineTest.javatestng-jcommander/src/test/java/test/methodselectors/CommandLineTest.javatestng-jcommander/src/test/java/test/methodselectors/MethodSelectorInSuiteCommandLineTest.javatestng-jcommander/src/test/java/test/methodselectors/NoTest1MethodSelector.javatestng-jcommander/src/test/java/test/reports/EmailableReporterCommandLineTest.javatestng-jcommander/src/test/java/test/thread/CustomExecutorServiceFactoryCommandLineTest.javatestng-jcommander/src/test/resources/1332.xmltestng-jcommander/src/test/resources/methodselector-in-xml.xmltestng-jcommander/src/test/resources/test/methodselectors/sampleTest.xmltestng-jcommander/src/test/resources/test/methodselectors/sampleTestExclusions.xmltestng-jcommander/src/test/resources/testnames/main-suite.xmltestng-jcommander/src/test/resources/testng-configfailure.xmltestng-jcommander/src/test/resources/testng-methodselectors.xmltestng-jcommander/testng-jcommander-build.gradle.ktstestng-test-kit/src/main/java/org/testng/jarfileutils/org/testng/SampleTest1.javatestng-test-kit/src/main/java/org/testng/jarfileutils/org/testng/SampleTest2.javatestng-test-kit/src/main/java/org/testng/jarfileutils/org/testng/SampleTest3.javatestng-test-kit/src/main/java/org/testng/jarfileutils/org/testng/SampleTest4.javatestng-test-kit/src/main/java/org/testng/jarfileutils/org/testng/SampleTest5.javatestng-test-kit/src/main/java/org/testng/testhelper/JarCreator.javatestng-test-kit/src/main/java/org/testng/testhelper/OutputDirectoryPatch.javatestng-test-kit/src/main/java/test/InvokedMethodNameListener.javatestng-test-kit/src/main/java/test/TestHelper.javatestng-test-kit/src/main/java/test/commandline/issue341/LocalLogAggregator.javatestng-test-kit/src/main/java/test/commandline/issue341/TestSampleA.javatestng-test-kit/src/main/java/test/commandline/issue341/TestSampleB.javatestng-test-kit/src/main/java/test/configurationfailurepolicy/ClassWithFailedBeforeMethodAndMultipleTests.javatestng-test-kit/src/main/java/test/groups/issue2232/Issue2232Suites.javatestng-test-kit/src/main/java/test/groups/issue2232/samples/SampleTest.javatestng-test-kit/src/main/java/test/groups/issue2232/samples/SampleTest2.javatestng-test-kit/src/main/java/test/listeners/cliwiring/FirstWiringListener.javatestng-test-kit/src/main/java/test/listeners/cliwiring/ReverseNameListenerComparator.javatestng-test-kit/src/main/java/test/listeners/cliwiring/SecondWiringListener.javatestng-test-kit/src/main/java/test/listeners/cliwiring/WiringLog.javatestng-test-kit/src/main/java/test/listeners/cliwiring/WiringSampleTest.javatestng-test-kit/src/main/java/test/listeners/factory/ExampleListener.javatestng-test-kit/src/main/java/test/listeners/factory/SampleTestCase.javatestng-test-kit/src/main/java/test/listeners/factory/SampleTestFactory.javatestng-test-kit/src/main/java/test/methodselectors/AllTestsMethodSelector.javatestng-test-kit/src/main/java/test/methodselectors/NoTest.javatestng-test-kit/src/main/java/test/methodselectors/NoTestSelector.javatestng-test-kit/src/main/java/test/methodselectors/SampleTest.javatestng-test-kit/src/main/java/test/methodselectors/Test2MethodSelector.javatestng-test-kit/src/main/java/test/reports/ReporterSample.javatestng-test-kit/src/main/java/test/testnames/TestNamesFeature.javatestng-test-kit/src/main/java/test/thread/issue3066/Issue3066ExecutorServiceFactory.javatestng-test-kit/src/main/java/test/thread/issue3066/Issue3066ThreadPoolExecutor.javatestng-test-kit/src/main/java/test/thread/issue3066/TestClassSample.javatestng-test-kit/src/main/kotlin/test/SimpleBaseTest.kttestng-test-kit/src/main/resources/jarfileutils/child.xmltestng-test-kit/src/main/resources/jarfileutils/child/child.xmltestng-test-kit/src/main/resources/jarfileutils/child/childofchild/childofchild.xmltestng-test-kit/src/main/resources/jarfileutils/childofchild/childofchild.xmltestng-test-kit/src/main/resources/jarfileutils/testng-tests.xmltestng-test-kit/testng-test-kit-build.gradle.ktstestng-test-osgi/src/test/java/org/testng/test/osgi/PlainOsgiTest.javatestng/testng-build.gradle.kts
💤 Files with no reviewable changes (7)
- testng-core/src/test/java/test/TestHelper.java
- testng-core/src/test/resources/testng.xml
- testng-core/src/test/java/test/thread/CustomExecutorServiceFactoryTest.java
- testng-core/src/test/java/test/listeners/factory/TestNGFactoryTest.java
- testng-core/src/test/java/test/commandline/CommandLineOverridesXml.java
- testng-core/src/test/java/test/reports/EmailableReporterTest.java
- testng-core/src/test/java/test/listeners/ListenersTest.java
🚧 Files skipped from review as they are similar to previous changes (68)
- testng-jcommander/src/test/resources/testng-configfailure.xml
- testng-cli/testng-cli-build.gradle.kts
- testng-test-kit/src/main/java/test/listeners/cliwiring/WiringSampleTest.java
- testng-test-kit/src/main/java/test/listeners/factory/SampleTestCase.java
- testng-jcommander/src/test/java/test/methodselectors/MethodSelectorInSuiteCommandLineTest.java
- testng-jcommander/src/main/resources/META-INF/services/org.testng.ITestNGCliRunner
- testng-test-kit/src/main/java/test/listeners/cliwiring/SecondWiringListener.java
- testng-cli/src/main/java/org/testng/cli/CliParseException.java
- testng-cli/src/test/java/org/testng/cli/CliOptionNamesTest.java
- testng-test-kit/src/main/java/test/methodselectors/AllTestsMethodSelector.java
- testng-jcommander/src/test/java/test/listeners/factory/TestNGFactoryCommandLineTest.java
- testng-test-kit/src/main/java/test/configurationfailurepolicy/ClassWithFailedBeforeMethodAndMultipleTests.java
- testng-test-kit/src/main/java/test/methodselectors/NoTestSelector.java
- testng-test-kit/src/main/resources/jarfileutils/childofchild/childofchild.xml
- testng-jcommander/src/test/resources/testnames/main-suite.xml
- testng-test-kit/src/main/java/test/commandline/issue341/TestSampleB.java
- testng-test-kit/src/main/java/test/methodselectors/NoTest.java
- testng-test-kit/src/main/resources/jarfileutils/child/childofchild/childofchild.xml
- testng-jcommander/src/test/java/test/configurationfailurepolicy/FailurePolicyCommandLineTest.java
- testng-jcommander/src/test/java/test/groups/issue2232/IssueCommandLineTest.java
- testng-test-kit/src/main/java/org/testng/jarfileutils/org/testng/SampleTest2.java
- testng-test-kit/src/main/java/test/groups/issue2232/samples/SampleTest.java
- testng-jcommander/src/test/resources/test/methodselectors/sampleTestExclusions.xml
- testng-test-kit/src/main/java/test/listeners/cliwiring/FirstWiringListener.java
- testng-jcommander/src/test/resources/test/methodselectors/sampleTest.xml
- testng-test-kit/src/main/java/org/testng/testhelper/OutputDirectoryPatch.java
- testng-test-kit/testng-test-kit-build.gradle.kts
- testng-test-kit/src/main/java/test/reports/ReporterSample.java
- testng-test-kit/src/main/java/test/thread/issue3066/Issue3066ThreadPoolExecutor.java
- testng-test-kit/src/main/java/org/testng/jarfileutils/org/testng/SampleTest1.java
- testng-jcommander/src/test/java/test/listeners/ListenerWiringCommandLineTest.java
- testng-core/src/main/java/org/testng/ITestNGCliRunner.java
- testng-jcommander/src/test/java/test/commandline/CommandLineOverridesXmlCommandLineTest.java
- testng-test-kit/src/main/java/test/listeners/factory/ExampleListener.java
- testng-test-kit/src/main/java/test/listeners/cliwiring/ReverseNameListenerComparator.java
- testng-test-kit/src/main/java/test/groups/issue2232/Issue2232Suites.java
- testng-test-kit/src/main/java/test/listeners/cliwiring/WiringLog.java
- testng-test-kit/src/main/java/org/testng/jarfileutils/org/testng/SampleTest5.java
- testng-test-kit/src/main/java/test/methodselectors/SampleTest.java
- testng-jcommander/src/main/java/org/testng/cli/jcommander/Converter.java
- testng-cli/src/main/java/org/testng/cli/AbstractCliRunner.java
- testng-core/src/test/java/test/configurationfailurepolicy/FailurePolicyTest.java
- testng-test-kit/src/main/java/test/testnames/TestNamesFeature.java
- testng-test-kit/src/main/resources/jarfileutils/testng-tests.xml
- testng-cli/src/test/java/org/testng/cli/CliConfigurerParityTest.java
- CHANGES.txt
- testng-cli/src/test/java/org/testng/cli/CliConfigurerValidateTest.java
- testng-jcommander/src/test/java/test/thread/CustomExecutorServiceFactoryCommandLineTest.java
- testng-test-kit/src/main/java/test/groups/issue2232/samples/SampleTest2.java
- testng-test-kit/src/main/java/test/listeners/factory/SampleTestFactory.java
- testng-jcommander/src/test/java/org/testng/cli/jcommander/JCommanderCliRunnerTest.java
- testng-test-kit/src/main/java/test/TestHelper.java
- testng-jcommander/src/test/resources/testng-methodselectors.xml
- testng-core/src/test/java/test/github1417/YetAnotherTestClassSample.java
- testng-test-kit/src/main/java/test/commandline/issue341/TestSampleA.java
- testng-core/src/test/java/test/junitreports/JUnitReportsTest.java
- testng-core/src/main/java/org/testng/CommandLineArgs.java
- testng-test-kit/src/main/java/test/InvokedMethodNameListener.java
- testng-core/src/main/java/org/testng/CliRunners.java
- testng-test-kit/src/main/resources/jarfileutils/child/child.xml
- testng-core/src/test/java/test/methodselectors/MethodSelectorInSuiteTest.java
- testng-test-osgi/src/test/java/org/testng/test/osgi/PlainOsgiTest.java
- testng-cli/src/main/java/org/testng/cli/CliConfigurer.java
- testng-jcommander/src/main/java/org/testng/cli/jcommander/JCommanderOptions.java
- testng-jcommander/src/test/java/test/methodselectors/CommandLineTest.java
- testng-cli/src/main/java/org/testng/cli/CliOptions.java
- testng-test-kit/src/main/java/test/methodselectors/Test2MethodSelector.java
- testng/testng-build.gradle.kts
juherr
force-pushed
the
juherr/extract-jcommander-module
branch
from
July 29, 2026 13:58
700e9cb to
601a311
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@testng-test-kit/src/main/java/org/testng/testhelper/JarCreator.java`:
- Around line 27-36: Ensure temporary JAR files returned by
JarCreator.generateJar are deleted after test consumption. Prefer adding
explicit cleanup at each caller with reliable finally/teardown handling, or
update generateJar to schedule deletion for every returned File while preserving
the existing generation behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b1ba8228-7b6e-4cc2-b928-21e4775e4196
📒 Files selected for processing (94)
CHANGES.txtsettings.gradle.ktstestng-cli/src/main/java/org/testng/cli/AbstractCliRunner.javatestng-cli/src/main/java/org/testng/cli/CliConfigurer.javatestng-cli/src/main/java/org/testng/cli/CliOptions.javatestng-cli/src/main/java/org/testng/cli/CliParseException.javatestng-cli/src/test/java/org/testng/cli/CliConfigurerParityTest.javatestng-cli/src/test/java/org/testng/cli/CliConfigurerValidateTest.javatestng-cli/src/test/java/org/testng/cli/CliOptionNamesTest.javatestng-cli/testng-cli-build.gradle.ktstestng-core/src/main/java/org/testng/CliRunners.javatestng-core/src/main/java/org/testng/CommandLineArgs.javatestng-core/src/main/java/org/testng/ITestNGCliRunner.javatestng-core/src/main/java/org/testng/TestNG.javatestng-core/src/test/java/test/TestHelper.javatestng-core/src/test/java/test/commandline/CommandLineOverridesXml.javatestng-core/src/test/java/test/configurationfailurepolicy/FailurePolicyTest.javatestng-core/src/test/java/test/github1417/YetAnotherTestClassSample.javatestng-core/src/test/java/test/groups/issue2232/IssueTest.javatestng-core/src/test/java/test/junitreports/JUnitReportsTest.javatestng-core/src/test/java/test/listeners/ListenersTest.javatestng-core/src/test/java/test/listeners/factory/TestNGFactoryTest.javatestng-core/src/test/java/test/methodselectors/MethodSelectorInSuiteTest.javatestng-core/src/test/java/test/reports/EmailableReporterTest.javatestng-core/src/test/java/test/testng1231/TestExecutionListenerInvocationOrder.javatestng-core/src/test/java/test/thread/CustomExecutorServiceFactoryTest.javatestng-core/src/test/resources/testng.xmltestng-core/testng-core-build.gradle.ktstestng-jcommander/src/main/java/org/testng/cli/jcommander/Converter.javatestng-jcommander/src/main/java/org/testng/cli/jcommander/JCommanderCliRunner.javatestng-jcommander/src/main/java/org/testng/cli/jcommander/JCommanderOptions.javatestng-jcommander/src/main/resources/META-INF/services/org.testng.ITestNGCliRunnertestng-jcommander/src/test/java/org/testng/cli/jcommander/JCommanderCliRunnerTest.javatestng-jcommander/src/test/java/test/commandline/CommandLineOverridesXmlCommandLineTest.javatestng-jcommander/src/test/java/test/configurationfailurepolicy/FailurePolicyCommandLineTest.javatestng-jcommander/src/test/java/test/groups/issue2232/IssueCommandLineTest.javatestng-jcommander/src/test/java/test/listeners/ListenerWiringCommandLineTest.javatestng-jcommander/src/test/java/test/listeners/factory/TestNGFactoryCommandLineTest.javatestng-jcommander/src/test/java/test/methodselectors/CommandLineTest.javatestng-jcommander/src/test/java/test/methodselectors/MethodSelectorInSuiteCommandLineTest.javatestng-jcommander/src/test/java/test/methodselectors/NoTest1MethodSelector.javatestng-jcommander/src/test/java/test/reports/EmailableReporterCommandLineTest.javatestng-jcommander/src/test/java/test/thread/CustomExecutorServiceFactoryCommandLineTest.javatestng-jcommander/src/test/resources/1332.xmltestng-jcommander/src/test/resources/methodselector-in-xml.xmltestng-jcommander/src/test/resources/test/methodselectors/sampleTest.xmltestng-jcommander/src/test/resources/test/methodselectors/sampleTestExclusions.xmltestng-jcommander/src/test/resources/testnames/main-suite.xmltestng-jcommander/src/test/resources/testng-configfailure.xmltestng-jcommander/src/test/resources/testng-methodselectors.xmltestng-jcommander/testng-jcommander-build.gradle.ktstestng-test-kit/src/main/java/org/testng/jarfileutils/org/testng/SampleTest1.javatestng-test-kit/src/main/java/org/testng/jarfileutils/org/testng/SampleTest2.javatestng-test-kit/src/main/java/org/testng/jarfileutils/org/testng/SampleTest3.javatestng-test-kit/src/main/java/org/testng/jarfileutils/org/testng/SampleTest4.javatestng-test-kit/src/main/java/org/testng/jarfileutils/org/testng/SampleTest5.javatestng-test-kit/src/main/java/org/testng/testhelper/JarCreator.javatestng-test-kit/src/main/java/org/testng/testhelper/OutputDirectoryPatch.javatestng-test-kit/src/main/java/test/InvokedMethodNameListener.javatestng-test-kit/src/main/java/test/TestHelper.javatestng-test-kit/src/main/java/test/commandline/issue341/LocalLogAggregator.javatestng-test-kit/src/main/java/test/commandline/issue341/TestSampleA.javatestng-test-kit/src/main/java/test/commandline/issue341/TestSampleB.javatestng-test-kit/src/main/java/test/configurationfailurepolicy/ClassWithFailedBeforeMethodAndMultipleTests.javatestng-test-kit/src/main/java/test/groups/issue2232/Issue2232Suites.javatestng-test-kit/src/main/java/test/groups/issue2232/samples/SampleTest.javatestng-test-kit/src/main/java/test/groups/issue2232/samples/SampleTest2.javatestng-test-kit/src/main/java/test/listeners/cliwiring/FirstWiringListener.javatestng-test-kit/src/main/java/test/listeners/cliwiring/ReverseNameListenerComparator.javatestng-test-kit/src/main/java/test/listeners/cliwiring/SecondWiringListener.javatestng-test-kit/src/main/java/test/listeners/cliwiring/WiringLog.javatestng-test-kit/src/main/java/test/listeners/cliwiring/WiringSampleTest.javatestng-test-kit/src/main/java/test/listeners/factory/ExampleListener.javatestng-test-kit/src/main/java/test/listeners/factory/SampleTestCase.javatestng-test-kit/src/main/java/test/listeners/factory/SampleTestFactory.javatestng-test-kit/src/main/java/test/methodselectors/AllTestsMethodSelector.javatestng-test-kit/src/main/java/test/methodselectors/NoTest.javatestng-test-kit/src/main/java/test/methodselectors/NoTestSelector.javatestng-test-kit/src/main/java/test/methodselectors/SampleTest.javatestng-test-kit/src/main/java/test/methodselectors/Test2MethodSelector.javatestng-test-kit/src/main/java/test/reports/ReporterSample.javatestng-test-kit/src/main/java/test/testnames/TestNamesFeature.javatestng-test-kit/src/main/java/test/thread/issue3066/Issue3066ExecutorServiceFactory.javatestng-test-kit/src/main/java/test/thread/issue3066/Issue3066ThreadPoolExecutor.javatestng-test-kit/src/main/java/test/thread/issue3066/TestClassSample.javatestng-test-kit/src/main/kotlin/test/SimpleBaseTest.kttestng-test-kit/src/main/resources/jarfileutils/child.xmltestng-test-kit/src/main/resources/jarfileutils/child/child.xmltestng-test-kit/src/main/resources/jarfileutils/child/childofchild/childofchild.xmltestng-test-kit/src/main/resources/jarfileutils/childofchild/childofchild.xmltestng-test-kit/src/main/resources/jarfileutils/testng-tests.xmltestng-test-kit/testng-test-kit-build.gradle.ktstestng-test-osgi/src/test/java/org/testng/test/osgi/PlainOsgiTest.javatestng/testng-build.gradle.kts
💤 Files with no reviewable changes (7)
- testng-core/src/test/java/test/TestHelper.java
- testng-core/src/test/java/test/listeners/factory/TestNGFactoryTest.java
- testng-core/src/test/java/test/thread/CustomExecutorServiceFactoryTest.java
- testng-core/src/test/resources/testng.xml
- testng-core/src/test/java/test/reports/EmailableReporterTest.java
- testng-core/src/test/java/test/commandline/CommandLineOverridesXml.java
- testng-core/src/test/java/test/listeners/ListenersTest.java
🚧 Files skipped from review as they are similar to previous changes (74)
- testng-jcommander/src/test/resources/methodselector-in-xml.xml
- testng-jcommander/src/test/resources/testnames/main-suite.xml
- testng-cli/src/main/java/org/testng/cli/CliParseException.java
- testng-test-kit/src/main/java/test/methodselectors/NoTest.java
- testng-cli/testng-cli-build.gradle.kts
- testng-test-kit/src/main/java/test/listeners/factory/SampleTestCase.java
- testng-test-kit/src/main/java/test/listeners/cliwiring/SecondWiringListener.java
- testng-test-kit/src/main/java/test/commandline/issue341/TestSampleB.java
- testng-test-kit/src/main/java/test/listeners/cliwiring/ReverseNameListenerComparator.java
- testng-jcommander/src/test/java/test/methodselectors/MethodSelectorInSuiteCommandLineTest.java
- testng-core/src/main/java/org/testng/ITestNGCliRunner.java
- testng-cli/src/main/java/org/testng/cli/AbstractCliRunner.java
- testng-cli/src/test/java/org/testng/cli/CliOptionNamesTest.java
- testng-test-kit/src/main/java/org/testng/jarfileutils/org/testng/SampleTest5.java
- testng-test-kit/src/main/java/test/methodselectors/SampleTest.java
- testng-test-kit/src/main/java/org/testng/jarfileutils/org/testng/SampleTest4.java
- testng-jcommander/src/test/resources/testng-methodselectors.xml
- testng-jcommander/src/test/java/test/listeners/factory/TestNGFactoryCommandLineTest.java
- testng-test-kit/src/main/java/test/thread/issue3066/TestClassSample.java
- testng-test-kit/src/main/resources/jarfileutils/testng-tests.xml
- testng-jcommander/src/test/java/test/configurationfailurepolicy/FailurePolicyCommandLineTest.java
- testng-test-kit/src/main/java/test/methodselectors/Test2MethodSelector.java
- testng-jcommander/src/main/java/org/testng/cli/jcommander/Converter.java
- testng-test-kit/src/main/java/test/commandline/issue341/TestSampleA.java
- testng-test-kit/src/main/resources/jarfileutils/childofchild/childofchild.xml
- testng-test-kit/testng-test-kit-build.gradle.kts
- testng-test-kit/src/main/java/org/testng/jarfileutils/org/testng/SampleTest3.java
- testng-jcommander/testng-jcommander-build.gradle.kts
- testng-jcommander/src/test/resources/1332.xml
- testng-core/src/test/java/test/github1417/YetAnotherTestClassSample.java
- testng-test-kit/src/main/java/test/thread/issue3066/Issue3066ExecutorServiceFactory.java
- testng-test-kit/src/main/java/test/methodselectors/NoTestSelector.java
- testng-test-kit/src/main/java/test/methodselectors/AllTestsMethodSelector.java
- testng-test-kit/src/main/java/test/groups/issue2232/samples/SampleTest.java
- testng-test-kit/src/main/resources/jarfileutils/child/child.xml
- testng-test-kit/src/main/java/test/testnames/TestNamesFeature.java
- testng-test-kit/src/main/java/test/listeners/cliwiring/FirstWiringListener.java
- testng-jcommander/src/test/java/test/groups/issue2232/IssueCommandLineTest.java
- testng-cli/src/test/java/org/testng/cli/CliConfigurerValidateTest.java
- testng-test-kit/src/main/kotlin/test/SimpleBaseTest.kt
- testng-test-kit/src/main/java/test/configurationfailurepolicy/ClassWithFailedBeforeMethodAndMultipleTests.java
- testng-jcommander/src/test/java/test/methodselectors/NoTest1MethodSelector.java
- settings.gradle.kts
- testng-test-kit/src/main/java/org/testng/testhelper/OutputDirectoryPatch.java
- testng-core/testng-core-build.gradle.kts
- testng-test-kit/src/main/java/test/InvokedMethodNameListener.java
- testng-core/src/main/java/org/testng/CliRunners.java
- testng-jcommander/src/main/java/org/testng/cli/jcommander/JCommanderCliRunner.java
- testng-core/src/test/java/test/testng1231/TestExecutionListenerInvocationOrder.java
- testng-jcommander/src/test/java/test/thread/CustomExecutorServiceFactoryCommandLineTest.java
- testng-core/src/test/java/test/methodselectors/MethodSelectorInSuiteTest.java
- testng-jcommander/src/test/java/test/listeners/ListenerWiringCommandLineTest.java
- testng-test-kit/src/main/java/test/thread/issue3066/Issue3066ThreadPoolExecutor.java
- testng-jcommander/src/test/java/test/commandline/CommandLineOverridesXmlCommandLineTest.java
- testng-test-kit/src/main/java/test/TestHelper.java
- testng-jcommander/src/test/resources/testng-configfailure.xml
- testng-test-kit/src/main/java/test/listeners/factory/SampleTestFactory.java
- testng-jcommander/src/test/java/org/testng/cli/jcommander/JCommanderCliRunnerTest.java
- testng-core/src/test/java/test/configurationfailurepolicy/FailurePolicyTest.java
- CHANGES.txt
- testng-test-kit/src/main/java/test/listeners/factory/ExampleListener.java
- testng-core/src/test/java/test/groups/issue2232/IssueTest.java
- testng-cli/src/main/java/org/testng/cli/CliOptions.java
- testng-test-kit/src/main/java/test/commandline/issue341/LocalLogAggregator.java
- testng-cli/src/test/java/org/testng/cli/CliConfigurerParityTest.java
- testng-test-kit/src/main/java/test/groups/issue2232/samples/SampleTest2.java
- testng-cli/src/main/java/org/testng/cli/CliConfigurer.java
- testng-core/src/main/java/org/testng/CommandLineArgs.java
- testng-jcommander/src/test/java/test/methodselectors/CommandLineTest.java
- testng-jcommander/src/main/java/org/testng/cli/jcommander/JCommanderOptions.java
- testng-core/src/test/java/test/junitreports/JUnitReportsTest.java
- testng-test-osgi/src/test/java/org/testng/test/osgi/PlainOsgiTest.java
- testng-test-kit/src/main/resources/jarfileutils/child/childofchild/childofchild.xml
- testng-core/src/main/java/org/testng/TestNG.java
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@testng-test-kit/src/main/java/org/testng/testhelper/JarCreator.java`:
- Around line 27-36: Ensure temporary JAR files returned by
JarCreator.generateJar are deleted after test consumption. Prefer adding
explicit cleanup at each caller with reliable finally/teardown handling, or
update generateJar to schedule deletion for every returned File while preserving
the existing generation behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b1ba8228-7b6e-4cc2-b928-21e4775e4196
📒 Files selected for processing (94)
CHANGES.txtsettings.gradle.ktstestng-cli/src/main/java/org/testng/cli/AbstractCliRunner.javatestng-cli/src/main/java/org/testng/cli/CliConfigurer.javatestng-cli/src/main/java/org/testng/cli/CliOptions.javatestng-cli/src/main/java/org/testng/cli/CliParseException.javatestng-cli/src/test/java/org/testng/cli/CliConfigurerParityTest.javatestng-cli/src/test/java/org/testng/cli/CliConfigurerValidateTest.javatestng-cli/src/test/java/org/testng/cli/CliOptionNamesTest.javatestng-cli/testng-cli-build.gradle.ktstestng-core/src/main/java/org/testng/CliRunners.javatestng-core/src/main/java/org/testng/CommandLineArgs.javatestng-core/src/main/java/org/testng/ITestNGCliRunner.javatestng-core/src/main/java/org/testng/TestNG.javatestng-core/src/test/java/test/TestHelper.javatestng-core/src/test/java/test/commandline/CommandLineOverridesXml.javatestng-core/src/test/java/test/configurationfailurepolicy/FailurePolicyTest.javatestng-core/src/test/java/test/github1417/YetAnotherTestClassSample.javatestng-core/src/test/java/test/groups/issue2232/IssueTest.javatestng-core/src/test/java/test/junitreports/JUnitReportsTest.javatestng-core/src/test/java/test/listeners/ListenersTest.javatestng-core/src/test/java/test/listeners/factory/TestNGFactoryTest.javatestng-core/src/test/java/test/methodselectors/MethodSelectorInSuiteTest.javatestng-core/src/test/java/test/reports/EmailableReporterTest.javatestng-core/src/test/java/test/testng1231/TestExecutionListenerInvocationOrder.javatestng-core/src/test/java/test/thread/CustomExecutorServiceFactoryTest.javatestng-core/src/test/resources/testng.xmltestng-core/testng-core-build.gradle.ktstestng-jcommander/src/main/java/org/testng/cli/jcommander/Converter.javatestng-jcommander/src/main/java/org/testng/cli/jcommander/JCommanderCliRunner.javatestng-jcommander/src/main/java/org/testng/cli/jcommander/JCommanderOptions.javatestng-jcommander/src/main/resources/META-INF/services/org.testng.ITestNGCliRunnertestng-jcommander/src/test/java/org/testng/cli/jcommander/JCommanderCliRunnerTest.javatestng-jcommander/src/test/java/test/commandline/CommandLineOverridesXmlCommandLineTest.javatestng-jcommander/src/test/java/test/configurationfailurepolicy/FailurePolicyCommandLineTest.javatestng-jcommander/src/test/java/test/groups/issue2232/IssueCommandLineTest.javatestng-jcommander/src/test/java/test/listeners/ListenerWiringCommandLineTest.javatestng-jcommander/src/test/java/test/listeners/factory/TestNGFactoryCommandLineTest.javatestng-jcommander/src/test/java/test/methodselectors/CommandLineTest.javatestng-jcommander/src/test/java/test/methodselectors/MethodSelectorInSuiteCommandLineTest.javatestng-jcommander/src/test/java/test/methodselectors/NoTest1MethodSelector.javatestng-jcommander/src/test/java/test/reports/EmailableReporterCommandLineTest.javatestng-jcommander/src/test/java/test/thread/CustomExecutorServiceFactoryCommandLineTest.javatestng-jcommander/src/test/resources/1332.xmltestng-jcommander/src/test/resources/methodselector-in-xml.xmltestng-jcommander/src/test/resources/test/methodselectors/sampleTest.xmltestng-jcommander/src/test/resources/test/methodselectors/sampleTestExclusions.xmltestng-jcommander/src/test/resources/testnames/main-suite.xmltestng-jcommander/src/test/resources/testng-configfailure.xmltestng-jcommander/src/test/resources/testng-methodselectors.xmltestng-jcommander/testng-jcommander-build.gradle.ktstestng-test-kit/src/main/java/org/testng/jarfileutils/org/testng/SampleTest1.javatestng-test-kit/src/main/java/org/testng/jarfileutils/org/testng/SampleTest2.javatestng-test-kit/src/main/java/org/testng/jarfileutils/org/testng/SampleTest3.javatestng-test-kit/src/main/java/org/testng/jarfileutils/org/testng/SampleTest4.javatestng-test-kit/src/main/java/org/testng/jarfileutils/org/testng/SampleTest5.javatestng-test-kit/src/main/java/org/testng/testhelper/JarCreator.javatestng-test-kit/src/main/java/org/testng/testhelper/OutputDirectoryPatch.javatestng-test-kit/src/main/java/test/InvokedMethodNameListener.javatestng-test-kit/src/main/java/test/TestHelper.javatestng-test-kit/src/main/java/test/commandline/issue341/LocalLogAggregator.javatestng-test-kit/src/main/java/test/commandline/issue341/TestSampleA.javatestng-test-kit/src/main/java/test/commandline/issue341/TestSampleB.javatestng-test-kit/src/main/java/test/configurationfailurepolicy/ClassWithFailedBeforeMethodAndMultipleTests.javatestng-test-kit/src/main/java/test/groups/issue2232/Issue2232Suites.javatestng-test-kit/src/main/java/test/groups/issue2232/samples/SampleTest.javatestng-test-kit/src/main/java/test/groups/issue2232/samples/SampleTest2.javatestng-test-kit/src/main/java/test/listeners/cliwiring/FirstWiringListener.javatestng-test-kit/src/main/java/test/listeners/cliwiring/ReverseNameListenerComparator.javatestng-test-kit/src/main/java/test/listeners/cliwiring/SecondWiringListener.javatestng-test-kit/src/main/java/test/listeners/cliwiring/WiringLog.javatestng-test-kit/src/main/java/test/listeners/cliwiring/WiringSampleTest.javatestng-test-kit/src/main/java/test/listeners/factory/ExampleListener.javatestng-test-kit/src/main/java/test/listeners/factory/SampleTestCase.javatestng-test-kit/src/main/java/test/listeners/factory/SampleTestFactory.javatestng-test-kit/src/main/java/test/methodselectors/AllTestsMethodSelector.javatestng-test-kit/src/main/java/test/methodselectors/NoTest.javatestng-test-kit/src/main/java/test/methodselectors/NoTestSelector.javatestng-test-kit/src/main/java/test/methodselectors/SampleTest.javatestng-test-kit/src/main/java/test/methodselectors/Test2MethodSelector.javatestng-test-kit/src/main/java/test/reports/ReporterSample.javatestng-test-kit/src/main/java/test/testnames/TestNamesFeature.javatestng-test-kit/src/main/java/test/thread/issue3066/Issue3066ExecutorServiceFactory.javatestng-test-kit/src/main/java/test/thread/issue3066/Issue3066ThreadPoolExecutor.javatestng-test-kit/src/main/java/test/thread/issue3066/TestClassSample.javatestng-test-kit/src/main/kotlin/test/SimpleBaseTest.kttestng-test-kit/src/main/resources/jarfileutils/child.xmltestng-test-kit/src/main/resources/jarfileutils/child/child.xmltestng-test-kit/src/main/resources/jarfileutils/child/childofchild/childofchild.xmltestng-test-kit/src/main/resources/jarfileutils/childofchild/childofchild.xmltestng-test-kit/src/main/resources/jarfileutils/testng-tests.xmltestng-test-kit/testng-test-kit-build.gradle.ktstestng-test-osgi/src/test/java/org/testng/test/osgi/PlainOsgiTest.javatestng/testng-build.gradle.kts
💤 Files with no reviewable changes (7)
- testng-core/src/test/java/test/TestHelper.java
- testng-core/src/test/java/test/listeners/factory/TestNGFactoryTest.java
- testng-core/src/test/java/test/thread/CustomExecutorServiceFactoryTest.java
- testng-core/src/test/resources/testng.xml
- testng-core/src/test/java/test/reports/EmailableReporterTest.java
- testng-core/src/test/java/test/commandline/CommandLineOverridesXml.java
- testng-core/src/test/java/test/listeners/ListenersTest.java
🚧 Files skipped from review as they are similar to previous changes (74)
- testng-jcommander/src/test/resources/methodselector-in-xml.xml
- testng-jcommander/src/test/resources/testnames/main-suite.xml
- testng-cli/src/main/java/org/testng/cli/CliParseException.java
- testng-test-kit/src/main/java/test/methodselectors/NoTest.java
- testng-cli/testng-cli-build.gradle.kts
- testng-test-kit/src/main/java/test/listeners/factory/SampleTestCase.java
- testng-test-kit/src/main/java/test/listeners/cliwiring/SecondWiringListener.java
- testng-test-kit/src/main/java/test/commandline/issue341/TestSampleB.java
- testng-test-kit/src/main/java/test/listeners/cliwiring/ReverseNameListenerComparator.java
- testng-jcommander/src/test/java/test/methodselectors/MethodSelectorInSuiteCommandLineTest.java
- testng-core/src/main/java/org/testng/ITestNGCliRunner.java
- testng-cli/src/main/java/org/testng/cli/AbstractCliRunner.java
- testng-cli/src/test/java/org/testng/cli/CliOptionNamesTest.java
- testng-test-kit/src/main/java/org/testng/jarfileutils/org/testng/SampleTest5.java
- testng-test-kit/src/main/java/test/methodselectors/SampleTest.java
- testng-test-kit/src/main/java/org/testng/jarfileutils/org/testng/SampleTest4.java
- testng-jcommander/src/test/resources/testng-methodselectors.xml
- testng-jcommander/src/test/java/test/listeners/factory/TestNGFactoryCommandLineTest.java
- testng-test-kit/src/main/java/test/thread/issue3066/TestClassSample.java
- testng-test-kit/src/main/resources/jarfileutils/testng-tests.xml
- testng-jcommander/src/test/java/test/configurationfailurepolicy/FailurePolicyCommandLineTest.java
- testng-test-kit/src/main/java/test/methodselectors/Test2MethodSelector.java
- testng-jcommander/src/main/java/org/testng/cli/jcommander/Converter.java
- testng-test-kit/src/main/java/test/commandline/issue341/TestSampleA.java
- testng-test-kit/src/main/resources/jarfileutils/childofchild/childofchild.xml
- testng-test-kit/testng-test-kit-build.gradle.kts
- testng-test-kit/src/main/java/org/testng/jarfileutils/org/testng/SampleTest3.java
- testng-jcommander/testng-jcommander-build.gradle.kts
- testng-jcommander/src/test/resources/1332.xml
- testng-core/src/test/java/test/github1417/YetAnotherTestClassSample.java
- testng-test-kit/src/main/java/test/thread/issue3066/Issue3066ExecutorServiceFactory.java
- testng-test-kit/src/main/java/test/methodselectors/NoTestSelector.java
- testng-test-kit/src/main/java/test/methodselectors/AllTestsMethodSelector.java
- testng-test-kit/src/main/java/test/groups/issue2232/samples/SampleTest.java
- testng-test-kit/src/main/resources/jarfileutils/child/child.xml
- testng-test-kit/src/main/java/test/testnames/TestNamesFeature.java
- testng-test-kit/src/main/java/test/listeners/cliwiring/FirstWiringListener.java
- testng-jcommander/src/test/java/test/groups/issue2232/IssueCommandLineTest.java
- testng-cli/src/test/java/org/testng/cli/CliConfigurerValidateTest.java
- testng-test-kit/src/main/kotlin/test/SimpleBaseTest.kt
- testng-test-kit/src/main/java/test/configurationfailurepolicy/ClassWithFailedBeforeMethodAndMultipleTests.java
- testng-jcommander/src/test/java/test/methodselectors/NoTest1MethodSelector.java
- settings.gradle.kts
- testng-test-kit/src/main/java/org/testng/testhelper/OutputDirectoryPatch.java
- testng-core/testng-core-build.gradle.kts
- testng-test-kit/src/main/java/test/InvokedMethodNameListener.java
- testng-core/src/main/java/org/testng/CliRunners.java
- testng-jcommander/src/main/java/org/testng/cli/jcommander/JCommanderCliRunner.java
- testng-core/src/test/java/test/testng1231/TestExecutionListenerInvocationOrder.java
- testng-jcommander/src/test/java/test/thread/CustomExecutorServiceFactoryCommandLineTest.java
- testng-core/src/test/java/test/methodselectors/MethodSelectorInSuiteTest.java
- testng-jcommander/src/test/java/test/listeners/ListenerWiringCommandLineTest.java
- testng-test-kit/src/main/java/test/thread/issue3066/Issue3066ThreadPoolExecutor.java
- testng-jcommander/src/test/java/test/commandline/CommandLineOverridesXmlCommandLineTest.java
- testng-test-kit/src/main/java/test/TestHelper.java
- testng-jcommander/src/test/resources/testng-configfailure.xml
- testng-test-kit/src/main/java/test/listeners/factory/SampleTestFactory.java
- testng-jcommander/src/test/java/org/testng/cli/jcommander/JCommanderCliRunnerTest.java
- testng-core/src/test/java/test/configurationfailurepolicy/FailurePolicyTest.java
- CHANGES.txt
- testng-test-kit/src/main/java/test/listeners/factory/ExampleListener.java
- testng-core/src/test/java/test/groups/issue2232/IssueTest.java
- testng-cli/src/main/java/org/testng/cli/CliOptions.java
- testng-test-kit/src/main/java/test/commandline/issue341/LocalLogAggregator.java
- testng-cli/src/test/java/org/testng/cli/CliConfigurerParityTest.java
- testng-test-kit/src/main/java/test/groups/issue2232/samples/SampleTest2.java
- testng-cli/src/main/java/org/testng/cli/CliConfigurer.java
- testng-core/src/main/java/org/testng/CommandLineArgs.java
- testng-jcommander/src/test/java/test/methodselectors/CommandLineTest.java
- testng-jcommander/src/main/java/org/testng/cli/jcommander/JCommanderOptions.java
- testng-core/src/test/java/test/junitreports/JUnitReportsTest.java
- testng-test-osgi/src/test/java/org/testng/test/osgi/PlainOsgiTest.java
- testng-test-kit/src/main/resources/jarfileutils/child/childofchild/childofchild.xml
- testng-core/src/main/java/org/testng/TestNG.java
🛑 Comments failed to post (1)
testng-test-kit/src/main/java/org/testng/testhelper/JarCreator.java (1)
27-36: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash ast-grep outline testng-test-kit/src/main/java/org/testng/testhelper/JarCreator.java --view expanded rg -n -C 6 '\bJarCreator\s*\.\s*generateJar\s*\(' . -g '*.java' -g '*.kt'Repository: testng-team/testng
Length of output: 4801
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "## test files" wc -l testng-jcommander/src/test/java/test/commandline/CommandLineOverridesXmlCommandLineTest.java \ testng-core/src/test/java/org/testng/JarFileUtilsTest.java \ testng-core/src/test/java/test/cli/CliTest.java echo echo "## CommandLineOverridesXmlCommandLineTest lifecycle terms" rg -n "delete|deleteOnExit|AfterClass|AfterTest|BeforeClass|jarfile|generateJar|testjar" testng-jcommander/src/test/java/test/commandline/CommandLineOverridesXmlCommandLineTest.java || true echo echo "## JarFileUtilsTest lifecycle terms" rg -n "delete|deleteOnExit|AfterClass|AfterTest|BeforeClass|jar|generateJar" testng-core/src/test/java/org/testng/JarFileUtilsTest.java || true echo echo "## CliTest lifecycle terms" rg -n "delete|deleteOnExit|AfterClass|AfterTest|AfterMethods|BeforeClass|AfterClass|jarfile|jar|generateJar|setTestJar" testng-core/src/test/java/test/cli/CliTest.java || trueRepository: testng-team/testng
Length of output: 2290
Delete generated test JARs after consumption.
generateJarreturns temp files to callers, but the test call sites do not establish cleanup ownership or schedule deletion. Add cleanup at the call sites, or makegenerateJarschedule deletion for returned files.🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 29-29: Invalid permissions for temporary file
Context: File jarFile = File.createTempFile(prefix, ".jar");
Note: [CWE-378] Creation of Temporary File With Insecure Permissions. Security best practice.(tempfile-permissions)
[warning] 29-29: Temporary file not deleted
Context: File.createTempFile(prefix, ".jar")
Note: [CWE-377] Insecure Temporary File. Security best practice.(tempfile-delete)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@testng-test-kit/src/main/java/org/testng/testhelper/JarCreator.java` around lines 27 - 36, Ensure temporary JAR files returned by JarCreator.generateJar are deleted after test consumption. Prefer adding explicit cleanup at each caller with reliable finally/teardown handling, or update generateJar to schedule deletion for every returned File while preserving the existing generation behavior.
2 tasks
juherr
force-pushed
the
juherr/extract-jcommander-module
branch
from
July 29, 2026 14:28
601a311 to
6274834
Compare
Member
Author
|
@krmahadevan ready for your review |
testng-core depended on org.jcommander:jcommander in `api` scope, so every consumer got the parsing library whether or not it drove TestNG from a command line. The coupling ran deep: CommandLineArgs was a @Parameter-annotated POJO, TestNG held a static JCommander field, and TestNG.configure(CommandLineArgs) mixed applying configuration with CLI-only string-to-object conversion. The front end now lives in two new modules: testng-cli CliOptions, CliConfigurer, AbstractCliRunner - no parser testng-jcommander the JCommander implementation, plus Converter testng-core keeps a minimal ServiceLoader SPI, ITestNGCliRunner, so that "java -cp testng.jar org.testng.TestNG suite.xml" is unchanged: both modules are bundled into the merged jar. Implementations never terminate the JVM - an unusable command line comes back as a TestNGException, and only TestNG.main turns it into a message, a usage banner and exit code 1. TestNG.setThreadCount now raises a TestNGException below 1 instead of printing the usage banner and calling System.exit(1), so that no library setter can kill an embedder's JVM. The command line output is unchanged: TestNG.main turns that exception into the same message and the same exit code. Backward compatibility for 7.x is kept through frozen @deprecated shims: CommandLineArgs (annotations stripped), TestNG.configure(CommandLineArgs), TestNG.validateCommandLineParameters, TestNG.main and TestNG.privateMain. TestNG.configure(Map), the method Maven Surefire calls, still builds a CommandLineArgs, so removing that class depends on Surefire moving first. CliConfigurerParityTest compares the state both paths produce, field by field, because nothing else keeps the frozen copy in sync with the live one. testng-test-kit becomes the shared test-fixture library, so the 22 tests whose subject is the command line could move to testng-jcommander. The 243 classes extending SimpleBaseTest are unaffected. The two modules are deliberately not published on their own, and are kept out of the BOM for the same reason: that would ship org.testng.cli.* and its service registration in two artifacts at once. It is a packaging change for the next major, tracked in testng-team#3301. Supersedes testng-team#2612.
juherr
force-pushed
the
juherr/extract-jcommander-module
branch
from
July 30, 2026 08:09
6274834 to
56cde42
Compare
Member
Author
|
@krmahadevan please review |
Member
You are on fire @juherr 🔥 |
Member
Author
|
🧑🚒 |
Member
|
@juherr - Please give me sometime to go through this. It is a large PR. Hope that is fine. I will get back in a day or two. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes # .
Did you remember to?
CHANGES.txt./gradlew autostyleApplyWhy
testng-coredepends onorg.jcommander:jcommanderinapiscope, so every consumer gets the parsing library whether or not it drives TestNG from a command line. The coupling runs deep:CommandLineArgsis a@Parameter-annotated POJO,TestNGholds a staticJCommanderfield, andTestNG.configure(CommandLineArgs)mixes applying configuration with CLI-only string-to-object conversion.This picks up #2612 and finishes it, with the additional goal that no CLI logic remains in the core.
What
Two new modules:
testng-cliCliOptions,CliConfigurer,AbstractCliRunner— parser-agnostic, no JCommandertestng-jcommanderConvertertestng-corekeeps a minimalServiceLoaderSPI,ITestNGCliRunner, and nothing else CLI-shaped. Both modules are bundled into the mergedorg.testng:testngjar, sojava -cp testng.jar org.testng.TestNG suite.xmlbehaves exactly as before.Implementations never terminate the JVM: an unusable command line comes back as a
TestNGException, and onlyTestNG.mainturns it into a message, a usage banner and exit code 1.System.exitno longer appears anywhere intestng-cliortestng-jcommander.Compatibility
7.x compatibility is kept through frozen
@Deprecatedshims:CommandLineArgs(annotations stripped),TestNG.configure(CommandLineArgs),TestNG.validateCommandLineParameters,TestNG.main,TestNG.privateMain.CliConfigurerParityTestcompares the state the frozen path and the new path produce, field by field — nothing else keeps them in sync.Behaviour changes worth reviewing:
TestNG.privateMainthrows where it used toSystem.exiton a bad command line.TestNG.mainis unchanged.TestNG.validateCommandLineParametersreports failures asTestNGExceptioninstead of JCommander'sParameterException. Both unchecked, signature unchanged, but a caller catchingParameterExceptionno longer catches it.TestNG.setThreadCountraises aTestNGExceptionbelow 1 instead of printing the usage banner and callingSystem.exit(1). No library setter should be able to kill an embedder's JVM; the command line output is unchanged becauseTestNG.mainturns that exception into the same message and exit code.org.testng.Convertermoved toorg.testng.cli.jcommander.Converter. Still intestng.jar; no shim.setListenerComparatorClass/setListenerFactoryClass/setExecutorServiceFactoryClass/setInjectorFactoryClass. Named rather than overloaded on purpose, so that existing calls passing a barenullkeep compiling; this follows thesetTestRunnerFactoryClassprecedent already inTestNG.testng-coreno longer brings JCommander transitively — embedders that relied on it being on the classpath have to declare it.Tests
testng-test-kitbecomes the shared test-fixture library so the 22 tests whose subject is the command line could move totestng-jcommander. The 243 classes extendingSimpleBaseTestare untouched.I diffed the executed test classes against
masterto prove nothing stopped running: 386 → 385 classes, the single removal beingtest.methodselectors.CommandLineTest, which moved wholesale. The 12414 → 12192 test-count delta is entirely 37 relocated or de-duplicated methods × the 6 invocations the suite applies to each class.Full build green:
testng-core12192 ·testng-jcommander39 ·testng-cli14 ·testng-asserts181 ·testng-test-osgi4 — 0 failures.Verified end to end against the built jar, not just by tests:
java -cp testng.jar org.testng.TestNG suite.xml→ exit 0NoClassDefFoundErrororg.testng.cli.jcommander.Converter -d …→ writes the YAMLDeliberately not done
The two modules are not published on their own. Only
:testngis published today, so publishing them would shiporg.testng.cli.*and itsMETA-INF/servicesentry in two artifacts at once — duplicate classes and duplicate service registrations that neither Gradle nor Maven can dedupe. That is a packaging change for the next major; tracked in #3301.Two pre-existing bugs are preserved for parity and now carry a
FIXMErather than staying invisible:-propagateDataProviderFailureAsTestFailureis applied on every run regardless of its value, and-alwaysrunlistenerscannot be switched off from the command line.Summary by CodeRabbit
New Features
java -cp testng.jar org.testng.TestNG suite.xmlworkflow.Bug Fixes
TestNGExceptionwith consistent error reporting;setThreadCount(<1)now throws instead of exiting.Documentation
Converterrelocation for 7.13+.