#2934 Implemented tests for the classpath in tests #2935
Conversation
WalkthroughThe changes in this pull request primarily involve updates to several Java test classes and a Gradle build script. The Changes
Possibly related PRs
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
embedded/build.gradle.kts (2)
19-34: LGTM! Clear dependency organization.The dependencies are now well-organized into logical sections with clear comments. The separation between packaging and compilation dependencies is appropriate.
Consider adding a comment explaining why these modules need to be
compileOnlyvs included in packaging. This would help future maintainers understand the architectural decisions.
Line range hint
44-48: Add error handling for the installer property.While the configuration is correct, the boolean conversion could fail if the property is missing or has an invalid value.
Consider adding error handling:
- create("IC", platformVersionProvider.get(), useInstaller = properties("useInstaller").get().toBoolean()) + create("IC", platformVersionProvider.get(), useInstaller = properties("useInstaller") + .map { it.toBoolean() } + .getOrElse(false) // or whatever default makes sense + )plugin/src/test/java/unit/PerlPluginInstrumentationTest.java (1)
57-76: LGTM with a minor documentation suggestionThe test data entries are consistently updated with the new pattern string parameter.
Consider adding a comment above the data() method explaining the purpose and format of the patternString parameter for better maintainability:
@Parameterized.Parameters(name = "{0}") + /** + * Provides test data in format: [testName, testClass, instrumentationPatternString] + * The patternString is used to filter instrumentation coverage for specific plugin components + */ public static Collection<Object[]> data() {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
embedded/build.gradle.kts(1 hunks)embedded/src/test/java/unit/EmbeddedPerlPluginInstrumentationTest.java(2 hunks)mason/htmlmason/src/test/java/unit/HtmlMasonPluginInstrumentationTest.java(2 hunks)mason/mason2/src/test/java/unit/Mason2PluginInstrumentationTest.java(2 hunks)mojo/src/test/java/unit/MojoPluginInstrumentationTest.java(2 hunks)plugin/src/test/java/unit/PerlPluginInstrumentationTest.java(2 hunks)plugin/src/testFixtures/java/base/PerlInstrumentationTestCase.java(3 hunks)tt2/src/test/java/unit/TemplateToolkitPluginInstrumentationTest.java(2 hunks)
🔇 Additional comments (16)
mojo/src/test/java/unit/MojoPluginInstrumentationTest.java (2)
32-33: LGTM! Constructor changes look good.
The constructor modification properly handles the new patternString parameter and correctly passes it to the superclass.
39-40: Verify pattern string constants are properly defined.
The test data uses PLUGIN_PATTERN_STRING and MOJO_PATTERN_STRING constants. Let's verify these are properly defined in the parent class.
✅ Verification successful
Pattern string constants are properly defined in parent class
Both constants are correctly defined in base/PerlInstrumentationTestCase.java:
PLUGIN_PATTERN_STRING = "/plugin/build/libs/plugin-.+?\\.jar!"MOJO_PATTERN_STRING = "/mojo/core/build/libs/core-.+?\\.jar!"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the pattern string constants are defined in PerlInstrumentationTestCase
# Search for the constant definitions
ast-grep --pattern 'class PerlInstrumentationTestCase {
$$$
$_ PLUGIN_PATTERN_STRING = $_;
$$$
}'
ast-grep --pattern 'class PerlInstrumentationTestCase {
$$$
$_ MOJO_PATTERN_STRING = $_;
$$$
}'
Length of output: 209
Script:
#!/bin/bash
# Let's try a different approach to find these constants
# First, let's find the test file's location and its parent class
fd "MojoPluginInstrumentationTest.java" --exec cat {}
# Then search for pattern string constants in the codebase
rg "PLUGIN_PATTERN_STRING" -A 1
rg "MOJO_PATTERN_STRING" -A 1
Length of output: 6855
tt2/src/test/java/unit/TemplateToolkitPluginInstrumentationTest.java (3)
2-2: LGTM: Copyright year updated appropriately
32-33: LGTM: Constructor signature updated consistently
The constructor changes look good with proper null-safety annotations and clear parameter naming.
Let's verify that all similar test classes have been updated consistently:
#!/bin/bash
# Description: Verify constructor signature consistency across test classes
# Expected: All test classes should have the same constructor parameter pattern
# Search for similar test class constructors
ast-grep --pattern 'class $_ extends PerlInstrumentationTestCase {
$$$
public $_($_, @NotNull Class<?> cls, @NotNull String patternString) {
super($_, $_);
}
$$$
}'39-40: LGTM: Test data updated with proper constant usage
The test data has been properly updated to use the pattern string constants.
Let's verify the constant definitions:
✅ Verification successful
Constants are properly defined and used consistently across test classes
Both constants are correctly defined in PerlInstrumentationTestCase:
PLUGIN_PATTERN_STRING = "/plugin/build/libs/plugin-.+?\\.jar!"TT2_PATTERN_STRING = "/tt2/core/build/libs/core-.+?\\.jar!"
The constants are used consistently across multiple test classes including TemplateToolkitPluginInstrumentationTest, MojoPluginInstrumentationTest, PerlPluginInstrumentationTest, and others, following the same pattern for jar file path matching.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify pattern string constants are properly defined
# Expected: Constants should be defined in PerlInstrumentationTestCase
# Search for the constant definitions
ast-grep --pattern 'class PerlInstrumentationTestCase {
$$$
public static final String PLUGIN_PATTERN_STRING = $_;
$$$
public static final String TT2_PATTERN_STRING = $_;
$$$
}'
Length of output: 189
Script:
#!/bin/bash
# Let's try to find the file containing PerlInstrumentationTestCase first
fd "PerlInstrumentationTestCase.java"
# Let's also search for these constants in any Java file
rg "PLUGIN_PATTERN_STRING|TT2_PATTERN_STRING" --type java -A 1
Length of output: 5308
embedded/src/test/java/unit/EmbeddedPerlPluginInstrumentationTest.java (1)
32-33: LGTM! Constructor changes look good.
The constructor signature update and super call are properly implemented with appropriate null-safety annotations.
embedded/build.gradle.kts (2)
18-18: LGTM! Well-structured property accessor.
The function follows Gradle best practices by using the providers API for type-safe property access.
36-43: LGTM! Proper test and plugin dependency configuration.
The use of testFixtures and local plugin configuration follows good practices for test organization and plugin development.
mason/mason2/src/test/java/unit/Mason2PluginInstrumentationTest.java (2)
32-33: LGTM! Constructor changes look good.
The constructor signature update properly includes the new patternString parameter with appropriate @NotNull annotation and correctly passes it to the superclass.
39-41: LGTM! Test data updated correctly.
The test data has been properly updated to include the pattern string constants for each test case.
Let's verify that these constants are properly defined in the parent class:
✅ Verification successful
Constants are properly defined in the parent class
All pattern string constants used in Mason2PluginInstrumentationTest.java are correctly defined in PerlInstrumentationTestCase.java:
PLUGIN_PATTERN_STRING = "/plugin/build/libs/plugin-.+?\\.jar!"MASON_FRAMEWORK_PATTERN_STRING = "/mason/framework/build/libs/lang\\.mason\\.framework-.+?\\.jar!"MASON2_PATTERN_STRING = "/mason/mason2/core/build/libs/core-.+?\\.jar!"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify pattern string constants in PerlInstrumentationTestCase
# Expected: Find definitions for all pattern string constants used in the test data
rg -A 1 "PLUGIN_PATTERN_STRING|MASON_FRAMEWORK_PATTERN_STRING|MASON2_PATTERN_STRING" --type java
Length of output: 5782
mason/htmlmason/src/test/java/unit/HtmlMasonPluginInstrumentationTest.java (3)
2-2: LGTM: Copyright year updated appropriately
39-41: Verify pattern string constants are properly defined
The test data uses three different pattern string constants. Let's ensure they are properly defined and documented.
✅ Verification successful
Pattern string constants are properly defined and used correctly
All three constants are defined in the parent class PerlInstrumentationTestCase with clear jar pattern values:
PLUGIN_PATTERN_STRING = "/plugin/build/libs/plugin-.+?\\.jar!"MASON_FRAMEWORK_PATTERN_STRING = "/mason/framework/build/libs/lang\\.mason\\.framework-.+?\\.jar!"MASON_PATTERN_STRING = "/mason/htmlmason/core/build/libs/core-.+?\\.jar!"
The constants are used consistently across multiple test files including HtmlMasonPluginInstrumentationTest, Mason2PluginInstrumentationTest, and others.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify pattern string constants in PerlInstrumentationTestCase
# Expected: Constants should be defined with clear values
# Search for the constant definitions
ast-grep --pattern 'class PerlInstrumentationTestCase {
$$$
static final String PLUGIN_PATTERN_STRING = $_;
$$$
static final String MASON_FRAMEWORK_PATTERN_STRING = $_;
$$$
static final String MASON_PATTERN_STRING = $_;
$$$
}'
Length of output: 242
Script:
#!/bin/bash
# Let's try a different approach to find these constants
# First, let's find the test case file
fd "PerlInstrumentationTestCase.java"
# Then search for any pattern string constants in the codebase
rg "PLUGIN_PATTERN_STRING|MASON_FRAMEWORK_PATTERN_STRING|MASON_PATTERN_STRING" -A 1
# Also search in the specific test file we're reviewing
fd "HtmlMasonPluginInstrumentationTest.java" --exec rg -A 5 "PATTERN_STRING"
Length of output: 6133
32-33: LGTM: Constructor properly updated with pattern string parameter
The changes follow good practices with proper null-safety annotations and correct super() call.
Let's verify consistency across related test classes:
✅ Verification successful
LGTM: Constructor pattern is consistent across plugin instrumentation tests
The constructor signature change is consistent with all other plugin instrumentation test classes in the codebase. All related test classes have the same constructor pattern with @NotNull String ignoredName, @NotNull Class<?> cls, @NotNull String patternString parameters and properly pass them to the superclass.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all related test classes have been updated with the same pattern
# Expected: All test classes should show similar constructor patterns
# Search for similar test class constructors
ast-grep --pattern 'class $_ extends PerlInstrumentationTestCase {
$$$
public $_($_, $_, $_) {
super($_, $_);
}
}'
Length of output: 242
Script:
#!/bin/bash
# Let's try a different approach to find test classes and their constructors
# First, find all test files
fd "Test.java" | while read -r file; do
echo "=== $file ==="
# Show class definition and constructor
rg "class.*Test.*extends.*PerlInstrumentationTestCase|public.*Test.*\(" "$file" -A 3
done
Length of output: 444899
Script:
#!/bin/bash
# Let's look for related test classes that extend PerlInstrumentationTestCase and have similar constructor pattern
rg -l "extends PerlInstrumentationTestCase" | while read -r file; do
echo "=== $file ==="
rg "public.*\(@NotNull String.*@NotNull Class.*@NotNull String" "$file" -A 2
done
Length of output: 2918
plugin/src/testFixtures/java/base/PerlInstrumentationTestCase.java (1)
42-49: LGTM! Well-structured changes to support pattern matching.
The addition of the pattern matching field and constructor parameter is well implemented:
- Pattern is compiled once at construction
- Fields are properly marked as final and NotNull
- Good encapsulation with private fields
plugin/src/test/java/unit/PerlPluginInstrumentationTest.java (2)
2-2: LGTM: Copyright year updated appropriately
50-51: LGTM: Constructor updated correctly with null-safety
The constructor changes look good with proper null-safety annotations and consistent parameter naming.
Let's verify that all test classes extending PerlInstrumentationTestCase have been updated:
✅ Verification successful
Let me verify the constructor signatures to ensure they all have the new parameter.
Constructor changes are consistently applied across all subclasses
All test classes extending PerlInstrumentationTestCase have been properly updated with the new constructor signature, including:
- Consistent @NotNull annotations
- Same parameter list: ignoredName, cls, patternString
- Correct super constructor calls
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all test classes extending PerlInstrumentationTestCase have updated constructors
# Expected: All constructors should have the new patternString parameter
# Search for classes extending PerlInstrumentationTestCase
ast-grep --pattern 'class $_ extends PerlInstrumentationTestCase {
$$$
}'
# Search for constructor implementations
rg -l "extends PerlInstrumentationTestCase" | xargs rg "super\(.*\)"
Length of output: 738
Script:
#!/bin/bash
# Search for constructor declarations in files extending PerlInstrumentationTestCase
rg -l "extends PerlInstrumentationTestCase" | xargs rg "public.*\(.*\).*\{" -A 1
Length of output: 3209
| {"plugin.core", PerlParserDefinition.class, PLUGIN_PATTERN_STRING}, | ||
| {"embedded", EmbeddedPerlParserDefinition.class, EMBEDDED_PATTERN_STRING} |
There was a problem hiding this comment.
💡 Codebase verification
Constants are properly defined but EMBEDDED_PATTERN_STRING needs attention
The constants are defined in the parent class PerlInstrumentationTestCase, but there's a commented issue with EMBEDDED_PATTERN_STRING:
PLUGIN_PATTERN_STRING = "/plugin/build/libs/plugin-.+?\\.jar!"EMBEDDED_PATTERN_STRING = "/embedded/core/build/libs/core-.+?\\.jar"(missing!suffix and has incorrect path according to comment)
The comment states: "this is wrong, should be build/libs/lang\.embedded-.+?\.jar!"
🔗 Analysis chain
LGTM! Test data structure properly updated.
The test data array has been correctly extended to include the pattern strings.
Let's verify the pattern string constants are properly defined in the parent class:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that PLUGIN_PATTERN_STRING and EMBEDDED_PATTERN_STRING constants exist in PerlInstrumentationTestCase
# Search for the constant definitions
ast-grep --pattern 'class PerlInstrumentationTestCase {
$$$
$_ PLUGIN_PATTERN_STRING = $_;
$$$
$_ EMBEDDED_PATTERN_STRING = $_;
$$$
}'
Length of output: 146
Script:
#!/bin/bash
# Let's try a different approach to find these constants
# First, let's find the parent test class file
fd -e java PerlInstrumentationTestCase
# Then let's search for the constants with ripgrep for better context
rg "PLUGIN_PATTERN_STRING|EMBEDDED_PATTERN_STRING" -A 2 -B 2
Length of output: 7561
| @Test | ||
| public void testProperClassSource() { | ||
| var className = myClass.getName(); | ||
| var classPath = myClass.getResource("/" + className.replace('.', '/') + ".class").getPath(); | ||
| if (!myClassPathPattern.matcher(classPath).find()) { | ||
| fail("Classpath for the " + className + " is expected to match " + myClassPathPattern + ", but was " + classPath); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving test robustness and error reporting
The new test method is good, but could be enhanced in two ways:
- The error message could include an example of what the pattern would match
- The path comparison might fail on Windows due to different path separators
Consider this enhancement:
@Test
public void testProperClassSource() {
var className = myClass.getName();
- var classPath = myClass.getResource("/" + className.replace('.', '/') + ".class").getPath();
+ var classPath = myClass.getResource("/" + className.replace('.', '/') + ".class")
+ .getPath().replace('\\', '/'); // Normalize path separators
if (!myClassPathPattern.matcher(classPath).find()) {
- fail("Classpath for the " + className + " is expected to match " + myClassPathPattern + ", but was " + classPath);
+ fail(String.format("Classpath for %s is invalid.%nExpected pattern: %s%nActual path: %s%nExample valid path: %s",
+ className, myClassPathPattern, classPath,
+ "/plugin/build/libs/plugin-1.0.0.jar!"));
}
}Committable suggestion skipped: line range outside the PR's diff.
| protected static final String PLUGIN_PATTERN_STRING = "/plugin/build/libs/plugin-.+?\\.jar!"; | ||
| // this is wrong, should be build/libs/lang\\.embedded-.+?\\.jar! | ||
| protected static final String EMBEDDED_PATTERN_STRING = "/embedded/core/build/libs/core-.+?\\.jar"; | ||
| protected static final String MOJO_PATTERN_STRING = "/mojo/core/build/libs/core-.+?\\.jar!"; | ||
| protected static final String TT2_PATTERN_STRING = "/tt2/core/build/libs/core-.+?\\.jar!"; | ||
| protected static final String MASON_FRAMEWORK_PATTERN_STRING = "/mason/framework/build/libs/lang\\.mason\\.framework-.+?\\.jar!"; | ||
| protected static final String MASON_PATTERN_STRING = "/mason/htmlmason/core/build/libs/core-.+?\\.jar!"; | ||
| protected static final String MASON2_PATTERN_STRING = "/mason/mason2/core/build/libs/core-.+?\\.jar!"; |
There was a problem hiding this comment.
Fix incorrect pattern for EMBEDDED_PATTERN_STRING
As noted in the comment, the pattern for embedded library path is incorrect. It's missing the '!' terminator and uses incorrect path structure.
Apply this fix:
- protected static final String EMBEDDED_PATTERN_STRING = "/embedded/core/build/libs/core-.+?\\.jar";
+ protected static final String EMBEDDED_PATTERN_STRING = "/embedded/build/libs/lang\\.embedded-.+?\\.jar!";📝 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 static final String PLUGIN_PATTERN_STRING = "/plugin/build/libs/plugin-.+?\\.jar!"; | |
| // this is wrong, should be build/libs/lang\\.embedded-.+?\\.jar! | |
| protected static final String EMBEDDED_PATTERN_STRING = "/embedded/core/build/libs/core-.+?\\.jar"; | |
| protected static final String MOJO_PATTERN_STRING = "/mojo/core/build/libs/core-.+?\\.jar!"; | |
| protected static final String TT2_PATTERN_STRING = "/tt2/core/build/libs/core-.+?\\.jar!"; | |
| protected static final String MASON_FRAMEWORK_PATTERN_STRING = "/mason/framework/build/libs/lang\\.mason\\.framework-.+?\\.jar!"; | |
| protected static final String MASON_PATTERN_STRING = "/mason/htmlmason/core/build/libs/core-.+?\\.jar!"; | |
| protected static final String MASON2_PATTERN_STRING = "/mason/mason2/core/build/libs/core-.+?\\.jar!"; | |
| protected static final String PLUGIN_PATTERN_STRING = "/plugin/build/libs/plugin-.+?\\.jar!"; | |
| // this is wrong, should be build/libs/lang\\.embedded-.+?\\.jar! | |
| protected static final String EMBEDDED_PATTERN_STRING = "/embedded/build/libs/lang\\.embedded-.+?\\.jar!"; | |
| protected static final String MOJO_PATTERN_STRING = "/mojo/core/build/libs/core-.+?\\.jar!"; | |
| protected static final String TT2_PATTERN_STRING = "/tt2/core/build/libs/core-.+?\\.jar!"; | |
| protected static final String MASON_FRAMEWORK_PATTERN_STRING = "/mason/framework/build/libs/lang\\.mason\\.framework-.+?\\.jar!"; | |
| protected static final String MASON_PATTERN_STRING = "/mason/htmlmason/core/build/libs/core-.+?\\.jar!"; | |
| protected static final String MASON2_PATTERN_STRING = "/mason/mason2/core/build/libs/core-.+?\\.jar!"; |
Qodana for JVM50 new problems were found
💡 Qodana analysis was run in the pull request mode: only the changed files were checked Detected 11 dependenciesThird-party software listThis page lists the third-party software dependencies used in perl5
Contact Qodana teamContact us at [email protected]
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -29,15 +29,15 @@ | |||
| @RunWith(Parameterized.class) | |||
| public class EmbeddedPerlPluginInstrumentationTest extends PerlInstrumentationTestCase { | |||
|
|
|||
| public EmbeddedPerlPluginInstrumentationTest(@NotNull String ignoredName, @NotNull Class<?> cls) { | |||
| super(cls); | |||
| public EmbeddedPerlPluginInstrumentationTest(@NotNull String ignoredName, @NotNull Class<?> cls, @NotNull String patternString) { | |||
Check warning
Code scanning / QDJVM
Unused declaration Warning test
| @@ -29,16 +29,16 @@ | |||
|
|
|||
| @RunWith(Parameterized.class) | |||
| public class HtmlMasonPluginInstrumentationTest extends PerlInstrumentationTestCase { | |||
| public HtmlMasonPluginInstrumentationTest(@NotNull String ignoredName, @NotNull Class<?> cls) { | |||
| super(cls); | |||
| public HtmlMasonPluginInstrumentationTest(@NotNull String ignoredName, @NotNull Class<?> cls, @NotNull String patternString) { | |||
Check warning
Code scanning / QDJVM
Unused declaration Warning test
| @@ -29,16 +29,16 @@ | |||
|
|
|||
| @RunWith(Parameterized.class) | |||
| public class Mason2PluginInstrumentationTest extends PerlInstrumentationTestCase { | |||
| public Mason2PluginInstrumentationTest(@NotNull String ignoredName, @NotNull Class<?> cls) { | |||
| super(cls); | |||
| public Mason2PluginInstrumentationTest(@NotNull String ignoredName, @NotNull Class<?> cls, @NotNull String patternString) { | |||
Check warning
Code scanning / QDJVM
Unused declaration Warning test
| @@ -29,15 +29,15 @@ | |||
| @RunWith(Parameterized.class) | |||
| public class MojoPluginInstrumentationTest extends PerlInstrumentationTestCase { | |||
|
|
|||
| public MojoPluginInstrumentationTest(@NotNull String ignoredName, @NotNull Class<?> cls) { | |||
| super(cls); | |||
| public MojoPluginInstrumentationTest(@NotNull String ignoredName, @NotNull Class<?> cls, @NotNull String patternString) { | |||
Check warning
Code scanning / QDJVM
Unused declaration Warning test
| @@ -47,33 +47,33 @@ | |||
| @RunWith(Parameterized.class) | |||
| public class PerlPluginInstrumentationTest extends PerlInstrumentationTestCase { | |||
|
|
|||
| public PerlPluginInstrumentationTest(@NotNull String ignoredName, @NotNull Class<?> cls) { | |||
| super(cls); | |||
| public PerlPluginInstrumentationTest(@NotNull String ignoredName, @NotNull Class<?> cls, @NotNull String patternString) { | |||
Check warning
Code scanning / QDJVM
Unused declaration Warning test
| @@ -29,15 +29,15 @@ | |||
| @RunWith(Parameterized.class) | |||
| public class TemplateToolkitPluginInstrumentationTest extends PerlInstrumentationTestCase { | |||
|
|
|||
| public TemplateToolkitPluginInstrumentationTest(@NotNull String ignoredName, @NotNull Class<?> cls) { | |||
| super(cls); | |||
| public TemplateToolkitPluginInstrumentationTest(@NotNull String ignoredName, @NotNull Class<?> cls, @NotNull String patternString) { | |||
Check warning
Code scanning / QDJVM
Unused declaration Warning test
|
Summary by CodeRabbit
New Features
Bug Fixes
Refactor