Skip to content

Conversation

@pranavgaikwad
Copy link
Contributor

@pranavgaikwad pranavgaikwad commented Dec 19, 2025

This adds infrastructure to run unit tests against an actual java project added to search scope.
Also adds an abstract test that we can re-use to add test cases for all locations.
Runs the new tests under existing mvn integration-test command.

Summary by CodeRabbit

  • Bug Fixes

    • Improved symbol matching to better align search results with AST node boundaries.
  • New Features

    • Enhanced handling of same-package symbol references.
    • Expanded wildcard qualification behavior for symbol queries.
  • Tests

    • Added integration tests covering method declarations and calls (fully-qualified, non-qualified, wildcard).
    • Introduced test helpers and project-management utilities for Java/Maven integration testing.
  • Chores

    • Updated test packaging and build settings; increased test JVM timeout and enabled richer test output.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 19, 2025

Walkthrough

Refactors AST match boundary logic and symbol qualification handling; adds a test infrastructure (project manager, search helper, abstract test base), new integration tests, small demo additions, and tweaks to test packaging and Tycho Surefire configuration.

Changes

Cohort / File(s) Summary
AST & Qualification Logic
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java, java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/SymbolProvider.java
CustomASTVisitor: changed visit eligibility from exact end-position equality to inclusive boundary checks (match within node span or aligning with node end). SymbolProvider: deferred wildcard expansion until after qualification extraction; added same-package reference handling and informational logging.
Test Framework & Helpers
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/testing/AbstractSymbolProviderTest.java, java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/testing/SearchTestHelper.java, java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/testing/TestProjectManager.java
Added AbstractSymbolProviderTest for lifecycle and common search helpers; SearchTestHelper to execute RuleEntry-based searches and filter/format results; TestProjectManager to import, wait-for-ready, and clean up Maven test projects.
Integration Tests
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/symbol/MethodCallSymbolProviderIntegrationTest.java, java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/symbol/MethodDeclarationSymbolProviderIntegrationTest.java
Added integration test suites that validate method call and declaration searches across fully-qualified, half-qualified, non-qualified, and wildcard patterns.
Test Project Sources
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/SampleApplication.java, java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/ServletExample.java
Added public merge(Object) methods in both classes and public static void callFullyQualifiedMethod() in SampleApplication to exercise fully-qualified call patterns.
Test Packaging & Build
java-analyzer-bundle.test/META-INF/MANIFEST.MF, java-analyzer-bundle.test/build.properties, pom.xml
Manifest: added org.eclipse.m2e.maven.runtime Require-Bundle entry. build.properties: included projects/ in bin.includes. pom.xml: increased Tycho Surefire fork timeout and enabled test stdout/stderr and summary printing.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Test as Test Runner
    participant TPM as TestProjectManager
    participant M2E as M2E/Maven Import
    participant WS as Workspace/JDT
    participant Search as SearchTestHelper / RuleEntry
    participant Provider as SymbolProvider

    Test->>TPM: request importMavenProject(projectName)
    TPM->>M2E: copy & invoke import
    M2E-->>TPM: import result (IJavaProject)
    TPM->>WS: waitForProjectReady(project)
    WS-->>TPM: indexing/builds complete
    Test->>Search: executeSearch(query, location,...)
    Search->>Provider: run RuleEntry search against project/JDT indexes
    Provider-->>Search: SymbolInformation results
    Search-->>Test: filtered/formatted results
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

  • Pay special attention to:
    • Boundary logic in CustomASTVisitor (off-by-one and edge-case positions).
    • Qualification/wildcard changes in SymbolProvider (ordering and same-package handling).
    • Concurrency and timeout handling in TestProjectManager (import, waiting, cleanup).
    • Integration of new test helpers with existing test lifecycle and CI (pom/tycho changes).

Possibly related PRs

Suggested labels

cherry-pick/release-0.8

Suggested reviewers

  • jmle
  • eemcmullan
  • shawn-hurley

Poem

🐇 In burrows of code I hop and I test,
Projects imported, built, and all blessed.
Matches widened, qualifications tuned,
Tests spring to life — the search is on-moond!
Hop! 🥕

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title references adding integration tests and fixing a bug in method calls, which aligns with the primary changes: new test infrastructure and a bug fix in the SymbolProvider's method-handling logic.
Docstring Coverage ✅ Passed Docstring coverage is 84.38% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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

❤️ Share

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

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (6)
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/symbol/MethodCallSymbolProviderIntegrationTest.java (3)

3-4: Remove unused import.

assertFalse is imported but never used in this test class.

🔎 Proposed fix
-import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;

21-22: Consider using assertEquals for better failure messages.

Using assertEquals(1, results.size()) provides clearer failure output than assertTrue(results.size() == 1). This applies to all similar assertions in both test methods.

🔎 Proposed fix example
-        assertTrue("[1] Should find usage of merge() call in test-project/SampleApplication.java#callFullyQualifiedMethod()", 
-                    results.size() == 1);
+        assertEquals("[1] Should find usage of merge() call in test-project/SampleApplication.java#callFullyQualifiedMethod()", 
+                     1, results.size());

58-58: Fix typo: "pattterns" → "patterns"

🔎 Proposed fix
-        // make sure pattterns work
+        // make sure patterns work
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/testing/TestProjectManager.java (2)

69-69: Consider thread-safe collection for importedProjects.

ArrayList is not thread-safe. If tests ever run in parallel or cleanup is triggered from a different thread, this could cause ConcurrentModificationException in cleanupAllTestProjects().

🔎 Proposed fix
-    private static final List<String> importedProjects = new ArrayList<>();
+    private static final List<String> importedProjects = Collections.synchronizedList(new ArrayList<>());

276-282: The fixed 500ms sleep could be flaky on slower systems.

Consider making this configurable or using a more robust synchronization mechanism that checks indexer state.

java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/testing/SearchTestHelper.java (1)

165-174: Consider validating the command result type.

The unchecked cast assumes commandHandler.executeCommand returns a List<SymbolInformation>, but if it returns an unexpected type, the cast will fail with a ClassCastException. Consider adding a type check or catching ClassCastException specifically to provide a clearer error message.

🔎 Proposed defensive cast with validation
 Object result = commandHandler.executeCommand(
     SampleDelegateCommandHandler.RULE_ENTRY_COMMAND_ID,
     arguments,
     null // IProgressMonitor
 );
 
+if (!(result instanceof List<?>)) {
+    throw new RuntimeException("Command returned unexpected type: " + 
+        (result == null ? "null" : result.getClass().getName()));
+}
+
 List<SymbolInformation> symbols = (List<SymbolInformation>) result;
 logInfo("Search returned " + symbols.size() + " results");
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 76df9a5 and a75364c.

📒 Files selected for processing (12)
  • java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java (1 hunks)
  • java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/SymbolProvider.java (2 hunks)
  • java-analyzer-bundle.test/META-INF/MANIFEST.MF (1 hunks)
  • java-analyzer-bundle.test/build.properties (1 hunks)
  • java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/SampleApplication.java (1 hunks)
  • java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/ServletExample.java (1 hunks)
  • java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/symbol/MethodCallSymbolProviderIntegrationTest.java (1 hunks)
  • java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/symbol/MethodDeclarationSymbolProviderIntegrationTest.java (1 hunks)
  • java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/testing/AbstractSymbolProviderTest.java (1 hunks)
  • java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/testing/SearchTestHelper.java (1 hunks)
  • java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/testing/TestProjectManager.java (1 hunks)
  • pom.xml (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-21T16:47:49.579Z
Learnt from: shawn-hurley
Repo: konveyor/java-analyzer-bundle PR: 165
File: java-analyzer-bundle.test/projects/customers-tomcat-legacy/pom.xml:117-119
Timestamp: 2025-11-21T16:47:49.579Z
Learning: In the java-analyzer-bundle repository, test projects under `java-analyzer-bundle.test/projects/` (such as `customers-tomcat-legacy`) intentionally use legacy and vulnerable dependencies to provide realistic migration test fixtures. These vulnerable versions are required so that MTA (Migration Toolkit for Applications) rules can be validated for their ability to detect such issues. Do not flag vulnerable dependencies in these test fixture projects as security issues.

Applied to files:

  • java-analyzer-bundle.test/META-INF/MANIFEST.MF
  • java-analyzer-bundle.test/build.properties
  • java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/testing/AbstractSymbolProviderTest.java
  • java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/testing/TestProjectManager.java
🧬 Code graph analysis (1)
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/symbol/MethodCallSymbolProviderIntegrationTest.java (1)
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/testing/AbstractSymbolProviderTest.java (1)
  • AbstractSymbolProviderTest (40-314)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Build tackle2-addon-analyzer
  • GitHub Check: Unit Tests (Phase 1)
🔇 Additional comments (20)
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/ServletExample.java (1)

30-35: Verify the JavaDoc reference to SampleApplication#callFullyQualifiedMethod() exists.

The JavaDoc references SampleApplication#callFullyQualifiedMethod(). Ensure this method exists in the SampleApplication class to avoid a broken documentation link. Additionally, consider enhancing the JavaDoc to clarify that this method serves as a test fixture for symbol resolution validation, as the current documentation doesn't explain the test purpose.

java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java (1)

76-82: LGTM! Helpful clarification.

The comment block clearly explains the boundary handling logic and provides useful context about why this check is necessary.

java-analyzer-bundle.test/META-INF/MANIFEST.MF (1)

13-14: LGTM!

The additional bundle dependencies (org.eclipse.buildship.core and org.eclipse.m2e.maven.runtime) are appropriately added to support the new Maven project import functionality in TestProjectManager.

java-analyzer-bundle.test/build.properties (1)

4-5: LGTM!

Adding projects/ to bin.includes ensures test fixture projects are packaged and available at runtime for the integration tests.

pom.xml (1)

73-77: LGTM!

The increased timeout (60→240 seconds) accommodates the new integration tests that require Maven project import and JDT indexing. Direct console output and test summary improve debuggability.

java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/SymbolProvider.java (2)

218-219: Good fix: Deferred wildcard expansion.

Moving the wildcard replacement (* → .*) after computing queryQualification ensures the qualification is extracted from the original query before pattern transformation. This fixes potential issues where * in the query would interfere with qualification extraction.


279-287: Verify: Permissive same-package matching may increase false positives.

The new logic returns true for any query where queryQualification has no dots (e.g., Foo.bar). While this correctly handles same-package references, it may significantly increase the number of matches passed to the AST visitor for filtering.

Ensure the AST visitor can efficiently handle increased candidate matches, especially for queries like SomeClass.method* which will now match in any compilation unit.

java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/SampleApplication.java (2)

98-107: LGTM!

The new methods provide good test fixtures for verifying fully-qualified method call searches. The comment clearly explains the testing purpose.


109-114: LGTM!

Simple merge method for testing method declaration searches across multiple classes.

java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/symbol/MethodDeclarationSymbolProviderIntegrationTest.java (1)

13-111: LGTM! Comprehensive test coverage for method declarations.

The tests cover fully-qualified, half-qualified, non-qualified, and pattern-based method declaration searches. Consider the same assertEquals refactor as suggested for MethodCallSymbolProviderIntegrationTest to improve failure diagnostics.

java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/testing/TestProjectManager.java (2)

144-158: Consider handling multiple Maven modules properly.

The loop breaks after finding the first imported project. If the test project has submodules, this might return the parent project instead of the expected module. Verify this is the intended behavior for multi-module projects.


59-472: Well-structured test infrastructure utility.

The TestProjectManager provides a comprehensive solution for test project lifecycle management with:

  • Robust path resolution with multiple fallbacks
  • Proper cleanup of both workspace and filesystem resources
  • Extensive logging for debugging test issues
  • Configurable workspace root paths for JDT/LS integration
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/testing/SearchTestHelper.java (3)

53-67: LGTM! Well-defined location type constants.

The location type constants are clearly named and well-documented in the class-level javadoc, making them easy to understand and use in tests.


190-228: LGTM! Null-safe filtering methods.

All filtering methods properly handle null checks before accessing nested properties or calling methods on potentially null values.


330-336: LGTM! Appropriate fallback logging design.

The dual logging approach with an empty catch block is appropriate here - if the JavaLanguageServerPlugin logging fails (e.g., in a different runtime environment), the method gracefully falls back to console output without breaking tests.

java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/testing/AbstractSymbolProviderTest.java (5)

46-52: LGTM! Static fields appropriate for integration test lifecycle.

The protected static fields support the @BeforeClass/@AfterClass pattern where a single project is imported once per test class and shared across test methods. This design assumes test classes run sequentially (standard JUnit behavior) and is appropriate for these integration tests.


61-87: LGTM! Robust test setup with environment gating.

The setup method properly gates on Eclipse runtime availability using Assume.assumeTrue, which will skip tests gracefully when run outside the required environment. The project reuse optimization (lines 77-82) is a nice touch that avoids redundant imports when multiple test classes share the same project.


92-101: LGTM! Comprehensive runtime detection.

The multi-catch pattern appropriately handles the various failure modes when accessing Eclipse APIs outside an OSGi runtime:

  • IllegalStateException when workspace is not initialized
  • NoClassDefFoundError when Eclipse classes aren't on classpath
  • ExceptionInInitializerError when static initialization fails

126-285: LGTM! Consistent and well-documented search helper methods.

All 16 search helper methods follow a consistent delegation pattern, each targeting a specific location type with clear javadoc examples. The dual searchAnnotations overloads (lines 159-175) properly handle both simple and element-filtered annotation queries.


290-313: LGTM! Convenience wrappers improve test readability.

These short-named delegates provide a more fluent API for test code, making assertions and filtering more concise while maintaining the same functionality as the underlying SearchTestHelper methods.

Comment on lines 84 to 92
private boolean shouldVisit(ASTNode node) {
return (this.match.getOffset() + this.match.getLength()) ==
(node.getStartPosition() + node.getLength());
int matchStart = this.match.getOffset();
int matchEnd = matchStart + this.match.getLength();
int nodeStart = node.getStartPosition();
int nodeEnd = nodeStart + node.getLength();
boolean result = (matchStart >= nodeStart && matchEnd <= nodeEnd) ||
(matchEnd == nodeEnd);
return result;
}
Copy link

@coderabbitai coderabbitai bot Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Logic error: second condition allows matches extending before the node.

The condition (matchEnd == nodeEnd) on line 90 accepts matches that start before the node begins. For example, if matchStart = 10, matchEnd = 20 and nodeStart = 15, nodeEnd = 20, the second condition evaluates to true even though the match extends outside the node's start boundary. This contradicts the comment stating "match falls within the node's range" and could cause incorrect nodes to be visited.

🔎 Proposed fix

If the intent is to check whether the match is fully contained within the node:

-        boolean result = (matchStart >= nodeStart && matchEnd <= nodeEnd) ||
-               (matchEnd == nodeEnd);
+        boolean result = matchStart >= nodeStart && matchEnd <= nodeEnd;

Alternatively, if you need to handle overlapping boundaries where both start and end align:

-        boolean result = (matchStart >= nodeStart && matchEnd <= nodeEnd) ||
-               (matchEnd == nodeEnd);
+        boolean result = (matchStart >= nodeStart && matchEnd <= nodeEnd) ||
+               (matchStart == nodeStart && matchEnd == nodeEnd);

Or if you need to check for any overlap between match and node ranges:

-        boolean result = (matchStart >= nodeStart && matchEnd <= nodeEnd) ||
-               (matchEnd == nodeEnd);
+        boolean result = (matchStart < nodeEnd && matchEnd > nodeStart);

Please clarify the intended boundary semantics and adjust accordingly.

📝 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.

Suggested change
private boolean shouldVisit(ASTNode node) {
return (this.match.getOffset() + this.match.getLength()) ==
(node.getStartPosition() + node.getLength());
int matchStart = this.match.getOffset();
int matchEnd = matchStart + this.match.getLength();
int nodeStart = node.getStartPosition();
int nodeEnd = nodeStart + node.getLength();
boolean result = (matchStart >= nodeStart && matchEnd <= nodeEnd) ||
(matchEnd == nodeEnd);
return result;
}
private boolean shouldVisit(ASTNode node) {
int matchStart = this.match.getOffset();
int matchEnd = matchStart + this.match.getLength();
int nodeStart = node.getStartPosition();
int nodeEnd = nodeStart + node.getLength();
boolean result = matchStart >= nodeStart && matchEnd <= nodeEnd;
return result;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai it is not possible to run into that situation as we will never have a method signature that encloses it container. is there a specific edge case you;re thinking of?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pranavgaikwad, the specific edge case I was concerned about was a scenario where matchStart < nodeStart but matchEnd == nodeEnd (e.g., match starting at offset 10, node starting at 15, both ending at 20). In that case, the second condition would evaluate to true even though the match extends before the node's start boundary.

However, I understand from your explanation that this won't occur in practice because of how JDT structures SearchMatch and AST nodes—the match won't extend outside its containing node's boundaries in this way. If that's based on your testing and understanding of JDT's behavior, then my concern doesn't apply, and the current logic is correct for your use case.

Thank you for the clarification! 🐰


🧠 Learnings used
Learnt from: shawn-hurley
Repo: konveyor/java-analyzer-bundle PR: 165
File: java-analyzer-bundle.test/projects/customers-tomcat-legacy/pom.xml:117-119
Timestamp: 2025-11-21T16:47:49.579Z
Learning: In the java-analyzer-bundle repository, test projects under `java-analyzer-bundle.test/projects/` (such as `customers-tomcat-legacy`) intentionally use legacy and vulnerable dependencies to provide realistic migration test fixtures. These vulnerable versions are required so that MTA (Migration Toolkit for Applications) rules can be validated for their ability to detect such issues. Do not flag vulnerable dependencies in these test fixture projects as security issues.

@pranavgaikwad pranavgaikwad force-pushed the chore/addIntegrationTestWithProjectSetup branch from a75364c to 9f180ff Compare December 19, 2025 19:21
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/symbol/MethodCallSymbolProviderIntegrationTest.java (1)

16-41: Consider using assertEquals for size assertions (optional).

The test logic is correct and covers important scenarios. However, assertions like assertTrue("message", results.size() == 1) could be replaced with assertEquals("message", 1, results.size()) for clearer failure messages that show expected vs. actual values.

🔎 Example refactor
-        assertTrue("[1] Should find usage of merge() call in test-project/SampleApplication.java#callFullyQualifiedMethod()", 
-                    results.size() == 1);
+        assertEquals("[1] Should find usage of merge() call in test-project/SampleApplication.java#callFullyQualifiedMethod()", 
+                     1, results.size());

Apply similar changes to other size assertions throughout the test.

java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/symbol/MethodDeclarationSymbolProviderIntegrationTest.java (1)

20-21: Consider assertEquals for size comparisons.

For size assertions, using assertEquals(message, expected, actual) instead of assertTrue(message, size == expected) would provide clearer failure messages showing the expected vs. actual count when a test fails.

Example:

-assertTrue("[1] Should find merge() declaration in SampleApplication.java", 
-           sampleAppResults.size() == 1);
+assertEquals("[1] Should find merge() declaration in SampleApplication.java", 
+             1, sampleAppResults.size());

This is a minor style improvement and doesn't affect correctness.

Also applies to: 28-29, 36-37, 45-46, 53-54, 65-66, 70-71, 74-75, 78-79, 84-85, 96-97, 102-103, 108-109

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a75364c and 9f180ff.

📒 Files selected for processing (5)
  • java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java (1 hunks)
  • java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/SymbolProvider.java (2 hunks)
  • java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/symbol/MethodCallSymbolProviderIntegrationTest.java (1 hunks)
  • java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/symbol/MethodDeclarationSymbolProviderIntegrationTest.java (1 hunks)
  • java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/testing/SearchTestHelper.java (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java
  • java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/testing/SearchTestHelper.java
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: shawn-hurley
Repo: konveyor/java-analyzer-bundle PR: 165
File: java-analyzer-bundle.test/projects/customers-tomcat-legacy/pom.xml:117-119
Timestamp: 2025-11-21T16:47:49.579Z
Learning: In the java-analyzer-bundle repository, test projects under `java-analyzer-bundle.test/projects/` (such as `customers-tomcat-legacy`) intentionally use legacy and vulnerable dependencies to provide realistic migration test fixtures. These vulnerable versions are required so that MTA (Migration Toolkit for Applications) rules can be validated for their ability to detect such issues. Do not flag vulnerable dependencies in these test fixture projects as security issues.
🧬 Code graph analysis (2)
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/symbol/MethodDeclarationSymbolProviderIntegrationTest.java (1)
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/testing/AbstractSymbolProviderTest.java (1)
  • AbstractSymbolProviderTest (40-314)
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/symbol/MethodCallSymbolProviderIntegrationTest.java (1)
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/testing/AbstractSymbolProviderTest.java (1)
  • AbstractSymbolProviderTest (40-314)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: JDT.LS Integration Tests (Phase 2)
  • GitHub Check: Build tackle2-addon-analyzer
🔇 Additional comments (4)
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/symbol/MethodCallSymbolProviderIntegrationTest.java (1)

1-14: LGTM! Clean test structure.

The class correctly extends AbstractSymbolProviderTest to leverage the test infrastructure, and imports are minimal and appropriate.

java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/symbol/MethodDeclarationSymbolProviderIntegrationTest.java (1)

1-111: Excellent integration test coverage!

The test implementation is comprehensive and well-structured. It effectively validates:

  • Fully qualified method declaration searches with exact matching
  • Half-qualified searches (with helpful comment explaining declaration vs. call behavior)
  • Non-qualified searches returning all matching declarations
  • Various wildcard pattern combinations

The test organization is clear, assertions have descriptive messages, and the use of helper methods from AbstractSymbolProviderTest promotes reusability.

java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/SymbolProvider.java (2)

219-219: Wildcard replacement reordering looks correct.

Moving the regex conversion after queryQualification extraction is sound since the qualification derivation (lines 212-217) uses lastIndexOf('.') which works correctly on the raw query string.


279-287: Same-package reference handling is well-reasoned.

The logic correctly handles cases where a simple class name (without package prefix) may reference a class in the same package that doesn't require an import. Deferring to AST verification is the right approach.

Note that this also covers the case where queryQualification is empty (when the query has no dots at all, like method*), which makes this method more permissive—matching behavior will rely entirely on AST verification. This appears intentional based on the code comments.

assertTrue("[2] Should find usage of merge() call in test-project/PackageUsageExample.java#merge()",
packageExampleResults.size() == 1);

// make sure pattterns work
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Fix typo in comment.

The comment has a typo: "pattterns" should be "patterns".

🔎 Proposed fix
-        // make sure pattterns work
+        // make sure patterns work
📝 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.

Suggested change
// make sure pattterns work
// make sure patterns work
🤖 Prompt for AI Agents
In
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/symbol/MethodCallSymbolProviderIntegrationTest.java
around line 58, the inline comment contains a typo "pattterns"; update the
comment to read "patterns" (e.g., "// make sure patterns work") to correct the
spelling.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant