test: improve osgi tests #3213
test: improve osgi tests #3213
Conversation
WalkthroughThis pull request enhances the OSGi testing framework by introducing new Maven bundle declarations in the OSGi options and additional test methods to validate module loading. The build configuration files, both for TestNG and TestNG OSGi, have been updated to require specific features, bump dependency versions, and refine package import instructions in the manifest. No modifications were made to exported or public entity declarations. Changes
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 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's It looks like Apparently, it breaks as the fields were converted from |
|
@juherr , @krmahadevan , it looks like the change of WDYT of reverting the field back to an array? |
vlsi
force-pushed
the
osgi_tests
branch
2 times, most recently
from
e1e4369 to
a092a59
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
testng-test-osgi/testng-test-osgi-build.gradle.kts (1)
50-52: Added Apache Aries SPI Fly for SLF4J 2.0 compatibility.The inclusion of this dependency is well-justified with a reference to SLF4J 2.0's requirements for OSGi serviceloader processor support.
Consider adding a comment about the specific version choice for Aries SPI Fly, as there might be important compatibility considerations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
testng-test-osgi/src/test/java/org/testng/test/osgi/DefaultTestngOsgiOptions.java(1 hunks)testng-test-osgi/src/test/java/org/testng/test/osgi/PlainOsgiTest.java(2 hunks)testng-test-osgi/testng-test-osgi-build.gradle.kts(1 hunks)testng/testng-build.gradle.kts(1 hunks)
👮 Files not reviewed due to content moderation or server errors (4)
- testng-test-osgi/src/test/java/org/testng/test/osgi/PlainOsgiTest.java
- testng-test-osgi/src/test/java/org/testng/test/osgi/DefaultTestngOsgiOptions.java
- testng/testng-build.gradle.kts
- testng-test-osgi/testng-test-osgi-build.gradle.kts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: 21, liberica, ubuntu, UTC, tr_TR, stress JIT
- GitHub Check: 17, microsoft, windows, UTC, fr_FR
- GitHub Check: 21, temurin, windows, Pacific/Chatham, fr_FR
- GitHub Check: 11, corretto, macos, UTC, de_DE, stress JIT
- GitHub Check: 17, corretto, ubuntu, UTC, fr_FR, stress JIT
🔇 Additional comments (39)
testng-test-osgi/src/test/java/org/testng/test/osgi/PlainOsgiTest.java (11)
47-52: Good addition of Guice module testThis test method verifies that the IModuleFactory interface loads correctly in the OSGi environment. By checking that the methods array is not empty, it confirms the class is properly accessible through the OSGi container.
54-58: Good addition of JCommander testThis test method verifies that the TestNG class can be loaded properly and its fields are accessible. This is important to confirm that JCommander integration with TestNG works correctly in the OSGi environment.
60-69: Comprehensive YAML integration testThis test effectively validates that SnakeYAML is properly integrated with TestNG in the OSGi environment. It tests the full parse-to-object flow, which is more thorough than just checking if classes load.
Beyond just checking if the classes can be loaded, this test confirms the actual functionality works by verifying the parsed suite has the expected name "My_Suite".
7-8: Appropriate imports added for new test functionality.The new imports support the added test methods for OSGi compatibility validation of various TestNG components.
Also applies to: 9-10, 15-16, 19-20
48-52: Well-structured test for Guice module factory compatibility.This test ensures that the
IModuleFactoryinterface is properly loaded in the OSGi environment, which is essential for verifying the integration with Guice.
54-58: Effective validation of JCommander loading.This test ensures TestNG's dependency on JCommander is properly resolved in the OSGi environment, which is critical for command-line argument parsing.
60-69: Comprehensive test for YAML parsing functionality.This test effectively validates that the YAML parser works correctly in the OSGi environment by:
- Creating a parser instance
- Providing a simple YAML input
- Verifying the parsed result
This ensures that TestNG's YAML configuration capabilities function properly within OSGi containers.
7-9: Well-structured import additions.The new imports are organized logically, aligning with the test methods being added. The inclusion of both standard Java imports like
ByteArrayInputStreamandStandardCharsetsalongside TestNG-specific imports likeIModuleFactoryis appropriate.Also applies to: 15-20
48-52: Good test for Guice module loading.This test effectively verifies that the OSGi container can load the
IModuleFactoryinterface properly. The assertion is appropriately minimal - just checking that methods exist indicates the class was loaded successfully without getting into implementation details.
54-58: JCommander loading test is well-implemented.The test correctly verifies that JCommander-dependent TestNG functionality can be loaded in the OSGi environment. Using
getFields()is a non-intrusive way to verify class loading.
60-69: YAML parser test demonstrates end-to-end functionality.This test goes beyond simple class loading by verifying actual YAML parsing functionality works correctly in the OSGi environment. The test includes:
- Creating a parser
- Providing sample YAML input
- Parsing the input
- Verifying the expected result
The use of AssertJ's fluent API with
extracting()makes the assertion clear and concise.testng-test-osgi/src/test/java/org/testng/test/osgi/DefaultTestngOsgiOptions.java (12)
24-24: Good addition of JCommander bundleThis aligns with the new JCommander test, ensuring the dependency is properly loaded in the OSGi container.
26-29: Good addition of core dependencies for testsThe addition of byte-buddy, guice, and snakeyaml bundles provides the necessary dependencies for the newly added test methods in PlainOsgiTest.
29-36: Complete dependency graph for Guice and GuavaYou've properly included all the required transitive dependencies for Guice and Guava, which is essential for OSGi where each bundle must be explicitly declared. This prevents ClassNotFoundExceptions that commonly occur in OSGi environments when transitive dependencies aren't properly declared.
37-37: Good addition of SPI Fly extensionThe Apache Aries SPI Fly dynamic framework extension is essential for SLF4J 2.0 compatibility in OSGi, as it handles the ServiceLoader functionality needed by newer SLF4J versions.
24-24: Added JCommander bundle support.This addition aligns with the new
jcommanderLoadstest inPlainOsgiTest.java.
26-26: Added ByteBuddy bundle support for runtime code generation.ByteBuddy is an important dependency for TestNG's runtime capabilities.
27-28: Added Guice and SnakeYAML bundle support.These additions properly align with the new test methods
guiceModuleFactoryLoadsandyamlLoadsinPlainOsgiTest.java.
29-37: Comprehensive support for Guice dependency tree and service loading.The additions include:
- Guava and its transitive dependencies (failureaccess, listenablefuture)
- JSR-305 annotations
- AOP Alliance for aspect-oriented programming
- Checker Qual for type checking
- Error Prone annotations
- J2ObjC annotations
- Apache Aries SPI Fly for service loading
This ensures that all necessary dependencies for Guice and service loading are available in the OSGi environment.
24-29: Essential bundles added for core functionality.The addition of these core dependencies (JCommander, Byte Buddy, Guice, SnakeYAML) directly supports the new test methods in
PlainOsgiTest.java. These bundles are required for the TestNG framework to function properly in the OSGi environment.
29-36: Comprehensive Guava dependency chain included.The inclusion of the complete Guava dependency chain (guava, failureaccess, listenablefuture, jsr305, checker-qual, error_prone_annotations, j2objc-annotations) demonstrates a thorough approach to dependency management in OSGi. Guava has multiple sub-components that need to be individually declared to ensure proper resolution in the OSGi container.
33-33: AOP Alliance bundle added for Guice compatibility.The addition of the Apache ServiceMix bundle for AOP Alliance is necessary for Guice to function properly in OSGi. This shows an understanding of Guice's transitive dependencies and OSGi requirements.
37-37: SPI Fly extension added for service loading support.The addition of the Apache Aries SPI Fly extension is important for supporting Java's ServiceLoader mechanism in OSGi. This is particularly relevant for SLF4J 2.0 which relies on service loading.
testng/testng-build.gradle.kts (3)
54-60: Improved OSGi import specificationsThe changes to Import-Package declarations are excellent improvements for OSGi compatibility:
- Replacing wildcard imports with specific version requirements
- Adding proper versioning for all imported packages
- Marking optional dependencies with
resolution:=optionalThis makes the bundle more robust in different OSGi environments by clearly expressing dependency requirements.
54-60: Improved OSGi imports with explicit version constraints.The previous wildcard imports have been replaced with more specific package imports and version constraints:
- Added explicit version "1.7.0" for JCommander
- Added SLF4J with version "2.0"
- Specified version "1.2.0" for Guice with optional resolution
- Added specific SnakeYAML packages with version "1.6" and optional resolution
This approach follows OSGi best practices by providing explicit version constraints rather than using wildcards, which helps prevent compatibility issues.
54-60:Details
❓ Verification inconclusive
Import-Package declarations improved with specific versions.
The previous wildcard imports have been replaced with specific version imports, which is a best practice for OSGi manifest declarations:
- JCommander is now pinned to version 1.7.0
- SLF4J is added with version 2.0
- Guice import is properly marked as optional with version 1.2.0
- SnakeYAML imports are more specific, targeting the exact packages used with version 1.6
These changes enhance OSGi compatibility by being more explicit about package dependencies and their versions.
🏁 Script executed:
#!/bin/bash # Check if the version numbers in the Import-Package statements align with the actual dependency versions in build files # Find the JCommander version in build files echo "Checking JCommander version" rg -A 1 "jcommander" --glob "*.gradle.kts" --glob "*.gradle" # Find the SnakeYAML version in build files echo "Checking SnakeYAML version" rg -A 1 "snakeyaml" --glob "*.gradle.kts" --glob "*.gradle" # Find the Guice version in build files echo "Checking Guice version" rg -A 1 "guice" --glob "*.gradle.kts" --glob "*.gradle"Length of output: 2598
OSGi Import-Package Declarations Updated – Verify Version Alignment
The updated manifest now replaces the previous wildcard imports with explicit package imports. This is a sound OSGi best practice that improves clarity over which API versions are exposed. Notable changes include:
- JCommander: Now imported as
com.beust.jcommander;version="1.7.0". (Please double-check that this version is intentional since the core dependency is declared as version 1.83 elsewhere.)- SLF4J: Added with version
"2.0".- Guice: Declared as
com.google.inject;version="1.2.0";resolution:=optional, marking it as optional. (Confirm that this API version aligns with the BOM dependency set to 5.1.0.)- SnakeYAML: The import is now split into three specific packages—
org.yaml.snakeyaml,org.yaml.snakeyaml.nodes, andorg.yaml.snakeyaml.constructor—each set to version"1.6"and marked as optional. (Verify that these version choices work as expected given that the implementation dependency is version 2.2.)These explicit dependency declarations help enforce consistent OSGi behavior. Please verify that the version numbers in these Import-Package statements were chosen intentionally to reflect the correct API versions, even though they differ from the versions used in some dependency declarations in other modules.
testng-test-osgi/testng-test-osgi-build.gradle.kts (13)
19-28: Good use of Gradle capabilities featureThe separation of TestNG dependencies into specific capability requirements (guice and yaml) is a good practice. This explicitly declares which features are needed instead of pulling in all optional dependencies, making the build more maintainable.
43-52: Comprehensive runtime dependency managementGood job adding the necessary runtime dependencies with clear explanations:
- assertj-core for testing assertions
- aopalliance bundle for Guice's OSGi needs
- error_prone_annotations for Guava
- Apache Aries SPI Fly for SLF4J 2.0 ServiceLoader support
The detailed "because" clauses provide excellent documentation for future maintainers.
34-38:Details
❓ Verification inconclusive
Version upgrade for PAX Exam
The upgrade of PAX Exam components from 4.13.x to 4.14.0 is a good improvement. Make sure to check the release notes for any breaking changes.
🌐 Web query:
What changes were introduced in PAX Exam 4.14.0 compared to 4.13.x?Error during web search
Attention: Verify Release Notes for PAX Exam 4.14.0
The upgrade to PAX Exam 4.14.0 is a positive improvement; however, since we couldn’t automatically retrieve the detailed changes compared to 4.13.x, please ensure that you manually review the official release notes for any breaking changes. The code snippet now updates the version numbers correctly, but it remains crucial to verify against the official documentation.
- Confirm that there are no critical changes affecting compatibility.
- If any adjustments are needed based on the release notes, update the configuration accordingly.
19-28: Enhanced dependency granularity with feature requirements.The change appropriately splits the TestNG dependency into separate capabilities with explicit feature requirements for:
- Guice dependency injection support
- YAML parsing support
This granular approach aligns with modular design principles and improves the clarity of dependency relationships.
34-35: Updated PAX Exam dependencies to version 4.14.0.The version update of PAX Exam dependencies from 4.13.x to 4.14.0 ensures the test framework is using recent features and bug fixes.
Also applies to: 37-38
43-46: Added AOP Alliance bundle for Guice OSGi support.The addition of the ServiceMix bundle for AOP Alliance is appropriately justified with a clear comment explaining its necessity for Guice in OSGi environments.
47-49: Added Error Prone annotations for Guava compatibility.The explicit inclusion of Error Prone annotations with version 2.36.0 is correctly justified for Guava compatibility, noting that only recent versions have proper OSGi headers.
41-42:Details
❓ Verification inconclusive
Verify Logback version constraints for long-term compatibility.
The Logback dependencies are pinned to version 1.4.11, which appears to be intentional based on past review comments mentioning a need to prevent upgrades to 1.3+.
Based on the previous review comments, it seems there's a deliberate decision to control Logback versions. Consider documenting this version constraint directly in the code with a comment explaining why this specific version is used and the risks of upgrading.
🌐 Web query:
What is the latest stable version of Logback that is compatible with slf4j 2.0?Error during web search
Action: Document Logback Version Constraint
It looks like the Logback dependencies are intentionally pinned to version 1.4.11—as noted in earlier review comments—to prevent inadvertent upgrades (i.e., avoiding upgrades to 1.3+ that might conflict with SLF4J 2.0 compatibility requirements). Since the web query to confirm "the latest stable version of Logback that is compatible with slf4j 2.0" returned an error, I recommend manually verifying this detail if you plan on revisiting the version constraints.
Recommendations:
- Add documentation: Insert a comment in
testng-test-osgi/testng-test-osgi-build.gradle.ktsnear the dependency declarations that explains why version 1.4.11 is used, including the compatibility concerns with SLF4J 2.0.- Manual verification: If an upgrade is under consideration, please double-check the latest compatible Logback release, as the automated web query could not deliver a definitive answer.
19-28: Granular capabilities requirements for TestNG.The changes improve dependency resolution by specifying exactly which TestNG capabilities are needed:
- The "guice" capability
- The "yaml" capability
This approach aligns with the OSGi principles of modularity and explicit dependency declaration. It ensures that both YAML and Guice functionality are properly made available to the tests.
34-38: Updated PAX Exam versions to 4.14.0.The PAX Exam dependencies have been upgraded from 4.13.x to 4.14.0, which is a good practice to stay current with the testing framework. This version update likely includes bug fixes and improvements for OSGi testing.
43-52: Runtime dependencies added with clear justifications.The new runtime dependencies are well-documented with detailed "because" clauses explaining their purpose:
- AssertJ Core is added for assertions in tests
- ServiceMix bundle for AOP Alliance is needed for Guice in OSGi
- Error Prone annotations are required for Guava
- Apache Aries SPI Fly extension is needed for SLF4J 2.0 service loading
These additions demonstrate a thorough understanding of OSGi dependency requirements and the complex interactions between libraries in an OSGi environment.
41-42: Consider version constraints for Logback.Based on previous review comments, there should be configuration to prevent upgrading Logback to version 1.3+. Consider adding a comment here to clarify this version constraint, referencing the RenovateBot configuration.
testImplementation("ch.qos.logback:logback-core:1.4.11") testImplementation("ch.qos.logback:logback-classic:1.4.11") + // Note: Avoid upgrading Logback to 1.3+ as configured in the RenovateBot rules
50-52: Clear documentation for SPI Fly requirement.The explanation for including the SPI Fly extension is excellent, including both the technical reason (SLF4J 2.0 requiring osgi.serviceloader.processor) and a reference to further information. This kind of documentation helps future maintainers understand why dependencies are included.
testng/testng-build.gradle.kts
Outdated
| *;resolution:=optional | ||
| com.beust.jcommander;version="1.7.0", | ||
| org.slf4j;version="2.0", | ||
| com.google.inject;version="1.2.0";resolution:=optional, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify SLF4J 2.0 compatibility
The import for SLF4J 2.0 is specified, but you should verify that all usage of SLF4J APIs in the codebase is compatible with version 2.0, as there were some breaking changes in the SLF4J API.
🏁 Script executed:
#!/bin/bash
# Check for SLF4J API usage patterns that might be affected by version 2.0 changes
# Look for usage of deprecated methods in SLF4J 2.0
echo "Checking for SLF4J API usage patterns affected by 2.0 changes..."
echo "1. Checking for Logger.warn(String, Throwable) usage (replaced with Logger.warn(String, Object...)):"
rg -l "\.warn\s*\(\s*.*,\s*\w+\s*\)" --type java
echo "2. Checking for LoggerFactory.getILoggerFactory() usage (consider using direct provider APIs instead):"
rg -l "LoggerFactory\.getILoggerFactory\(\)" --type java
echo "3. Checking for slf4j-api imports:"
rg -l "import\s+org\.slf4j\." --type javaLength of output: 925
SLF4J 2.0 Compatibility: Action Needed on Logger.warn Usage
Our verification found one notable issue:
- In testng-core-api/src/main/java/org/testng/log4testng/Logger.java, there is usage of
Logger.warn(String, Throwable). Since SLF4J 2.0 replaces this with a varargs method (Logger.warn(String, Object...)), please confirm that this patterned usage is either fully supported by your SLF4J 2.0 dependencies or update it accordingly. - Usage of
LoggerFactory.getILoggerFactory()and the SLF4J imports appear correct with no additional instances causing concern.
Please review and address the identified usage in Logger.java to ensure compatibility with SLF4J 2.0.
| testImplementation("ch.qos.logback:logback-core:1.4.11") | ||
| testImplementation("ch.qos.logback:logback-classic:1.4.11") |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify logback version constraints
Note that there's a dependency constraint to keep logback below 1.3.0 as mentioned in a previous review comment. The current version 1.4.11 exceeds this constraint.
🏁 Script executed:
#!/bin/bash
# Check if the current logback version conflicts with project requirements
echo "Checking logback version constraints..."
echo "Current version in build file: 1.4.11"
echo "Previous review comment indicated a constraint to stay below 1.3.0"
# Search for any logback-related issues in the code
echo "Checking for logback imports in the codebase:"
rg -l "import.*logback" --type java
# Check for possible compatibility issues with slf4j
echo "Checking for slf4j version to ensure compatibility with logback 1.4.x:"
rg -A 3 "slf4j.*version" --type kotlinLength of output: 1032
Action Required: Logback Version Conflict
The dependency in testng-test-osgi/testng-test-osgi-build.gradle.kts still uses Logback 1.4.11, which violates the previously established constraint that Logback must remain below version 1.3.0. The output of the verification script confirms that this version is in use, and no updates elsewhere in the codebase indicate an intentional change of constraint.
- File:
testng-test-osgi/testng-test-osgi-build.gradle.kts(lines 41-42) - Issue: Using Logback 1.4.11 instead of a version below 1.3.0.
- Action: Please update the Logback dependencies to a version that conforms with the project constraints or provide an updated rationale and adjustments across the project (including compatibility checks with SLF4J as seen in
testng/testng-build.gradle.kts).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
testng-test-osgi/src/test/java/org/testng/test/osgi/PlainOsgiTest.java (2)
53-56: Consider strengthening the JCommander test.The current test verifies that
TestNG.class.getFields()is not empty, which confirms basic class loading but doesn't specifically verify JCommander functionality.- public void jcommanderLoads() { - assertThat(TestNG.class.getFields()).isNotEmpty(); + public void jcommanderLoads() { + // Verify JCommander integration by checking a JCommander-related field or method + assertThat(TestNG.class.getDeclaredField("commander")).isNotNull(); + // Also verify basic class loading + assertThat(TestNG.class.getFields()).isNotEmpty();
48-65: Consider adding documentation for the new test methods.Adding Javadoc comments for each of the new test methods would improve code maintainability by clearly documenting their purpose, similar to the existing comment for the
versionShouldStartWithDigitmethod.+ /** + * Verifies that the Guice module factory interface loads properly in the OSGi environment. + */ @Test public void guiceModuleFactoryLoads() { assertThat(IModuleFactory.class.getMethods()).isNotEmpty(); } + /** + * Verifies that JCommander dependencies load properly in the OSGi environment. + */ @Test public void jcommanderLoads() { assertThat(TestNG.class.getFields()).isNotEmpty(); } + /** + * Verifies that the YAML parser loads and functions correctly in the OSGi environment. + */ @Test public void yamlLoads() { YamlParser parser = new YamlParser(); ByteArrayInputStream input = new ByteArrayInputStream("name: My_Suite\n".getBytes(StandardCharsets.UTF_8)); XmlSuite suite = parser.parse("test.yml", input, false); assertThat(suite).extracting("name").isEqualTo("My_Suite"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
testng-test-osgi/src/test/java/org/testng/test/osgi/DefaultTestngOsgiOptions.java(1 hunks)testng-test-osgi/src/test/java/org/testng/test/osgi/PlainOsgiTest.java(2 hunks)testng-test-osgi/testng-test-osgi-build.gradle.kts(1 hunks)testng/testng-build.gradle.kts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- testng/testng-build.gradle.kts
- testng-test-osgi/src/test/java/org/testng/test/osgi/DefaultTestngOsgiOptions.java
- testng-test-osgi/testng-test-osgi-build.gradle.kts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: 11, zulu, ubuntu, UTC, ru_RU
- GitHub Check: 17, microsoft, ubuntu, Pacific/Chatham, ru_RU, stress JIT
- GitHub Check: 11, microsoft, windows, Pacific/Chatham, fr_FR
- GitHub Check: 11, zulu, windows, UTC, de_DE
- GitHub Check: 21, corretto, windows, UTC, fr_FR, stress JIT
🔇 Additional comments (4)
testng-test-osgi/src/test/java/org/testng/test/osgi/PlainOsgiTest.java (4)
7-9: Appropriate imports added for the new test methods.The new imports for
ByteArrayInputStreamandStandardCharsetsare correctly imported to support the YAML parsing test.
15-16: Required TestNG-related imports added for OSGi tests.The imports for
IModuleFactory,TestNG,YamlParser, andXmlSuiteproperly support the new test methods verifying OSGi bundle loading.Also applies to: 19-20
48-51: Good test for Guice module factory availability.This test appropriately verifies that the
IModuleFactoryinterface is properly loaded in the OSGi environment by ensuring its methods can be accessed.
58-65: Well-implemented YAML parsing test.This test effectively verifies both the availability and basic functionality of the YAML parser in the OSGi environment. It creates a simple YAML string, parses it, and verifies the expected output.
Vlad, Sometime back I had raised a PR to fix this at the pax library itself. My PR: ops4j/org.ops4j.pax.exam#1112 I forgot about it. But it looks like it got accepted a newer version was released which includes this change. |
|
The test |
|
ok, it looks like the never pax-exam does not use the reflection |
|
@vlsi Thanks for the fix. It helps a lot 👍 |
|
It should fix #1678 too |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
testng-test-osgi/testng-test-osgi-build.gradle.kts (1)
41-42:⚠️ Potential issueLogback version exceeds constraint.
The current logback version 1.4.11 exceeds the dependency constraint of < 1.3.0 mentioned in the past review comment.
According to the previous review comment, there should be a constraint to keep logback below version 1.3.0. Please either:
- Downgrade the logback dependencies to a version less than 1.3.0, or
- Update the dependency constraint if using 1.4.x is now appropriate
🧹 Nitpick comments (1)
testng-test-osgi/src/test/java/org/testng/test/osgi/PlainOsgiTest.java (1)
49-52: New test for JCommander loading.The test checks that the TestNG class has fields, confirming that TestNG with JCommander support is accessible and loaded correctly in the OSGi environment.
Consider using a more specific assertion that verifies JCommander functionality directly rather than just checking if TestNG has fields. For example:
- public void jcommanderLoads() { - assertThat(TestNG.class.getFields()).isNotEmpty(); + public void jcommanderLoads() { + // More specific check for JCommander integration + assertThat(TestNG.class.getFields()).isNotEmpty(); + // Verify specific field or method related to JCommander if possible + assertThat(TestNG.class.getDeclaredField("m_jCommander")).isNotNull();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
testng-test-osgi/src/test/java/org/testng/test/osgi/DefaultTestngOsgiOptions.java(1 hunks)testng-test-osgi/src/test/java/org/testng/test/osgi/PlainOsgiTest.java(2 hunks)testng-test-osgi/testng-test-osgi-build.gradle.kts(1 hunks)testng/testng-build.gradle.kts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- testng-test-osgi/src/test/java/org/testng/test/osgi/DefaultTestngOsgiOptions.java
- testng/testng-build.gradle.kts
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: 11, microsoft, macos, Pacific/Chatham, de_DE, stress JIT
- GitHub Check: 21, oracle, ubuntu, UTC, ru_RU
- GitHub Check: 17, temurin, windows, UTC, ru_RU
- GitHub Check: 11, microsoft, same hashcode, ubuntu, Pacific/Chatham, tr_TR
- GitHub Check: 21, oracle, same hashcode, ubuntu, UTC, ru_RU
- GitHub Check: 17, zulu, ubuntu, UTC, de_DE, stress JIT
🔇 Additional comments (9)
testng-test-osgi/src/test/java/org/testng/test/osgi/PlainOsgiTest.java (4)
7-8: New imports added for OSGi module testing.The new imports support the recently added test methods that validate module loading in the OSGi environment, particularly for Guice, JCommander, and YAML functionality.
Also applies to: 14-20
34-42: Improved version string retrieval in test.The test now directly uses
Version.getVersionString()instead of using reflection, which is cleaner and more maintainable. The assertion logic has also been simplified to just check that the version is non-empty and starts with a digit.
44-47: New test for Guice module factory loading.This test verifies that the
IModuleFactoryinterface is properly loaded in the OSGi environment by checking that it has methods, which is essential for confirming Guice integration works correctly.
54-61: New test for YAML parser loading.This test properly verifies YAML functionality by creating a YamlParser, parsing a simple YAML string, and confirming the resulting XmlSuite has the expected name. This is a good thorough test of the YAML component's functionality in the OSGi environment.
testng-test-osgi/testng-test-osgi-build.gradle.kts (5)
19-28: Added capability requirements for Guice and YAML.The feature requirements for "guice" and "yaml" align well with the new test methods introduced in PlainOsgiTest.java that verify these components.
43-46: Added assertj-core and aopalliance dependencies.The addition of assertj-core supports the assertions in the new tests, while the aopalliance bundle provides necessary packages for Guice in OSGi with a proper explanation of why it's needed.
47-49: Added error_prone_annotations dependency.The addition of error_prone_annotations is appropriately justified as being needed for Guava with OSGi headers.
34-38:Details
❓ Verification inconclusive
Updated pax-exam dependencies to version 4.14.0.
The upgrade from 4.13.1/4.13.5 to 4.14.0 for the pax-exam components likely resolves compatibility issues mentioned in the PR objectives, particularly the issue with TestContainerException.
🏁 Script executed:
#!/bin/bash # Check if PAX Exam 4.14.0 resolves the TestContainerException issue echo "Checking PAX Exam changelog or release notes for version 4.14.0" curl -s "https://github.com/ops4j/org.ops4j.pax.exam2/releases/tag/exam-4.14.0" | grep -i -A 10 "changes" || echo "Could not find explicit changelog" # Check for TestContainerException in the codebase echo "Checking for TestContainerException references:" rg "TestContainerException" --type javaLength of output: 462
PAX Exam 4.14.0 Dependency Upgrade – Verification Needed
The updated dependency versions appear to follow the intended upgrade. However, our verification script did not locate explicit changelog details confirming resolution of the TestContainerException issue, nor were any references to TestContainerException found in the codebase. Please manually verify that the upgrade to 4.14.0 indeed addresses the compatibility concerns (including the noted JUnit5 incompatibility) as described in the PR objectives.
- File:
testng-test-osgi/testng-test-osgi-build.gradle.kts(Lines 34–38)- Next Step: Confirm runtime behavior or validate against the pax-exam release notes if available.
50-52:Details
❓ Verification inconclusive
Added SPIFly dynamic framework extension.
The addition of the SPIFly extension is well-justified for slf4j-api 2.0 compatibility in OSGi environments, with reference to a specific solution.
🌐 Web query:
What does osgi.serviceloader.processor capability provide in OSGi for slf4j-api 2.0?Error during web search
Verify SPIFly Extension Addition for SLF4J-api 2.0 Compatibility
The added SPIFly dynamic framework extension at lines 50–52 in testng-test-osgi/testng-test-osgi-build.gradle.kts is intended to provide the necessary
osgi.serviceloader.processorcapability demanded by SLF4J-api 2.0 in OSGi environments. The reference to the StackOverflow discussion (https://stackoverflow.com/a/77867804) supports this choice.A web lookup to further detail the role of
osgi.serviceloader.processorreturned an error, so please ensure through manual verification that the extension fully meets the required behavior in your specific build configuration.
- Verify that the extension version
1.3.7correctly enables the OSGi service loader processing required by SLF4J-api 2.0.- Confirm that the integration maintains compatibility across all target OSGi environments.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
testng-test-osgi/testng-test-osgi-build.gradle.kts (1)
19-28: Consider Consolidating TestNG Dependency Declarations
Both the Guice and YAML capabilities are added via two separatetestImplementation(projects.testng)declarations. If Gradle DSL allows, consider consolidating them into a single dependency declaration to reduce duplication and improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
testng-test-osgi/src/test/java/org/testng/test/osgi/DefaultTestngOsgiOptions.java(1 hunks)testng-test-osgi/src/test/java/org/testng/test/osgi/PlainOsgiTest.java(2 hunks)testng-test-osgi/testng-test-osgi-build.gradle.kts(1 hunks)testng/testng-build.gradle.kts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- testng-test-osgi/src/test/java/org/testng/test/osgi/PlainOsgiTest.java
- testng-test-osgi/src/test/java/org/testng/test/osgi/DefaultTestngOsgiOptions.java
- testng/testng-build.gradle.kts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: 21, temurin, macos, UTC, ru_RU
- GitHub Check: 21, liberica, ubuntu, Pacific/Chatham, ru_RU, stress JIT
- GitHub Check: 11, zulu, windows, America/New_York, ru_RU, stress JIT
- GitHub Check: 11, microsoft, same hashcode, macos, UTC, ru_RU
- GitHub Check: 11, corretto, windows, UTC, ru_RU
🔇 Additional comments (9)
testng-test-osgi/testng-test-osgi-build.gradle.kts (9)
19-23: Add Guice Capability to TestNG Dependency
This block adds a requirement for the"guice"feature on theprojects.testngdependency to ensure that the appropriate Maven bundle is activated for OSGi tests.
24-28: Add YAML Capability to TestNG Dependency
This block adds a requirement for the"yaml"feature to theprojects.testngdependency, aligning with the updated test requirements. Please confirm that these capability requirements align with the corresponding Maven bundle declarations used elsewhere.
34-34: Update pax-exam-container-native Version
Upgrading to version4.14.0aligns with the revised dependencies needed for improved compatibility in the OSGi environment.
37-37: Update pax-exam-testng Dependency Version
The change to version4.14.0is in line with the updated dependency strategy. Ensure integration tests confirm that this update does not introduce regressions.
38-38: Update pax-exam-link-mvn Dependency Version
Bumping this dependency to4.14.0is consistent with the overall update of pax-exam modules. Please verify that module resolution in the OSGi tests remains stable.
43-43: Add AssertJ Runtime Dependency
The addition oforg.assertj:assertj-core:3.23.1enhances the assertion capabilities available in tests, thereby improving test clarity and diagnostics.
44-46: Add AOP Alliance Dependency to Support Guice in OSGi
Introducing theorg.apache.servicemix.bundles:org.apache.servicemix.bundles.aopalliance:1.0_6dependency addresses OSGi header issues fororg.aopalliance.interceptas required by Guice. The rationale provided in thebecauseclause is clear and informative.
47-49: Add Error Prone Annotations Dependency
The runtime inclusion ofcom.google.errorprone:error_prone_annotations:2.36.0is justified by its necessity for Guava, ensuring that only recent versions with proper OSGi headers are used.
50-52: Add Aries Spifly Extension for SLF4J Compatibility
Includingorg.apache.aries.spifly:org.apache.aries.spifly.dynamic.framework.extension:1.3.7helps to bridge the gap for SLF4J API requirements in an OSGi environment, as referenced by the StackOverflow explanation.
|
LGTM |
|
@vlsi @krmahadevan , the CI builds failed on master branch https://github.com/testng-team/testng/actions/runs/13690486049 could you take a look? I need the latest snapshot jar for integration test in testng-p2 repo |
|
@missedone - The failures are due to flaky tests ( I still have not been able to fix them ) and then there is a failure due to JDK 22 not being available. 
|
|
OK, it's not a block to me, I had built and deploy the snapshot to my local maven repo, and verified the testng-p2 |
4 participants
Fixes #1190
Summary by CodeRabbit