Support to execlude somes test in option of command line #3200
Conversation
WalkthroughThe pull request introduces enhancements to TestNG's test name matching functionality, focusing on providing more flexible regex-based test name selection. The changes primarily involve modifying the Changes
Assessment against linked issues
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Sorry, I try to revert the update of README.md, it still shows there is one line have update.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
testng-core/src/test/java/org/testng/xml/XmlTestTest.java (1)
23-29: Consider providing additional regex edge cases in tests
These tests only cover a basic pattern match and a negative pattern. It may be useful to add more boundary scenarios (like empty regex patterns, special character sets, or capturing groups) to ensure broader validation of the matching logic.testng-core-api/src/main/java/org/testng/xml/XmlTest.java (1)
628-628: Document implications of regex-based matching
While regex-based matching offers flexibility, it can introduce subtle pitfalls if the user expects literal matching. A brief javadoc note clarifying that wildcard or partial matches are possible might help avoid confusion.CHANGES.txt (1)
9-9: Correct typographical errors in release note
There's a misspelling ("execlude somes 'test'"). Consider correcting it to something like "exclude some tests" for clarity and consistency with the rest of the release notes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGES.txt(1 hunks)README.md(1 hunks)testng-core-api/src/main/java/org/testng/xml/XmlTest.java(2 hunks)testng-core/src/main/java/org/testng/xml/internal/TestNamesMatcher.java(2 hunks)testng-core/src/test/java/org/testng/xml/XmlTestTest.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- README.md
🔇 Additional comments (2)
testng-core/src/main/java/org/testng/xml/internal/TestNamesMatcher.java (1)
93-94: Validate performance of regex matching on large test name sets
Although regex provides flexibility, it can be slower on large collections. Verify that performance is acceptable, especially in scenarios with numerous test names or frequent calls togetMissedTestNames.testng-core-api/src/main/java/org/testng/xml/XmlTest.java (1)
4-4: Ensure consistent usage of java.util.regex.Pattern
Importing Pattern suggests usage of regex logic within the class. Just confirm that the rest of the class interacts properly without overshadowing simpler checks.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
testng-core/src/test/java/test/methodselectors/CommandLineTest.java (1)
173-192: Enhance test coverage and fix issue reference.The test description references GITHUB-2407 but this test appears to be verifying functionality for GITHUB-3196 (support to exclude tests in command line options).
Consider:
- Updating the description to reference the correct issue: GITHUB-3196
- Adding assertions to verify that tests not matching the regex pattern were excluded
- Adding more test cases with different regex patterns to improve coverage
- @Test(description = "GITHUB-2407") + @Test(description = "GITHUB-3196") public void testSpecificTestNamesWithRegexCommandLineExclusions() { String[] args = new String[] { "src/test/resources/testnames/main-suite.xml", "-log", "0", "-d", OutputDirectoryPatch.getOutputDirectory(), "-testnames", "/^testGroup1.*/" }; TestNG.privateMain(args, tla); String[] passed = {"sampleOutputTest1"}; String[] failed = {}; + String[] skipped = {"testGroup2Test1", "testGroup2Test2"}; // Add verification for excluded tests verifyTests("Passed", passed, tla.getPassedTests()); verifyTests("Failed", failed, tla.getFailedTests()); + verifyTests("Skipped", skipped, tla.getSkippedTests()); }testng-core-api/src/main/java/org/testng/xml/XmlTest.java (1)
628-637: Optimize regex pattern compilation and add error handling.The current implementation compiles the regex pattern on every match. For better performance and error handling:
Consider:
public boolean nameMatchesAny(List<String> names) { return names.contains(getName()) || names.stream() .anyMatch( regex -> { if (regex.startsWith("/") && regex.endsWith("/")) { String trimmedRegex = regex.substring(1, regex.length() - 1); - return Pattern.matches(trimmedRegex, getName()); + try { + Pattern pattern = Pattern.compile(trimmedRegex); + return pattern.matcher(getName()).matches(); + } catch (java.util.regex.PatternSyntaxException e) { + throw new TestNGException("Invalid regex pattern: " + regex, e); + } } return false; }); }CHANGES.txt (1)
9-9: Enhance change log entry with more details.The current entry "Fixed: GITHUB-3196: support to execlude somes tests in option of command line" has typos and lacks details about the feature.
Consider updating to:
-Fixed: GITHUB-3196: support to execlude somes tests in option of command line +Fixed: GITHUB-3196: Support excluding tests using regex patterns in command line options. Tests can now be excluded using patterns like "/^pattern.*/" in the -testnames option.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGES.txt(1 hunks)testng-core-api/src/main/java/org/testng/xml/XmlTest.java(2 hunks)testng-core/src/main/java/org/testng/xml/internal/TestNamesMatcher.java(2 hunks)testng-core/src/test/java/org/testng/xml/XmlTestTest.java(1 hunks)testng-core/src/test/java/test/methodselectors/CommandLineTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- testng-core/src/main/java/org/testng/xml/internal/TestNamesMatcher.java
- testng-core/src/test/java/org/testng/xml/XmlTestTest.java
🔇 Additional comments (1)
testng-core-api/src/main/java/org/testng/xml/XmlTest.java (1)
4-4: LGTM!The import for Pattern is appropriate for the new regex functionality.
Hi, |
Fixes #3196 .
Did you remember to?
CHANGES.txt./gradlew autostyleApplyWe encourage pull requests that:
If your pull request involves fixing SonarQube issues then we would suggest that you please discuss this with the
TestNG-dev before you spend time working on it.
Note: For more information on contribution guidelines please make sure you refer our Contributing section for detailed set of steps.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Improvements