✨ Add comprehensive integration test suite with CI/CD pipeline #165
Conversation
WalkthroughAdds a two‑phase CI/local pipeline and container test image, a Go-based JDT.LS integration test framework, many Java symbol-provider enhancements and new providers, two test projects with fixtures, extensive unit/integration tests, updated Dockerfiles/Makefile, and comprehensive test documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant Phase1 as Phase 1 (Unit Tests)
participant Maven as Maven
participant Phase2 as Phase 2 (Integration)
participant Builder as Image Build
participant Container as Container Runtime
participant JDTLS as JDT.LS
participant GoT as Go Tests
GH->>Phase1: push/PR triggers
Phase1->>Maven: mvn clean integration-test
Maven-->>Phase1: surefire reports
Phase1-->>GH: Phase1 complete
GH->>Phase2: continue
Phase2->>Builder: build Dockerfile.test
Builder-->>Phase2: image
Phase2->>Container: run image (Podman/Docker)
Container->>JDTLS: launch server
Container->>GoT: run go test
GoT->>JDTLS: executeCommand / symbol searches
JDTLS-->>GoT: symbol results
GoT-->>Container: test exit code
Container-->>Phase2: results
Phase2-->>GH: Phase2 complete
sequenceDiagram
participant GoClient as Go JDTLS client
participant Dialer as CmdDialer
participant JDTLS as JDT.LS process
participant RPC as JSON-RPC
GoClient->>Dialer: create pipes
GoClient->>JDTLS: start process (cmd)
JDTLS-->>Dialer: stdout/stdin
GoClient->>RPC: connect over Dialer (jsonrpc2)
RPC-->>GoClient: connection established
GoClient->>JDTLS: initialize LSP (initialize/initialized)
JDTLS-->>RPC: initialize result
GoClient->>JDTLS: executeCommand (search) / workspace calls
JDTLS-->>RPC: symbol results
GoClient->>JDTLS: shutdown/exit
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–75 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 22
♻️ Duplicate comments (5)
java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.resources/.history/3a/f0329fb788b00010150f900a41fc8a5f (1)
1-3: Same nested metadata structure issue.See the comment on file
.../history/6e/c0bd9eb788b00010150f900a41fc8a5fregarding the nested.metadata/.metadata/path structure.java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.resources/.history/ca/20a89fb788b00010150f900a41fc8a5f (1)
1-4: Same nested metadata structure issue.See the comment on file
.../history/6e/c0bd9eb788b00010150f900a41fc8a5fregarding the nested.metadata/.metadata/path structure.java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.resources/.history/3f/a0e0a0b788b00010150f900a41fc8a5f (1)
1-6: Same nested metadata structure issue.See the comment on file
.../history/6e/c0bd9eb788b00010150f900a41fc8a5fregarding the nested.metadata/.metadata/path structure.java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.resources/.history/d1/507ad5b688b00010150f900a41fc8a5f (1)
1-8: Same nested metadata structure issue.See the comment on file
.../history/6e/c0bd9eb788b00010150f900a41fc8a5fregarding the nested.metadata/.metadata/path structure.java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.resources/.history/21/b0bec7b688b00010150f900a41fc8a5f (1)
1-5: Same nested metadata structure issue.See the comment on file
.../history/6e/c0bd9eb788b00010150f900a41fc8a5fregarding the nested.metadata/.metadata/path structure.
🧹 Nitpick comments (34)
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/inheritance/CustomException.java (1)
1-21: LGTM! Clean test exception implementation.The custom exception follows standard Java patterns with proper
serialVersionUIDand delegates to superclass constructors appropriately. For a test file focused on inheritance detection, this implementation is well-suited.Optionally, consider adding the fourth common constructor pattern
public CustomException(Throwable cause)for completeness, though it's not essential for the testing purpose.Optional: Add fourth constructor pattern
public CustomException(String message, Throwable cause) { super(message, cause); } + + public CustomException(Throwable cause) { + super(cause); + } }java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/service/ICustomerService.java (1)
8-10: Consider removing redundant modifiers and fixing spacing.Interface methods are implicitly public in Java, so the
publickeyword is redundant. Additionally, line 10 is missing a space between>andfindAll.Apply this diff to improve code style:
- public Customer findById(Long id); + Customer findById(Long id); - public Page<Customer>findAll(Pageable pageable); + Page<Customer> findAll(Pageable pageable);java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/EnumExample.java (1)
28-35: Consider using Optional for null safety.The method returns
nullwhen no matching code is found, which could lead to NPEs in calling code. Consider returningOptional<EnumExample>instead.Apply this diff:
- public static EnumExample fromCode(int code) { + public static Optional<EnumExample> fromCode(int code) { for (EnumExample e : values()) { if (e.code == code) { - return e; + return Optional.of(e); } } - return null; + return Optional.empty(); }Add the import at the top of the file:
import java.util.Optional;java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/resources/persistence.properties (1)
2-4: Externalize database connection details.The JDBC URL contains a hardcoded IP address and the credentials are hardcoded. While this is a test project, consider externalizing these values to environment variables or a separate configuration file to make the tests more portable and maintainable.
java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/service/CustomerService.java (1)
17-18: Consider constructor injection for better testability.While field injection works, constructor injection is the recommended approach for dependency injection as it makes dependencies explicit, supports immutability, and facilitates testing. However, if this legacy pattern is intentional for testing the analyzer's handling of field injection, this can be deferred.
Apply this diff to use constructor injection:
- @Autowired - private CustomerRepository repository; + private final CustomerRepository repository; + + @Autowired + public CustomerService(CustomerRepository repository) { + this.repository = repository; + }java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/model/Customer.java (1)
11-97: Consider implementing equals() and hashCode() for JPA entity.JPA entities should typically implement
equals()andhashCode()methods to ensure proper entity comparison, especially when entities are used in collections or compared across persistence contexts. Without these methods, entity equality is based on object identity rather than business identity.For a JPA entity, consider implementing these methods based on the business key (id in this case):
@Override public boolean equals(Object o) { if (this == o) return true; if (!(o instanceof Customer)) return false; Customer customer = (Customer) o; return id != null && id.equals(customer.id); } @Override public int hashCode() { return getClass().hashCode(); }Note: Using
getClass().hashCode()for the hashCode implementation ensures consistency across proxy instances and prevents issues when the entity hasn't been persisted yet.java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandlerTest.java (3)
6-6: Remove unused import.The
failimport is not used anywhere in the test class.Apply this diff:
-import static org.junit.Assert.fail;
48-61: Strengthen test assertions to verify actual behavior.Most of these integration tests only assert that the result is not null, which provides minimal verification of correctness. Integration tests should validate:
- Expected result count (e.g.,
assertTrue(result.size() > 0)or specific counts)- Presence of specific expected symbols in results
- Result structure and content accuracy
For example,
testRuleEntryWithMethodCallLocationcould verify that results containprintlnmethod calls, ortestRuleEntryWithImportLocationcould verify specific import statements are found.Consider strengthening at least a subset of these tests to verify actual search behavior rather than just non-null responses.
Also applies to: 63-76, 78-91, 93-105, 107-120, 122-135, 137-150, 152-169, 191-204, 206-219, 221-234, 236-249
48-61: Consolidate or differentiate duplicate test coverage.Both
testRuleEntryWithFullMode(lines 48-61) andtestRuleEntryWithWildcardQuery(lines 176-189) test identical parameters:
- query:
"java.io.*"- location:
"10"- project:
"test-project"The only difference is
analysisMode: "full" vs "source-only". Consider:
- Renaming tests to clarify they're testing different analysis modes with the same query, or
- Using distinct queries to provide broader test coverage
Also applies to: 176-189
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/RuleEntryParamsTest.java (4)
3-8: Align on JUnit version and prefer assertThrows over @test(expected).If the repo is on JUnit 5, migrate to Jupiter (org.junit.jupiter.api.*) and use assertThrows. If staying on JUnit 4, target 4.13+ and replace @test(expected=...) for clearer scope and message checks.
Example (JUnit 4.13+):
-@Test(expected = NumberFormatException.class) -public void testInvalidLocationFormat() { - ... - new RuleEntryParams("test-command", arguments); -} +@Test +public void testInvalidLocationFormat() { + assertThrows(NumberFormatException.class, () -> new RuleEntryParams("test-command", arguments)); +}Confirm which JUnit is configured in the build before changing.
Also applies to: 15-15
156-173: Add negative/out-of-range location tests to lock the contract.You validate 0–14; add explicit tests for invalid values (e.g., -1, 99, null/empty) to ensure the API rejects them as intended (exception type/message).
176-200: Strengthen assertions for annotation elements.This only checks type. Add assertions that elements are parsed (count, name/value), or document that elements are ignored if that’s the contract.
Would you like me to propose assertions once the
RuleEntryParams.getAnnotationQuery()API for elements is confirmed?
19-74: Reduce setup duplication with a small helper.Repeated Map/List plumbing obscures intent. Extract a helper that builds base params, then override per test. Keeps tests terse and readable.
Example helper:
private static Map<String,Object> baseParams(String project, String query, String location, String analysisMode) { Map<String,Object> m = new HashMap<>(); m.put("project", project); m.put("query", query); m.put("location", location); m.put("analysisMode", analysisMode); return m; }Also applies to: 76-154, 156-250
java-analyzer-bundle.test/projects/test-project/.settings/org.eclipse.m2e.core.prefs (1)
1-4: Verify that IDE-specific project settings are necessary.Eclipse project
.settingsfiles are IDE-specific and may cause inconsistencies across different development environments and CI/CD pipelines. TheresolveWorkspaceProjects=truesetting (Line 3) in particular assumes an Eclipse workspace structure that may not exist in containerized test environments.If these settings are required for local development, consider documenting the Eclipse setup requirements. Otherwise, these files could be excluded from version control.
java-analyzer-bundle.test/projects/test-project/.settings/org.eclipse.core.resources.prefs (1)
1-3: Consider configuring encoding in the build tool instead.While UTF-8 encoding is correct, IDE-specific settings may not be respected in CI/CD environments or by developers using different IDEs. Consider setting the encoding explicitly in the Maven
pom.xmlto ensure consistency:<properties> <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding> </properties>This ensures UTF-8 encoding is enforced across all build environments, not just Eclipse.
java-analyzer-bundle.test/projects/customers-tomcat-legacy/Dockerfile (1)
1-26: LGTM! Well-structured multi-stage build.The Dockerfile follows best practices with a multi-stage build pattern that separates the build and runtime environments, reducing the final image size. The dependency caching layer (line 11) optimizes build times, and the
--chown=1001:0flag (line 23) ensures OpenShift compatibility.Consider removing the commented-out lines (4 and 20) if the JDK 8 versions are no longer needed for reference:
-# FROM maven:3.6-jdk-8-slim as build FROM maven:3.8-openjdk-11 as build-# FROM tomcat:9-jdk8-openjdk-slim FROM tomcat:9-jdk11-openjdk-slimjava-analyzer-bundle.test/projects/README.md (1)
17-25: Add language specifiers to fenced code blocks.The directory structure code blocks are missing language specifiers, which markdown linters flag. While this doesn't affect functionality, it improves markdown compliance and can enhance rendering in some viewers.
Apply this pattern to both directory structure blocks:
-``` +```text io.konveyor.demo/ ├── annotations/ - Custom annotations ...Also applies to: 62-80 </blockquote></details> <details> <summary>java-analyzer-bundle.test/integration/run_local.sh (2)</summary><blockquote> `4-4`: **Harden the shell options** Use stricter bash flags to fail fast and catch unset vars/pipe errors. ```diff -set -e +set -Eeuo pipefail
37-45: Avoid installing Go on every test run; pre-bake it in the imageInstalling Go with microdnf for each run adds minutes and network flakiness. Bake Go into jdtls-analyzer:test (or a builder stage) and drop the runtime install.
jdtls-analyzer:test \ - -c "microdnf install -y golang && go mod download && go test -v" + -c "go mod download && go test -v"If pre-baking isn’t feasible, at least mount a Go module cache to speed downloads:
$CONTAINER_RUNTIME run --rm \ - -v "$REPO_ROOT/java-analyzer-bundle.test:/tests${VOLUME_FLAGS}" \ + -v "$REPO_ROOT/java-analyzer-bundle.test:/tests${VOLUME_FLAGS}" \ + -v "$HOME/.cache/gomod:/root/go/pkg/mod${VOLUME_FLAGS}" \ -e WORKSPACE_DIR=/tests/projects \ -e JDTLS_PATH=/jdtls \.github/workflows/integration-tests.yml (3)
43-47: Remove redundant setup-go; Go runs inside the containerThis step doesn’t affect the container. Drop it to save time or move tests outside the container if you intend to use the runner’s Go toolchain.
- - name: Set up Go - uses: actions/setup-go@v5 - with: - go-version: '1.23.9' + # Go is provided inside the container image used below
58-71: Pre-bake Go in the image or mount a module cache for stability and speedInstalling Go via microdnf at runtime is slow and flaky. Prefer baking Go into jdtls-analyzer:test and drop the install. If you must install at runtime, mount a cache to reuse modules.
Example (pre-baked Go; no install at runtime):
- -c "microdnf install -y golang && cd /tests/integration && go mod download && go test -v" + -c "cd /tests/integration && go mod download && go test -v"Optional module cache mount:
- podman run --rm \ + podman run --rm \ + -v "${HOME}/.cache/gomod:/root/go/pkg/mod:Z" \
23-25: Use non-interactive Maven flags for CIAvoids superfluous output and toolchain downloads.
- run: mvn clean integration-test + run: mvn -B -ntp clean integration-testjava-analyzer-bundle.test/projects/customers-tomcat-legacy/Makefile (4)
9-14: Declare 'all' as PHONYSilences checkmake warning and prevents unintended file target collisions.
-.PHONY: help +.PHONY: all help
20-21: Use -f flag for prune instead of echoing to stdinMore reliable and clearer.
- echo Y | docker image prune + docker image prune -f
28-31: Fix target description; it runs a container, not a vagrant VMAvoids confusion for users.
-run: ## Run a vagrant VM using this image +run: ## Run a container from the built image
35-37: Make remove idempotentDon’t fail if images don’t exist; force-remove or ignore errors.
- docker rmi $(REMOTE_IMAGE_NAME) - docker rmi $(LOCAL_IMAGE_NAME) + -docker rmi -f $(REMOTE_IMAGE_NAME) + -docker rmi -f $(LOCAL_IMAGE_NAME)java-analyzer-bundle.test/integration/README.md (4)
17-46: Add a language specifier to fenced block to satisfy markdownlint (MD040)Use text for ASCII diagrams.
-``` +```text ┌─────────────────────────────────────────────────────────┐ ...--- `384-384`: **Hyphenate “5-second”** Minor grammar fix. ```diff -1. **Project not loaded**: JDT.LS needs time to index projects (5 second wait in Initialize()) +1. **Project not loaded**: JDT.LS needs time to index projects (5-second wait in Initialize())
107-118: Align helper names and file path with the codeThe code defines unexported helpers in integration/test_helpers.go. Update docs to match.
-### 3. `test_helpers.go` -Helper functions for result verification. +### 3. `test_helpers.go` +Helper functions for result verification (package-local). -**Key Functions**: +**Key functions** (as implemented): ```go -func verifySymbolInResults(symbols []protocol.SymbolInformation, expectedName string, - expectedKind ...protocol.SymbolKind) bool +func verifySymbolInResults(symbols []protocol.SymbolInformation, expectedName string, expectedKind ...protocol.SymbolKind) bool -func verifySymbolLocationContains(symbols []protocol.SymbolInformation, - expectedName, expectedFile string) bool +func verifySymbolLocationContains(symbols []protocol.SymbolInformation, expectedName, expectedFile string) bool--- `277-306`: **Fix helper reference path and exported names in examples** Examples mention tests/test_utils.go and exported helpers; update to integration/test_helpers.go and unexported names to match the code. ```diff -Tests verify using helper functions from `tests/test_utils.go`: +Tests verify using helper functions from `integration/test_helpers.go`: @@ - if VerifySymbolInResults(symbols, "SampleApplication") { + if verifySymbolInResults(symbols, "SampleApplication") { @@ - if VerifySymbolInResults(symbols, "Customer", protocol.Class) { + if verifySymbolInResults(symbols, "Customer", protocol.Class) { @@ - if VerifySymbolLocationContains(symbols, "Customer", "Customer.java") { + if verifySymbolLocationContains(symbols, "Customer", "Customer.java") {java-analyzer-bundle.test/integration/integration_test.go (1)
26-33: Consider defaulting WORKSPACE_DIR to /tests/projects for parity with CIKeeps local defaults consistent with run_local.sh and the workflow.
- workspaceDir := os.Getenv("WORKSPACE_DIR") - if workspaceDir == "" { - workspaceDir = "/workspace" - } + workspaceDir := os.Getenv("WORKSPACE_DIR") + if workspaceDir == "" { + workspaceDir = "/tests/projects" + }java-analyzer-bundle.test/integration/client/jdtls_client.go (3)
53-58: Don’t kill the process in CmdDialer.Close; let Shutdown manage lifecycleKilling here can race against graceful shutdown and cause noisy exits. Close pipes and return.
func (rwc *CmdDialer) Close() error { - if rwc.Cmd.Process != nil { - rwc.Cmd.Process.Kill() - } - return rwc.Cmd.Wait() + if rwc.Stdin != nil { + _ = rwc.Stdin.Close() + } + if rwc.Stdout != nil { + _ = rwc.Stdout.Close() + } + // Let Shutdown() signal and wait/kill with timeout. + return nil }
187-193: Avoid hard-coding the bundle jar/version; make it configurable or discoverableHard-coded SNAPSHOT path is brittle. Support env/config or glob to locate the bundle.
- InitializationOptions: map[string]interface{}{ - "bundles": []string{ - "/jdtls/plugins/java-analyzer-bundle.core-1.0.0-SNAPSHOT.jar", - }, - "workspaceFolders": []string{string(workspaceURI)}, - }, + InitializationOptions: map[string]interface{}{ + "bundles": discoverBundles(os.Getenv("BUNDLE_JARS"), "/jdtls/plugins/java-analyzer-bundle.core-*.jar"), + "workspaceFolders": []string{string(workspaceURI)}, + },Add helper (same file):
func discoverBundles(env, pattern string) []string { if env != "" { return strings.Split(env, ",") } matches, _ := filepath.Glob(pattern) return matches }
208-211: Replace fixed sleep with a bounded readiness waitA fixed 5s can be flaky on CI. Poll a lightweight request until the server responds or a timeout elapses.
- // Wait for server to process projects - time.Sleep(5 * time.Second) + // Wait (bounded) for server to process projects + deadline := time.Now().Add(30 * time.Second) + for time.Now().Before(deadline) { + var ping interface{} + if err := c.conn.Call(c.ctx, "workspace/workspaceFolders", nil).Await(c.ctx, &ping); err == nil { + break + } + time.Sleep(500 * time.Millisecond) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
java-analyzer-bundle.test/integration/go.sumis excluded by!**/*.sumjava-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.resources/.projects/customers-tomcat/.markers.snapis excluded by!**/*.snapjava-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.resources/.projects/customers-tomcat/.syncinfo.snapis excluded by!**/*.snapjava-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.resources/.projects/test-project/.markers.snapis excluded by!**/*.snapjava-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.resources/.projects/test-project/.syncinfo.snapis excluded by!**/*.snapjava-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.resources/.root/.markers.snapis excluded by!**/*.snapjava-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.resources/0.snapis excluded by!**/*.snapjava-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.jdt.core/variablesAndContainers.datis excluded by!**/*.dat
📒 Files selected for processing (72)
.github/workflows/integration-tests.yml(1 hunks)Dockerfile(2 hunks)Makefile(1 hunks)java-analyzer-bundle.test/integration/README.md(1 hunks)java-analyzer-bundle.test/integration/client/jdtls_client.go(1 hunks)java-analyzer-bundle.test/integration/go.mod(1 hunks)java-analyzer-bundle.test/integration/integration_test.go(1 hunks)java-analyzer-bundle.test/integration/run_local.sh(1 hunks)java-analyzer-bundle.test/integration/test_helpers.go(1 hunks)java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.resources/.history/1a/b04dc5b688b00010150f900a41fc8a5f(1 hunks)java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.resources/.history/21/b0bec7b688b00010150f900a41fc8a5f(1 hunks)java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.resources/.history/3a/f0329fb788b00010150f900a41fc8a5f(1 hunks)java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.resources/.history/3f/a0e0a0b788b00010150f900a41fc8a5f(1 hunks)java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.resources/.history/47/c056cab688b00010150f900a41fc8a5f(1 hunks)java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.resources/.history/48/c04c9cb788b00010150f900a41fc8a5f(1 hunks)java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.resources/.history/69/10c7c3b688b00010150f900a41fc8a5f(1 hunks)java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.resources/.history/6b/e093b2b788b00010150f900a41fc8a5f(1 hunks)java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.resources/.history/6e/c0bd9eb788b00010150f900a41fc8a5f(1 hunks)java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.resources/.history/b0/6044a0b788b00010150f900a41fc8a5f(1 hunks)java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.resources/.history/b1/a0f1b8b688b00010150f900a41fc8a5f(1 hunks)java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.resources/.history/be/3086c6b688b00010150f900a41fc8a5f(1 hunks)java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.resources/.history/c5/d0d7bbb688b00010150f900a41fc8a5f(1 hunks)java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.resources/.history/ca/20a89fb788b00010150f900a41fc8a5f(1 hunks)java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.resources/.history/cd/d055a1b788b00010150f900a41fc8a5f(1 hunks)java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.resources/.history/d/401ec9b688b00010150f900a41fc8a5f(1 hunks)java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.resources/.history/d1/507ad5b688b00010150f900a41fc8a5f(1 hunks)java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.runtime/.settings/org.eclipse.core.resources.prefs(1 hunks)java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.runtime/.settings/org.eclipse.jdt.core.prefs(1 hunks)java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.runtime/.settings/org.eclipse.jdt.launching.prefs(1 hunks)java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.runtime/.settings/org.eclipse.m2e.core.prefs(1 hunks)java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.jdt.core/javaLikeNames.txt(1 hunks)java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.jdt.core/savedIndexNames.txt(1 hunks)java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.jdt.launching/.install.xml(1 hunks)java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.jdt.launching/libraryInfos.xml(1 hunks)java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.m2e.core/workspacestate.properties(1 hunks)java-analyzer-bundle.test/projects/README.md(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/.gitignore(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/Dockerfile(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/Makefile(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/pom.xml(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/OrderManagementAppInitializer.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/config/PersistenceConfig.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/config/WebConfig.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/controller/CustomerController.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/exception/ResourceNotFoundException.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/exception/handler/ExceptionHandlingController.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/model/Customer.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/repository/CustomerRepository.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/service/CustomerService.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/service/ICustomerService.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/resources/import.sql(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/resources/persistence.properties(1 hunks)java-analyzer-bundle.test/projects/test-project/.classpath(1 hunks)java-analyzer-bundle.test/projects/test-project/.project(1 hunks)java-analyzer-bundle.test/projects/test-project/.settings/org.eclipse.core.resources.prefs(1 hunks)java-analyzer-bundle.test/projects/test-project/.settings/org.eclipse.jdt.apt.core.prefs(1 hunks)java-analyzer-bundle.test/projects/test-project/.settings/org.eclipse.jdt.core.prefs(1 hunks)java-analyzer-bundle.test/projects/test-project/.settings/org.eclipse.m2e.core.prefs(1 hunks)java-analyzer-bundle.test/projects/test-project/pom.xml(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/Calculator.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/EnumExample.java(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/projects/test-project/src/main/java/io/konveyor/demo/annotations/CustomAnnotation.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/annotations/DeprecatedApi.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/inheritance/BaseService.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/inheritance/CustomException.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/inheritance/DataService.java(1 hunks)java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/RuleEntryParamsTest.java(1 hunks)java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandlerTest.java(2 hunks)java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/query/AnnotationQueryTest.java(1 hunks)pom.xml(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/controller/CustomerController.java (1)
java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/exception/ResourceNotFoundException.java (1)
ResourceNotFoundException(19-27)
java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/exception/handler/ExceptionHandlingController.java (1)
java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/exception/ResourceNotFoundException.java (1)
ResourceNotFoundException(19-27)
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/query/AnnotationQueryTest.java (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/query/AnnotationQuery.java (1)
AnnotationQuery(11-75)
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/SampleApplication.java (1)
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/inheritance/BaseService.java (1)
BaseService(9-39)
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandlerTest.java (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java (1)
SampleDelegateCommandHandler(43-419)
java-analyzer-bundle.test/integration/integration_test.go (1)
java-analyzer-bundle.test/integration/client/jdtls_client.go (2)
JDTLSClient(19-28)NewJDTLSClient(82-91)
🪛 actionlint (1.7.8)
.github/workflows/integration-tests.yml
63-63: shellcheck reported issue in this script: SC2046:warning:2:6: Quote this to prevent word splitting
(shellcheck)
🪛 checkmake (0.2.2)
java-analyzer-bundle.test/projects/customers-tomcat-legacy/Makefile
[warning] 38-38: Missing required phony target "all"
(minphony)
[warning] 38-38: Missing required phony target "test"
(minphony)
[warning] 9-9: Target "all" should be declared PHONY.
(phonydeclared)
Makefile
[warning] 25-25: Target body for "help" exceeds allowed length of 5 (24).
(maxbodylength)
[warning] 115-115: Target body for "clean" exceeds allowed length of 5 (6).
(maxbodylength)
🪛 LanguageTool
java-analyzer-bundle.test/integration/README.md
[grammar] ~384-~384: Use a hyphen to join words.
Context: ...: JDT.LS needs time to index projects (5 second wait in Initialize()) 2. **Build ...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
java-analyzer-bundle.test/integration/README.md
17-17: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
62-62: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
java-analyzer-bundle.test/projects/README.md
17-17: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
62-62: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 OSV Scanner (2.2.3)
java-analyzer-bundle.test/projects/customers-tomcat-legacy/pom.xml
[CRITICAL] 1-1: ch.qos.logback:logback-classic 1.1.7: QOS.ch Logback vulnerable to Deserialization of Untrusted Data
[CRITICAL] 1-1: ch.qos.logback:logback-classic 1.1.7: logback serialization vulnerability
[CRITICAL] 1-1: org.postgresql:postgresql 42.2.23: org.postgresql:postgresql vulnerable to SQL Injection via line comment generation
[CRITICAL] 1-1: org.postgresql:postgresql 42.2.23: TemporaryFolder on unix-like systems does not limit access to created files
[CRITICAL] 1-1: org.postgresql:postgresql 42.2.23: pgjdbc Arbitrary File Write Vulnerability
[CRITICAL] 1-1: org.postgresql:postgresql 42.2.23: Path traversal in org.postgresql:postgresql
[CRITICAL] 1-1: org.postgresql:postgresql 42.2.23: PostgreSQL JDBC Driver SQL Injection in ResultSet.refreshRow() with malicious column names
[CRITICAL] 1-1: org.postgresql:postgresql 42.2.23: pgjdbc Does Not Check Class Instantiation when providing Plugin Classes
[CRITICAL] 1-1: org.springframework:spring-web 5.3.7: Spring Framework DoS via conditional HTTP request
[CRITICAL] 1-1: org.springframework:spring-web 5.3.7: Spring Framework URL Parsing with Host Validation
[CRITICAL] 1-1: org.springframework:spring-web 5.3.7: Spring Framework DataBinder Case Sensitive Match Exception
[CRITICAL] 1-1: org.springframework:spring-web 5.3.7: Pivotal Spring Framework contains unsafe Java deserialization methods
[CRITICAL] 1-1: org.springframework:spring-web 5.3.7: Spring Web vulnerable to Open Redirect or Server Side Request Forgery
[CRITICAL] 1-1: org.springframework:spring-web 5.3.7: Spring Framework URL Parsing with Host Validation Vulnerability
[CRITICAL] 1-1: org.springframework:spring-webmvc 5.3.7: Remote Code Execution in Spring Framework
[CRITICAL] 1-1: org.springframework:spring-webmvc 5.3.7: Spring Framework is vulnerable to security bypass via mvcRequestMatcher pattern mismatch
[CRITICAL] 1-1: org.springframework:spring-webmvc 5.3.7: Path traversal vulnerability in functional web frameworks
[CRITICAL] 1-1: org.springframework:spring-webmvc 5.3.7: Spring Framework Path Traversal vulnerability
[CRITICAL] 1-1: org.springframework:spring-webmvc 5.3.7: Spring Framework MVC Applications Path Traversal Vulnerability
[CRITICAL] 1-1: org.springframework:spring-webmvc 5.3.7: Spring MVC controller vulnerable to a DoS attack
⏰ 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 (44)
java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.resources/.history/b0/6044a0b788b00010150f900a41fc8a5f (1)
1-5: Verify intent of checking in Eclipse workspace metadata and.historydirectory.This file is located in Eclipse's
.historydirectory, which contains auto-generated workspace snapshots typically excluded from version control. Additionally, the path contains a nested.metadata/.metadata/structure, which is unusual.If the intent is to ensure reproducible test fixtures with specific Eclipse settings (Java 17 target, forbidden reference warnings, etc.), this approach works but requires explicit documentation. Otherwise,
.metadata/and.history/directories are generally better left out of VCS.Questions:
- Is the nested
.metadata/.metadata/intentional, or does the path need adjustment?- Is there a documented reason for including Eclipse
.historysnapshots in the test project? If so, consider adding a note in the integration test README.java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.jdt.core/javaLikeNames.txt (1)
1-1: Metadata file for test infrastructure — LGTM.This metadata file is appropriate for the Eclipse JDT test scaffolding. However, note the unusual double
.metadata/in the path; verify this is intentional and not an artifact of project export/setup.java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.jdt.core/savedIndexNames.txt (1)
1-19: Metadata file for test infrastructure — LGTM.This Eclipse JDT saved index file is a valid part of the test project metadata. The INDEX VERSION header and 18 index entries follow the expected format. However, verify the double
.metadata/path structure is intentional and not an artifact of tooling setup (same concern as the siblingjavaLikeNames.txtfile).java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.resources/.history/1a/b04dc5b688b00010150f900a41fc8a5f (1)
1-3: Verify that Eclipse metadata should be committed to the repository.This file is Eclipse IDE workspace metadata/preferences located in a
.metadata/.historydirectory. While it makes sense to commit test project configurations for reproducibility, Eclipse's.metadatadirectories are typically auto-generated and transient.Consider: Should this be part of the repository, or should the test infrastructure regenerate these preferences during setup? If metadata is to be committed, document the rationale in the test README.
java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.jdt.launching/.install.xml (1)
1-6: Verify environment portability and version control approach for Eclipse metadata file.This file hardcodes system-specific JDK paths and is checked into version control, which raises portability concerns:
- The paths are architecture-specific (aarch64 only) and won't work on x86_64 systems.
- The paths depend on exact JDK installations at specific locations—if the test environment doesn't have these exact paths, the file becomes stale.
- This is Eclipse workspace metadata (
.metadata/.plugins/) which is typically excluded from version control and auto-generated per environment.Please clarify the testing approach:
- Does the Dockerfile/container definition ensure these exact JDK paths exist?
- Should this file be auto-generated during test environment setup, or is it intentionally pinned?
- How will this work across different architectures (x86_64, aarch64) or for developers with different JDK installations?
If this is meant to be container-specific, consider documenting this dependency in a README or Dockerfile comment. If it should work across environments, consider generating it dynamically rather than checking it into version control.
java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.resources/.history/b1/a0f1b8b688b00010150f900a41fc8a5f (1)
1-2: ****The review comment targets a file that does not exist in the repository. No
.metadata/directories or files are present in the codebase, meaning there is nothing to remove from version control. The review comment appears to be based on incorrect context and should be dismissed.Likely an incorrect or invalid review comment.
java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/service/ICustomerService.java (1)
3-5: No action required.The review comment incorrectly flagged missing Spring Data dependencies. The pom.xml properly declares
spring-data-jpaand importsspring-data-bomin the dependency management section. The imports ofPageandPageablewill resolve correctly.Likely an incorrect or invalid review comment.
java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.resources/.history/be/3086c6b688b00010150f900a41fc8a5f (1)
1-4: This review comment is incorrect and should be disregarded.The file referenced in the review comment does not exist in the repository. The path
java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.resources/.history/be/3086c6b688b00010150f900a41fc8a5fwas not found despite exhaustive searches.Verification confirmed:
- No
.metadata/directories are present in the repository- No
.history/directories exist- The
java-analyzer-bundle.test/directory contains only normal Eclipse project files (.classpath,.project,.settings/with standard configuration)- No instances of the referenced file hash exist
The repository is in the correct state with IDE metadata properly excluded. No action is required.
Likely an incorrect or invalid review comment.
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/EnumExample.java (1)
1-27: LGTM! Clean enum structure.The enum constants, fields, constructor, and accessors are well-structured and follow standard Java conventions.
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/annotations/DeprecatedApi.java (1)
1-16: LGTM! Well-defined annotation.The annotation structure is correct with appropriate retention policy and targets for marking deprecated APIs in tests.
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/Calculator.java (1)
1-43: LGTM! Test fixture methods are well-implemented.All methods in the Calculator class are correctly implemented for integration testing purposes:
- The
dividemethod properly guards against division by zero- The deprecated
summethod appropriately uses the@DeprecatedApiannotation- The
getStatusmethod correctly references theEnumExampleenumjava-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.runtime/.settings/org.eclipse.jdt.launching.prefs (1)
1-2: Review comment references a file that does not exist in the repository.The file
java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.runtime/.settings/org.eclipse.jdt.launching.prefsis not present in the codebase. No.metadata/.metadata/directories ororg.eclipse.jdt.launching.prefsfiles exist anywhere in this repository.All concerns raised in the original review comment are therefore invalid and should be disregarded.
Likely an incorrect or invalid review comment.
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/ServletExample.java (2)
8-14: LGTM! Clear test purpose documented.The Javadoc clearly documents that this servlet is for testing package imports and Jakarta EE API references, which aligns with the PR's Phase 2 integration testing objectives.
16-21: LGTM! Simple and appropriate for testing.The doGet implementation is straightforward and suitable for integration testing purposes.
java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.resources/.history/cd/d055a1b788b00010150f900a41fc8a5f (1)
1-7: The review comment is incorrect. The PR is removing Eclipse metadata files, not adding them.The git status shows these files are marked with "D" (deleted), not "A" (added). The file referenced in the review comment (
java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.resources/.history/cd/d055a1b788b00010150f900a41fc8a5f) is being deleted from the repository.The PR fulfills the exact recommendation made in the review comment—removing the entire
.metadatadirectory and its contents. No action is needed.Likely an incorrect or invalid review comment.
java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/OrderManagementAppInitializer.java (1)
13-29: LGTM!The Spring WebApplicationInitializer setup is correctly implemented with proper context configuration, package scanning, and servlet registration.
java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/resources/import.sql (1)
1-5: LGTM!The test data inserts are well-structured and appropriate for integration testing.
java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/config/WebConfig.java (1)
7-11: LGTM!The configuration correctly excludes DataSourceAutoConfiguration, which is appropriate given the manual datasource configuration in persistence.properties.
java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/repository/CustomerRepository.java (1)
6-8: LGTM!Standard Spring Data repository interface correctly defined with appropriate generic types.
java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/exception/ResourceNotFoundException.java (1)
19-26: LGTM!Well-implemented custom runtime exception with proper serialVersionUID and clear constructor.
java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/exception/handler/ExceptionHandlingController.java (1)
11-22: LGTM!The global exception handler is properly configured to map ResourceNotFoundException to HTTP 404 responses with appropriate logging.
java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/controller/CustomerController.java (1)
35-38: LGTM!The findAll method correctly uses Spring Data's Pageable support to return paginated customer results.
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandlerTest.java (3)
29-32: LGTM! Improved test name.The renamed test method is clearer and more descriptive than the previous name.
171-174: LGTM! Proper negative test.This test correctly verifies that unsupported commands throw
UnsupportedOperationException, providing good coverage for error handling.
93-105: No action required—original review comment is incorrect.The test does not require Java EE dependencies. The
"javax.ejb.Stateless"value is a string query parameter passed to the command handler's analyzer, not an actual class dependency. All similar tests in this file use the same pattern: string queries like"java.io.*","java.util.ArrayList","toString", and"TestClass"are passed as symbolic patterns to search for within code. The test verifies the handler returns results; it does not require these classes or annotations to be present on the classpath.Likely an incorrect or invalid review comment.
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/RuleEntryParamsTest.java (1)
1-17: This is an Eclipse PDE test plugin; src/main/java test placement is intentional and correct.The module uses
<packaging>eclipse-test-plugin</packaging>, making it an Eclipse Plugin Development Environment test bundle. In PDE projects, test code in src/main/java is the correct convention—tests are discovered and run by Eclipse's test runner, not Maven Surefire. No changes needed.Likely an incorrect or invalid review comment.
java-analyzer-bundle.test/projects/test-project/.settings/org.eclipse.jdt.apt.core.prefs (1)
1-2: Configuration looks reasonable.Disabling annotation processing is appropriate for projects that don't use annotation processors. This setting is clear and explicit.
java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.resources/.history/48/c04c9cb788b00010150f900a41fc8a5f (1)
1-2: Eclipse metadata files are correctly being removed from version control.Verification confirms these files are staged for deletion in the PR. The git status shows all nested
.metadata/.metadata/files—including history entries, indexes, preferences, and configuration—are marked as deleted. These are auto-generated, workspace-specific, and machine-dependent artifacts that should never be tracked.Additionally, consider adding
.metadata/to.gitignoreto prevent accidental re-commits, as the current.gitignorecovers other Eclipse files (.project,.classpath,.settings/) but not the.metadata/directory itself.java-analyzer-bundle.test/projects/customers-tomcat-legacy/.gitignore (1)
1-35: LGTM! Comprehensive .gitignore for test project.The .gitignore file appropriately covers common IDE configurations, OS-specific files, editor artifacts, and Maven build outputs. The patterns are well-organized and suitable for a Java Maven test project.
java-analyzer-bundle.test/projects/test-project/pom.xml (1)
1-30: LGTM! Clean test project configuration.The POM file is well-structured with:
- Clear project metadata and description
- Modern Java 17 target aligned with current standards
- Appropriate jakarta.servlet-api dependency with correct provided scope for servlet container environments
- Minimal dependencies suitable for a test fixture
java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.resources/.history/6e/c0bd9eb788b00010150f900a41fc8a5f (1)
1-2: The file referenced in this review does not exist in the repository.The path
java-analyzer-bundle.test/projects/.metadata/.metadata/.plugins/org.eclipse.core.resources/.history/6e/c0bd9eb788b00010150f900a41fc8a5fis not present anywhere in the codebase. There are no.historyfiles, no nested.metadata/.metadata/directories, and no.pluginsdirectories in the test projects.The repository only contains standard Eclipse configuration files under
.settings/(e.g.,org.eclipse.jdt.core.prefs,org.eclipse.core.resources.prefs), which are appropriate and necessary for integration tests that load Java projects into a JDT.LS workspace, as documented injava-analyzer-bundle.test/projects/README.mdandjava-analyzer-bundle.test/integration/README.md.Likely an incorrect or invalid review comment.
java-analyzer-bundle.test/projects/README.md (1)
1-288: Excellent documentation for the test projects!This README provides comprehensive coverage of the test infrastructure, including:
- Clear project overviews with technology stacks
- Detailed location type coverage (0-14)
- Practical test scenario examples
- Integration test strategy with phased approach
- Guidance for adding new test projects
The coverage matrix and migration pattern documentation will be particularly valuable for maintainers and contributors.
pom.xml (2)
26-26: Correct alignment of coverage filter with project packages.The coverage filter change from
org.eclipse.jdt.ls.*toio.konveyor.tackle.*properly aligns with the actual project package structure, ensuring code coverage reports accurately reflect the codebase.
148-166: Well-configured Jacoco coverage reporting.The addition of
prepare-agentandreportexecutions follows Jacoco best practices:
prepare-agentinstruments code before tests runappend=trueenables merging coverage from multiple test executionsreportgoal in the verify phase generates coverage reportsThese changes support the CI/CD integration testing infrastructure introduced in this PR.
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/annotations/CustomAnnotation.java (1)
1-17: Well-designed test annotation for integration testing.The annotation is properly configured with:
RUNTIMEretention for reflection-based testing- Broad target support (TYPE, METHOD, FIELD) to cover multiple test scenarios
- Sensible default values for flexible usage
This aligns well with the integration test strategy for location type 4 (annotation searches).
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/inheritance/BaseService.java (1)
1-39: Well-structured base class for integration testing.The design effectively supports testing of multiple location types:
- Location type 1 (inheritance): Subclasses extend BaseService
- Location type 5 (implements type): Implements Serializable and Comparable
- Proper serialVersionUID for Serializable implementation
- Abstract
initialize()method enforces subclass contract- Protected
serviceNamefield enables subclass customizationThe implementation is clean and serves its testing purpose well.
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/query/AnnotationQueryTest.java (1)
1-232: Excellent comprehensive unit test coverage!This test class provides thorough coverage of AnnotationQuery functionality with 18 test methods covering:
- Basic constructor and getter behavior
- Pattern matching (exact, regex, wildcard)
- Special annotation location handling (location type 4)
- Factory method (
fromMap) with various inputs- Edge cases (null inputs, empty patterns, empty elements)
- Location-specific behavior across all location types (0-14)
The tests are well-structured, properly named, and test both positive and negative scenarios. This aligns perfectly with the Phase 1 unit testing objectives mentioned in the PR.
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/SampleApplication.java (1)
94-96: LGTM! Proper override of abstract method.The
initialize()method correctly implements the abstract method fromBaseService, providing the required implementation for this concrete class.java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/inheritance/DataService.java (1)
1-33: Well-designed service class for integration testing.The class effectively demonstrates:
- Location type 1 (inheritance): Extends BaseService
- Location type 5 (implements type): Implements AutoCloseable
- Location type 4 (annotation): Annotated with @CustomAnnotation
- Proper lifecycle management with
initialize()andclose()methods- Clean state management with the
initializedflagThe implementation follows the AutoCloseable pattern correctly and provides good test coverage for multiple location types.
java-analyzer-bundle.test/integration/test_helpers.go (1)
9-33: LGTM!The helper functions are clear and correctly implement symbol verification logic for the integration tests. The early-return pattern and optional parameter handling in
verifySymbolInResultsis well-designed.Dockerfile (2)
24-25: Good refactoring of build stage.Using
ENVinstead of inline shell export is the correct approach for Dockerfiles, and the simplified Maven command is cleaner.
45-45: Plugin path update aligns with new architecture.The move from
/jdtls/java-analyzer-bundle/core/.../to/jdtls/plugins/correctly reflects the updated plugin structure referenced throughout this PR.java-analyzer-bundle.test/projects/test-project/.classpath (1)
1-57: Classpath looks correct for Maven + Java 17JRE container and Maven classpath entries align with the test project setup.
java-analyzer-bundle.test/integration/integration_test.go (1)
9-10: Import path is correct and properly alignedThe import
"github.com/konveyor/java-analyzer-bundle/integration/client"correctly matches the module path structure in go.mod (github.com/konveyor/java-analyzer-bundle/integration). This will properly resolve to the local client package. No action required.
...a/.metadata/.plugins/org.eclipse.core.resources/.history/47/c056cab688b00010150f900a41fc8a5f
Outdated
Show resolved
Hide resolved
...a/.metadata/.plugins/org.eclipse.core.resources/.history/c5/d0d7bbb688b00010150f900a41fc8a5f
Outdated
Show resolved
Hide resolved
...yzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/SampleApplication.java
Show resolved
Hide resolved
...yzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/SampleApplication.java
Show resolved
Hide resolved
...nalyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/ServletExample.java
Show resolved
Hide resolved
...le.test/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandlerTest.java
Show resolved
Hide resolved
...le.test/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandlerTest.java
Show resolved
Hide resolved
shawn-hurley
force-pushed
the
testing/adding-bundle-specific-tests
branch
from
3f70d1f to
50e7c43
Compare
There was a problem hiding this comment.
Actionable comments posted: 18
♻️ Duplicate comments (8)
java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/controller/CustomerController.java (1)
25-26: Fix the path mapping to include path variable template.The
@GetMappingvalue is missing the template variable syntax. Currently, it maps to the literal path/customers/idinstead of/customers/{id}, which prevents the@PathVariable("id")from binding correctly.java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandlerTest.java (2)
34-46: Align project name with test suite convention.This test uses
"project"while all other tests use"test-project". This inconsistency has been flagged in previous reviews.
236-249: Replace TestClass with an existing test class.The query for
"TestClass"references a class that doesn't exist in test-project. This issue has been flagged in previous reviews—available classes include SampleApplication, Calculator, EnumExample, etc..github/workflows/integration-tests.yml (1)
63-71: Quote $(pwd) to prevent word splitting.This issue has been previously identified. The unquoted
$(pwd)in the volume mount will cause issues with paths containing spaces.Apply this diff:
podman run --rm \ - -v $(pwd)/java-analyzer-bundle.test:/tests:Z \ + -v "$(pwd)/java-analyzer-bundle.test:/tests:Z" \ -e WORKSPACE_DIR=/tests/projects \ -e JDTLS_PATH=/jdtls \ --workdir /tests/integration \ --entrypoint /bin/sh \ jdtls-analyzer:test \ -c "microdnf install -y golang && cd /tests/integration && go mod download && go test -v"java-analyzer-bundle.test/projects/customers-tomcat-legacy/pom.xml (3)
19-19: Spring Framework version has known critical vulnerabilities.This comment duplicates prior feedback. Spring 5.3.7 has multiple critical vulnerabilities including DoS, path traversal, and RCE.
117-119: Logback version has known critical vulnerabilities.This comment duplicates prior feedback. Version 1.1.7 has multiple critical deserialization vulnerabilities.
127-129: PostgreSQL driver version has known critical vulnerabilities.This comment duplicates prior feedback. Version 42.2.23 has multiple critical vulnerabilities including SQL injection, arbitrary file write, and path traversal.
java-analyzer-bundle.test/integration/client/jdtls_client.go (1)
102-116: Point-dataat the workspace root.JDT.LS expects
-datato be the workspace directory and will create.metadataunderneath. Passing<workspace>/.metadatacauses the server to create<workspace>/.metadata/.metadataand can break workspace state. Ensure the workspace root exists and pass it directly.@@ - // Prepare workspace - dataDir := filepath.Join(c.workspaceDir, ".metadata") - if err := os.MkdirAll(dataDir, 0755); err != nil { - return fmt.Errorf("failed to create data directory: %w", err) - } + // Ensure workspace directory exists; JDT.LS will create .metadata under it + if err := os.MkdirAll(c.workspaceDir, 0755); err != nil { + return fmt.Errorf("failed to create workspace directory: %w", err) + } @@ - c.cmd = exec.Command(jdtlsBin, - "-configuration", configDir, - "-data", dataDir, - ) + c.cmd = exec.Command(jdtlsBin, + "-configuration", configDir, + "-data", c.workspaceDir, + )
🧹 Nitpick comments (26)
java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/service/ICustomerService.java (1)
7-12: Consider Java interface conventions.Two optional style improvements:
- The
publicmodifier on interface methods (lines 8, 10) is redundant since interface methods are implicitly public.- The "I" prefix for interface names is more of a C# convention; idiomatic Java typically omits it (e.g.,
CustomerServicefor the interface,CustomerServiceImplfor the implementation).Apply this diff to remove redundant modifiers:
public interface ICustomerService { - public Customer findById(Long id); + Customer findById(Long id); - public Page<Customer>findAll(Pageable pageable); + Page<Customer> findAll(Pageable pageable); }java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/model/Customer.java (1)
23-97: LGTM! Standard JPA entity structure for test fixture.The field declarations, accessor methods, and
toStringimplementation follow standard JPA entity patterns. Column length constraints are appropriate for customer data.Optional: Consider adding
equals()andhashCode()methods based on the business key (e.g.,username) for better entity semantics, though this is not critical for test fixtures.java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/entity/Product.java (2)
16-18: Consider adding @GeneratedValue for ID auto-generation.The
@Idfield typically includes@GeneratedValueto enable automatic ID generation by the persistence provider. If IDs will be manually assigned in tests, the current implementation is acceptable.Apply this diff if auto-generation is desired:
@Id + @GeneratedValue @Column(name = "id") private Long id;
14-59: Well-structured test entity.The class is clean and properly annotated for its purpose as a test fixture for JPA annotation matching. The implementation is straightforward and appropriate for integration testing.
Optionally, consider adding
equals()andhashCode()methods based on the ID or business key (e.g.,sku) for more robust entity behavior in collections and comparisons:@Override public boolean equals(Object o) { if (this == o) return true; if (!(o instanceof Product)) return false; Product product = (Product) o; return Objects.equals(sku, product.sku); } @Override public int hashCode() { return Objects.hash(sku); }java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/ServletExample.java (1)
23-28: Set content type for consistency and proper HTTP response handling.The
doPostmethod should set the response content type, similar todoGet(line 19). Without it, the client may not correctly interpret the response format.Apply this diff to set the content type:
@Override protected void doPost(HttpServletRequest request, HttpServletResponse response) throws IOException { + response.setContentType("text/html"); String data = request.getParameter("data"); response.getWriter().println("Received: " + data); }Note: The XSS vulnerability previously flagged in the past review comments at lines 26-27 should also be addressed before merging.
java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/controller/CustomerController.java (2)
20-23: Consider constructor injection and making the logger final.Field injection works but constructor injection is generally preferred for better testability and immutability. Additionally, the static logger field should be declared final.
Apply this diff to use constructor injection and make the logger final:
- @Autowired - private CustomerService customerService; + private final CustomerService customerService; - private static Logger logger = Logger.getLogger( CustomerController.class.getName() ); + private static final Logger logger = Logger.getLogger( CustomerController.class.getName() ); + + @Autowired + public CustomerController(CustomerService customerService) { + this.customerService = customerService; + }
35-38: Prefer@GetMappingover@RequestMappingfor clarity.While
@RequestMappingdefaults to GET when no method is specified, using@GetMappingmakes the HTTP method explicit and improves code readability.Apply this diff to use the more specific annotation:
- @RequestMapping + @GetMapping public Page<Customer> findAll(Pageable pageable){ return customerService.findAll(pageable); }java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandlerTest.java (3)
6-6: Remove unused import.The
failimport is not used anywhere in the test class.Apply this diff:
import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; -import static org.junit.Assert.fail;
48-61: Consider consolidating duplicate tests.
testRuleEntryWithFullModeandtestRuleEntryWithWildcardQueryare nearly identical—both query"java.io.*"with location"10", differing only inanalysisMode. Consider using a parameterized test or extracting a helper method to reduce duplication.Example approach using a helper method:
private void testRuleEntryWithQuery(String analysisMode, String testName) throws Exception { List<Object> params = new ArrayList<>(); Map<String, String> param = Map.of( "project", "test-project", "query", "java.io.*", "location", "10", "analysisMode", analysisMode ); params.add(param); List<Object> result = (List<Object>) commandHandler.executeCommand( SampleDelegateCommandHandler.RULE_ENTRY_COMMAND_ID, params, null); assertNotNull(testName + " should return non-null result", result); } @Test public void testRuleEntryWithFullMode() throws Exception { testRuleEntryWithQuery("full", "testRuleEntryWithFullMode"); } @Test public void testRuleEntryWithWildcardQuery() throws Exception { testRuleEntryWithQuery("source-only", "testRuleEntryWithWildcardQuery"); }Also applies to: 176-189
48-234: Consider strengthening test assertions.Most integration tests only verify
assertNotNull(result)without checking result content. While this confirms execution completes, consider adding assertions to verify:
- Expected result size (e.g.,
assertTrue(!result.isEmpty())when matches are expected)- Presence of specific symbols/locations
- Result structure correctness
This would increase confidence that the search functionality returns correct results, not just non-null values.
Example for
testRuleEntryWithMethodCallLocation:List<Object> result = (List<Object>) commandHandler.executeCommand(SampleDelegateCommandHandler.RULE_ENTRY_COMMAND_ID, params, null); assertNotNull(result); +assertTrue("Should find println method calls", !result.isEmpty());java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/exception/handler/ExceptionHandlingController.java (2)
4-4: Consider using SLF4J for better Spring integration.JBoss Logging is uncommon in Spring projects. SLF4J is the de facto logging facade for Spring applications and provides better interoperability with various logging implementations.
If you'd like to standardize on SLF4J, apply this diff:
-import org.jboss.logging.Logger; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory;And update line 14:
- private static Logger logger = Logger.getLogger( ExceptionHandlingController.class.getName() ); + private static final Logger logger = LoggerFactory.getLogger(ExceptionHandlingController.class);
14-14: Addfinalmodifier to the static logger field.Static fields that are not intended to be reassigned should be marked as
finalto prevent accidental modification and clearly communicate immutability.Apply this diff:
- private static Logger logger = Logger.getLogger( ExceptionHandlingController.class.getName() ); + private static final Logger logger = Logger.getLogger(ExceptionHandlingController.class.getName());java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/inheritance/BaseService.java (1)
9-39: Consider overriding equals() and hashCode() for consistency with Comparable.When a class implements
Comparable, it's recommended to also overrideequals()andhashCode()to ensure consistency: ifcompareTo()returns 0,equals()should return true.Example implementation:
@Override public boolean equals(Object obj) { if (this == obj) return true; if (obj == null || getClass() != obj.getClass()) return false; BaseService that = (BaseService) obj; return Objects.equals(serviceName, that.serviceName); } @Override public int hashCode() { return Objects.hash(serviceName); }java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/inheritance/DataService.java (1)
25-28: Consider using a more specific exception type.The
close()method declaresthrows Exception, which is overly broad. For better clarity and type safety, consider declaring a more specific exception type (or no exception if none are actually thrown).If no exceptions are thrown, you can simplify to:
@Override -public void close() throws Exception { +public void close() { this.initialized = false; System.out.println("DataService closed"); }java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/RuleEntryParamsTest.java (2)
156-173: Consider parameterized tests for better failure diagnostics.The loop-based approach efficiently tests all 15 location types, but when a failure occurs, it's harder to identify which specific location value failed. Consider using JUnit's
@Parameterizedrunner or separate test methods for each location to improve failure diagnostics and make the test results more granular.Example using JUnit 4 Parameterized:
@RunWith(Parameterized.class) public class RuleEntryParamsLocationTest { @Parameterized.Parameters(name = "location={0}") public static Collection<Object[]> data() { return Arrays.asList(new Object[][] { {0}, {1}, {2}, {3}, {4}, {5}, {6}, {7}, {8}, {9}, {10}, {11}, {12}, {13}, {14} }); } @Parameterized.Parameter public int location; @Test public void testLocation() { List<Object> arguments = new ArrayList<>(); Map<String, Object> params = new HashMap<>(); params.put("project", "test-project"); params.put("query", "test.query"); params.put("location", String.valueOf(location)); params.put("analysisMode", "full"); arguments.add(params); RuleEntryParams ruleParams = new RuleEntryParams("test-command", arguments); assertEquals(location, ruleParams.getLocation()); } }
19-217: Strong positive and negative test coverage.The test methods effectively cover normal operation scenarios, optional parameters, boolean flags, collections, and nested objects. The separation of concerns (one aspect per test) makes the suite maintainable and easy to understand.
For even more robust validation, consider adding edge case tests:
- Empty strings for required parameters (project, query, analysisMode)
- Negative or out-of-range location values (e.g., -1, 15, 100)
- Empty collections for includedPaths
- Malformed annotationQuery structures
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/persistence/JdbcOnlyService.java (3)
8-16: Docs/code mismatch; clarify intent (variable binding vs. presence).Comment says “variable binding,” but no
setXXX(...)calls are present. Confirm intent:
- If goal is only “PreparedStatement without EntityManager,” update comment.
- If goal is “variable binding,” add a bound parameter (see next comment’s diff).
19-30: Use try‑with‑resources to avoid leaks; optionally demonstrate binding.Manual closes can leak on exceptions. Prefer try-with-resources. If you want to actually show binding, use a placeholder and
setString(...).Apply:
- public void executeQuery(Connection conn) throws SQLException { - // Uses only JDBC PreparedStatement - PreparedStatement stmt = conn.prepareStatement("SELECT * FROM products"); - ResultSet rs = stmt.executeQuery(); - - while (rs.next()) { - System.out.println("Product: " + rs.getString("name")); - } - - rs.close(); - stmt.close(); - } + public void executeQuery(Connection conn) throws SQLException { + // Uses only JDBC PreparedStatement (no EntityManager) + try (PreparedStatement stmt = + conn.prepareStatement("SELECT name FROM products WHERE name LIKE ?")) { + // Optional binding to exercise parameterized queries in tests + stmt.setString(1, "%"); + try (ResultSet rs = stmt.executeQuery()) { + while (rs.next()) { + System.out.println("Product: " + rs.getString("name")); + } + } + } + }
24-26: AvoidSystem.outin test assets that may run under CI.Consider returning a list of names or using a logger to keep CI output clean.
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/annotations/DeprecatedApi.java (2)
14-15: Document elements and (optionally) add forRemoval for parity with JDK @deprecated.
- Brief Javadoc on since/replacement clarifies intent in tests.
- Optional: add boolean forRemoval defaulting to false; backward compatible and useful for assertions.
public @interface DeprecatedApi { - String since() default ""; - String replacement() default ""; + /** + * Version the API became deprecated (e.g., "1.0"). + */ + String since() default ""; + /** + * Suggested replacement API signature or identifier. + */ + String replacement() default ""; + /** + * Indicates the API is intended to be removed in a future release. + */ + boolean forRemoval() default false; }
3-13: Add @documented and expand @target for better test coverage.The test project is configured for Java 17, which fully supports all proposed annotation targets (TYPE_PARAMETER, TYPE_USE added in Java 8). Mark the annotation @documented so it appears in generated Javadoc, and expand @target to exercise more code locations in integration tests.
Apply:
import java.lang.annotation.ElementType; +import java.lang.annotation.Documented; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; import java.lang.annotation.Target; /** * Annotation for marking deprecated APIs. */ +@Documented @Retention(RetentionPolicy.RUNTIME) -@Target({ElementType.TYPE, ElementType.METHOD}) +@Target({ + ElementType.TYPE, + ElementType.METHOD, + ElementType.FIELD, + ElementType.PARAMETER, + ElementType.CONSTRUCTOR, + ElementType.TYPE_PARAMETER, + ElementType.TYPE_USE +}) public @interface DeprecatedApi {java-analyzer-bundle.test/README.md (1)
1-296: Excellent comprehensive documentation!This README provides thorough coverage of the test infrastructure, including:
- Clear quick start guide
- Well-organized structure with Phase 1 and Phase 2 sections
- Complete coverage matrix for all 15 location types
- Migration testing examples
- CI/CD pipeline documentation
Optional: Add language identifier to fenced code block
The static analysis tool flagged line 213 for missing a language specification on a fenced code block. Consider adding a language identifier (e.g.,
bash,java, ortext) to improve rendering and syntax highlighting.java-analyzer-bundle.test/docs/integration-tests.md (1)
1-800: Excellent integration test documentation!This comprehensive guide covers:
- Clear architecture diagrams
- Detailed component descriptions
- Multiple execution methods (local, container, CI)
- Test scenarios for all location types
- Migration testing patterns
- Troubleshooting guidance
Optional: Minor grammar refinement
Consider hyphenating "5-second wait" on line 384 for improved readability, though this is a very minor nitpick.
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/EnumConstantSymbolProvider.java (1)
19-52: Remove unused query field or implement filtering logic.The
queryfield is confirmed as unused inEnumConstantSymbolProvider.get(). However, peer implementations (MethodCallSymbolProvider, AnnotationSymbolProvider, ConstructorCallSymbolProvider, TypeSymbolProvider, ReturnTypeSymbolProvider) actively usequeryfor filtering. Either:
- Remove the unused field if enum constant filtering is not needed, or
- Implement query-based filtering logic consistent with other providers (e.g., check
if (this.query.contains("."))before adding symbols).java-analyzer-bundle.test/projects/customers-tomcat-legacy/Makefile (2)
9-9: Add .PHONY declaration for 'all' target.The
alltarget should be declared.PHONYfor consistency with other targets and to follow Makefile best practices.Apply this diff:
+.PHONY: all all: build
20-36: Consider using container runtime detection for consistency.The root Makefile (line 6-10) detects Podman/Docker dynamically, but this Makefile hardcodes
docker. For consistency and flexibility, consider using the same runtime detection pattern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
java-analyzer-bundle.test/integration/go.sumis excluded by!**/*.sum
📒 Files selected for processing (73)
.github/workflows/integration-tests.yml(1 hunks).gitignore(1 hunks)Dockerfile(2 hunks)Dockerfile.test(1 hunks)Makefile(1 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java(6 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SymbolInformationTypeRequestor.java(4 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/AnnotationSymbolProvider.java(4 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/ConstructorCallSymbolProvider.java(3 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java(9 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/EnumConstantSymbolProvider.java(1 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/MethodCallSymbolProvider.java(3 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/PackageDeclarationSymbolProvider.java(1 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/ReturnTypeSymbolProvider.java(2 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/SymbolProviderResolver.java(1 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/VariableDeclarationSymbolProvider.java(1 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/WithSearchPattern.java(1 hunks)java-analyzer-bundle.test/README.md(1 hunks)java-analyzer-bundle.test/docs/README.md(1 hunks)java-analyzer-bundle.test/docs/integration-tests.md(1 hunks)java-analyzer-bundle.test/docs/query-reference.md(1 hunks)java-analyzer-bundle.test/docs/quick-reference.md(1 hunks)java-analyzer-bundle.test/docs/test-projects.md(1 hunks)java-analyzer-bundle.test/integration/client/jdtls_client.go(1 hunks)java-analyzer-bundle.test/integration/go.mod(1 hunks)java-analyzer-bundle.test/integration/integration_test.go(1 hunks)java-analyzer-bundle.test/integration/run_local.sh(1 hunks)java-analyzer-bundle.test/integration/test_helpers.go(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/.gitignore(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/Dockerfile(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/Makefile(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/pom.xml(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/OrderManagementAppInitializer.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/config/PersistenceConfig.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/config/WebConfig.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/controller/CustomerController.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/exception/ResourceNotFoundException.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/exception/handler/ExceptionHandlingController.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/model/Customer.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/repository/CustomerRepository.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/service/CustomerService.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/service/ICustomerService.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/resources/import.sql(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/resources/persistence.properties(1 hunks)java-analyzer-bundle.test/projects/test-project/.classpath(1 hunks)java-analyzer-bundle.test/projects/test-project/.project(1 hunks)java-analyzer-bundle.test/projects/test-project/.settings/org.eclipse.core.resources.prefs(1 hunks)java-analyzer-bundle.test/projects/test-project/.settings/org.eclipse.jdt.apt.core.prefs(1 hunks)java-analyzer-bundle.test/projects/test-project/.settings/org.eclipse.jdt.core.prefs(1 hunks)java-analyzer-bundle.test/projects/test-project/.settings/org.eclipse.m2e.core.prefs(1 hunks)java-analyzer-bundle.test/projects/test-project/pom.xml(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/Calculator.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/EnumExample.java(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/projects/test-project/src/main/java/io/konveyor/demo/annotations/CustomAnnotation.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/annotations/DeprecatedApi.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/config/DataSourceConfig.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/config/MySQLDataSourceConfig.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/entity/Product.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/inheritance/BaseService.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/inheritance/CustomException.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/inheritance/DataService.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/jms/MessageProcessor.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/jms/TopicMessageProcessor.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/persistence/AnotherMixedService.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/persistence/JdbcOnlyService.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/persistence/PureJpaService.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/persistence/ServiceWithEntityManager.java(1 hunks)java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/RuleEntryParamsTest.java(1 hunks)java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandlerTest.java(2 hunks)java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/query/AnnotationQueryTest.java(1 hunks)pom.xml(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- java-analyzer-bundle.test/docs/test-projects.md
🚧 Files skipped from review as they are similar to previous changes (17)
- java-analyzer-bundle.test/integration/go.mod
- java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/annotations/CustomAnnotation.java
- java-analyzer-bundle.test/integration/test_helpers.go
- java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/resources/persistence.properties
- java-analyzer-bundle.test/projects/test-project/.settings/org.eclipse.m2e.core.prefs
- java-analyzer-bundle.test/projects/customers-tomcat-legacy/.gitignore
- java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/inheritance/CustomException.java
- java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/service/CustomerService.java
- java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/OrderManagementAppInitializer.java
- java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/resources/import.sql
- java-analyzer-bundle.test/projects/customers-tomcat-legacy/Dockerfile
- java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/exception/ResourceNotFoundException.java
- java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/Calculator.java
- java-analyzer-bundle.test/projects/test-project/.settings/org.eclipse.jdt.core.prefs
- java-analyzer-bundle.test/projects/test-project/.settings/org.eclipse.core.resources.prefs
- java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/SampleApplication.java
- pom.xml
🧰 Additional context used
🧬 Code graph analysis (11)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/SymbolProviderResolver.java (2)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/EnumConstantSymbolProvider.java (1)
EnumConstantSymbolProvider(19-53)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/PackageDeclarationSymbolProvider.java (1)
PackageDeclarationSymbolProvider(16-84)
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/config/DataSourceConfig.java (1)
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/config/MySQLDataSourceConfig.java (1)
DataSourceDefinition(9-21)
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/jms/TopicMessageProcessor.java (1)
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/jms/MessageProcessor.java (1)
MessageDriven(12-23)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/MethodCallSymbolProvider.java (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java (1)
CustomASTVisitor(28-279)
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/config/MySQLDataSourceConfig.java (1)
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/config/DataSourceConfig.java (1)
DataSourceDefinition(9-21)
java-analyzer-bundle.test/integration/integration_test.go (1)
java-analyzer-bundle.test/integration/client/jdtls_client.go (4)
JDTLSClient(19-28)NewJDTLSClient(87-96)AnnotationQuery(252-255)AnnotationElement(246-249)
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/jms/MessageProcessor.java (1)
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/jms/TopicMessageProcessor.java (1)
MessageDriven(12-22)
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandlerTest.java (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java (1)
SampleDelegateCommandHandler(43-427)
java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/exception/handler/ExceptionHandlingController.java (1)
java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/exception/ResourceNotFoundException.java (1)
ResourceNotFoundException(19-27)
java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/controller/CustomerController.java (1)
java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/exception/ResourceNotFoundException.java (1)
ResourceNotFoundException(19-27)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/ConstructorCallSymbolProvider.java (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java (1)
CustomASTVisitor(28-279)
🪛 actionlint (1.7.8)
.github/workflows/integration-tests.yml
63-63: shellcheck reported issue in this script: SC2046:warning:2:6: Quote this to prevent word splitting
(shellcheck)
🪛 checkmake (0.2.2)
java-analyzer-bundle.test/projects/customers-tomcat-legacy/Makefile
[warning] 38-38: Missing required phony target "all"
(minphony)
[warning] 38-38: Missing required phony target "test"
(minphony)
[warning] 9-9: Target "all" should be declared PHONY.
(phonydeclared)
Makefile
[warning] 25-25: Target body for "help" exceeds allowed length of 5 (24).
(maxbodylength)
[warning] 115-115: Target body for "clean" exceeds allowed length of 5 (6).
(maxbodylength)
🪛 LanguageTool
java-analyzer-bundle.test/docs/integration-tests.md
[grammar] ~384-~384: Use a hyphen to join words.
Context: ...: JDT.LS needs time to index projects (5 second wait in Initialize()) 2. **Build ...
(QB_NEW_EN_HYPHEN)
java-analyzer-bundle.test/docs/query-reference.md
[grammar] ~608-~608: Ensure spelling is correct
Context: ...**: ~5 seconds Per-Query Time: ~100-500ms depending on result count **Result Coun...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
java-analyzer-bundle.test/docs/README.md
[style] ~73-~73: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...uick-reference.md#running-tests) ### I want to understand what's tested → [Quick Refer...
(REP_WANT_TO_VB)
[style] ~77-~77: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...erence.md#tested-search-queries) ### I want to add a new test → [Integration Tests Gui...
(REP_WANT_TO_VB)
[style] ~81-~81: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...ence.md#untested-query-patterns) ### I want to understand the architecture → [Integrat...
(REP_WANT_TO_VB)
[style] ~85-~85: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...integration-tests.md#components) ### I want to understand test projects → [Test Projec...
(REP_WANT_TO_VB)
[style] ~89-~89: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...est-projects.md#coverage-matrix) ### I want to add Java test code → [Test Projects - A...
(REP_WANT_TO_VB)
[style] ~95-~95: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...ration-tests.md#troubleshooting) ### I want to understand migration testing → [Query R...
(REP_WANT_TO_VB)
🪛 markdownlint-cli2 (0.18.1)
java-analyzer-bundle.test/README.md
213-213: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
java-analyzer-bundle.test/docs/quick-reference.md
17-17: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
62-62: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
java-analyzer-bundle.test/docs/integration-tests.md
17-17: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
311-311: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
java-analyzer-bundle.test/docs/README.md
213-213: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 OSV Scanner (2.2.3)
java-analyzer-bundle.test/projects/customers-tomcat-legacy/pom.xml
[CRITICAL] 1-1: ch.qos.logback:logback-classic 1.1.7: QOS.ch Logback vulnerable to Deserialization of Untrusted Data
[CRITICAL] 1-1: ch.qos.logback:logback-classic 1.1.7: logback serialization vulnerability
[CRITICAL] 1-1: org.postgresql:postgresql 42.2.23: org.postgresql:postgresql vulnerable to SQL Injection via line comment generation
[CRITICAL] 1-1: org.postgresql:postgresql 42.2.23: TemporaryFolder on unix-like systems does not limit access to created files
[CRITICAL] 1-1: org.postgresql:postgresql 42.2.23: pgjdbc Arbitrary File Write Vulnerability
[CRITICAL] 1-1: org.postgresql:postgresql 42.2.23: Path traversal in org.postgresql:postgresql
[CRITICAL] 1-1: org.postgresql:postgresql 42.2.23: PostgreSQL JDBC Driver SQL Injection in ResultSet.refreshRow() with malicious column names
[CRITICAL] 1-1: org.postgresql:postgresql 42.2.23: pgjdbc Does Not Check Class Instantiation when providing Plugin Classes
[CRITICAL] 1-1: org.springframework:spring-web 5.3.7: Spring Framework DoS via conditional HTTP request
[CRITICAL] 1-1: org.springframework:spring-web 5.3.7: Spring Framework URL Parsing with Host Validation
[CRITICAL] 1-1: org.springframework:spring-web 5.3.7: Spring Framework DataBinder Case Sensitive Match Exception
[CRITICAL] 1-1: org.springframework:spring-web 5.3.7: Pivotal Spring Framework contains unsafe Java deserialization methods
[CRITICAL] 1-1: org.springframework:spring-web 5.3.7: Spring Web vulnerable to Open Redirect or Server Side Request Forgery
[CRITICAL] 1-1: org.springframework:spring-web 5.3.7: Spring Framework URL Parsing with Host Validation Vulnerability
[CRITICAL] 1-1: org.springframework:spring-webmvc 5.3.7: Remote Code Execution in Spring Framework
[CRITICAL] 1-1: org.springframework:spring-webmvc 5.3.7: Spring Framework is vulnerable to security bypass via mvcRequestMatcher pattern mismatch
[CRITICAL] 1-1: org.springframework:spring-webmvc 5.3.7: Path traversal vulnerability in functional web frameworks
[CRITICAL] 1-1: org.springframework:spring-webmvc 5.3.7: Spring Framework Path Traversal vulnerability
[CRITICAL] 1-1: org.springframework:spring-webmvc 5.3.7: Spring Framework MVC Applications Path Traversal Vulnerability
[CRITICAL] 1-1: org.springframework:spring-webmvc 5.3.7: Spring MVC controller vulnerable to a DoS attack
⏰ 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). (1)
- GitHub Check: Build tackle2-addon-analyzer
🔇 Additional comments (52)
.gitignore (1)
7-7: Good addition to support test infrastructure.The pattern
*/projects/.metadataappropriately ignores IDE metadata directories within test project hierarchies, preventing unnecessary VCS noise as test projects are scaffolded during the integration testing phase.java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/jms/MessageProcessor.java (3)
1-11: LGTM!The imports and documentation are appropriate for a JMS message-driven bean test fixture. The Javadoc clearly explains the test's purpose.
12-17: LGTM!The
@MessageDrivenconfiguration is valid and properly structured. The activation config properties correctly specify a queue-based JMS destination, complementing the topic-basedTopicMessageProcessorin the test suite.
19-22: LGTM!The
onMessageimplementation is appropriately simple for a test fixture. The goal is to exercise the analyzer's detection capabilities, not to perform actual message processing.java-analyzer-bundle.test/projects/test-project/.project (1)
1-34: LGTM! Standard Eclipse project configuration.The project descriptor is correctly configured with Java and Maven builders, appropriate natures, and sensible resource filters.
java-analyzer-bundle.test/projects/test-project/pom.xml (1)
1-62: LGTM! Well-structured Maven POM for testing.The configuration is appropriate for a test project with Java 17, UTF-8 encoding, and provided-scope dependencies. The mix of Jakarta EE 9+ (
jakarta.servlet) and older Java EE APIs (javax.*) appears intentional for comprehensive testing coverage across API generations.java-analyzer-bundle.test/projects/test-project/.classpath (1)
1-57: LGTM! Standard Eclipse classpath for Maven projects.The classpath correctly maps the Maven project structure with JavaSE-17 (consistent with the POM), includes annotation processing support, and properly distinguishes test from production sources.
java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/repository/CustomerRepository.java (1)
1-8: LGTM! Standard Spring Data repository for test fixture.This is a correctly structured Spring Data repository interface. The empty interface extending
PagingAndSortingRepository<Customer, Long>is standard practice, as Spring Data will generate the implementation at runtime with full CRUD and pagination support.java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/model/Customer.java (1)
15-19: Verify the intentional use of initialValue = 6 for test data.The sequence generator specifies
initialValue = 6, which is unusual. This suggests the test database is pre-seeded with 5 customer records. Please confirm this aligns with your test data setup expectations.java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/persistence/AnotherMixedService.java (2)
1-7: LGTM!The package declaration and imports correctly set up the mixed JPA/JDBC scenario that this test class is designed to demonstrate.
9-17: LGTM!The class structure and documentation clearly explain the test scenario. The EntityManager field is properly annotated with
@PersistenceContextfor JPA dependency injection.java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/ServletExample.java (3)
1-11: LGTM! Clear documentation and appropriate imports.The package structure, imports, and JavaDoc clearly document this servlet's purpose for testing Jakarta EE API references and package imports.
12-15: LGTM! Proper servlet class structure.The class properly extends
HttpServletand includes the requiredserialVersionUIDfield.
16-21: LGTM! Properly implemented doGet handler.The method correctly sets the content type and returns a simple HTML response.
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/ReturnTypeSymbolProvider.java (2)
23-24: LGTM! Enhanced matching with readable type conversion.The conversion from JVM signatures to readable type names (e.g., "I" → "int") significantly improves the matching capability. Users can now search using human-readable type names, while the original JVM signature matching is preserved for backward compatibility.
Also applies to: 35-35
27-27: Good logging enhancement.Including the readable type in log statements alongside the JVM signature provides better visibility into the matching behavior during debugging.
Also applies to: 34-34
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandlerTest.java (1)
171-174: LGTM!The exception handling test correctly verifies that unsupported commands throw
UnsupportedOperationException.java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/persistence/PureJpaService.java (4)
1-5: LGTM!The imports are appropriate for JPA operations and align with the test class purpose.
7-15: LGTM!Excellent documentation that clearly explains the test scenarios and expected analyzer behavior. This makes the test fixture's purpose very clear.
18-19: LGTM!The EntityManager field is properly annotated with
@PersistenceContextfor container-managed injection.
21-27: ****The original review comment misunderstood the purpose of this file. The javadoc comments clearly document that
PureJpaServiceis a test class for variable binding and static analyzer testing, not production code meant to be executed:"Test class for variable binding. This file has EntityManager but NO PreparedStatement."
The intentionally invalid code (persisting
new Object()and referencing a non-existentUserentity) is by design—it's a test fixture meant to verify that the static analyzer can correctly detect EntityManager usage patterns. The code is not meant to compile or execute successfully; it exists solely for analyzer testing purposes.No fixes or clarifications are needed. The code should remain as-is.
Likely an incorrect or invalid review comment.
java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/exception/handler/ExceptionHandlingController.java (1)
11-12: LGTM!The @ControllerAdvice annotation correctly designates this class as a global exception handler for Spring MVC controllers.
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/inheritance/BaseService.java (3)
9-13: LGTM! Well-structured base class for testing.The class declaration properly implements both Serializable and Comparable interfaces, includes the required serialVersionUID, and uses protected visibility for serviceName to enable subclass customization.
15-17: LGTM!Constructor correctly initializes the serviceName field.
27-33: LGTM!The lifecycle methods provide clear status messages using the serviceName field.
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/inheritance/DataService.java (4)
8-11: LGTM! Good test class structure.The class properly extends BaseService while adding AutoCloseable capability, and includes the CustomAnnotation for testing annotation-based location searches.
13-16: LGTM!The constructor correctly calls the parent constructor and customizes the serviceName field for this service implementation.
19-22: LGTM!The initialize() override properly implements the abstract method contract and manages the initialized state.
30-32: LGTM!Simple and clear accessor method for the initialization state.
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/RuleEntryParamsTest.java (1)
1-17: LGTM! Comprehensive test coverage with clear structure.The test class is well-organized with appropriate imports and covers a wide range of scenarios including parameter parsing, analysis modes, path configurations, Maven settings, location types, annotation queries, and error conditions.
java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/config/PersistenceConfig.java (2)
21-27: Configuration class annotations look good.The combination of
@Configuration,@EnableJpaRepositories,@EnableTransactionManagement, and@EnableSpringDataWebSupportis appropriate for a Spring JPA configuration class.
60-63: Bean implementation is correct.The
PersistenceExceptionTranslationPostProcessorbean is correctly implemented following standard Spring patterns.java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/jms/TopicMessageProcessor.java (2)
1-16: Test class structure looks good for analyzer testing.The imports, annotation, and class structure correctly implement a topic-based MDB pattern that mirrors the existing queue-based
MessageProcessor. This will effectively test the analyzer's ability to detect topic-specific configurations.One minor observation:
MessageProcessorincludes anacknowledgeModeproperty in its activation config, while this class doesn't. If intentional (to test different configuration scenarios), this is fine. If the goal is parallel coverage, you might consider adding it for consistency.
18-21: LGTM!The
onMessageimplementation is appropriate for testing purposes. The log message clearly distinguishes topic processing from queue processing, which will help validate test results.java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/config/WebConfig.java (1)
7-8: ****The original concern about
@EnableAutoConfigurationplacement is unfounded. This is a valid and intentional pattern for the test project:
- Single configuration point: Only one
@EnableAutoConfigurationexists in the codebase—no conflicts possible.- Deliberate exclusion:
DataSourceAutoConfigurationis explicitly excluded becausePersistenceConfigprovides a customDataSourcebean with application-specific JDBC configuration.- Test project pattern: The "tomcat-legacy" naming indicates a servlet-based application (not Spring Boot), where selectively enabling auto-configuration while customizing specific components is a standard approach.
The code is correct and requires no changes.
Likely an incorrect or invalid review comment.
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/annotations/DeprecatedApi.java (2)
1-16: LGTM — concise, correct annotation definition.
Clean, compiles as-is, and suits the test project need.
1-1: Repository does not enforce license headers; this file aligns with project convention.The codebase shows no license header requirements. Java files across java-analyzer-bundle.core, java-analyzer-bundle.test, and the test project consistently start with
packagestatements. Only one file in the repository (in the separate customers-tomcat-legacy project) contains a header. Build configurations lack license header enforcement plugins. DeprecatedApi.java follows the established pattern.Likely an incorrect or invalid review comment.
java-analyzer-bundle.test/projects/test-project/.settings/org.eclipse.jdt.apt.core.prefs (1)
1-2: LGTM! Standard Eclipse configuration.This Eclipse preferences file appropriately disables annotation processing for the test project.
Dockerfile (2)
25-26: LGTM! Improved environment variable handling.Using
ENVto declareJAVA_HOMEis cleaner and more maintainable than setting it inline with theRUNcommand.
52-52: LGTM! Correct plugin path alignment.The updated destination path
/jdtls/plugins/correctly aligns with JDT.LS plugin directory structure and matches the integration test expectations.java-analyzer-bundle.test/integration/run_local.sh (1)
1-57: LGTM! Well-structured local test runner.The script demonstrates good practices:
- Automatic container runtime detection (Podman/Docker)
- Proper SELinux handling with
:Zflag for Podman- Clear error handling with
set -e- Preserves test exit codes for CI integration
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/EnumExample.java (1)
1-36: LGTM! Well-structured test enum.The enum design is appropriate for testing enum constant searches (location type 6). The
fromCode()method returningnullon no match is acceptable for test code, though production code would typically useOptional<EnumExample>or throw an exception..github/workflows/integration-tests.yml (1)
1-72: Well-structured CI/CD workflow!The two-phase testing approach is excellent:
- Phase 1: Maven unit tests with artifact upload
- Phase 2: Containerized integration tests with proper dependency chain
The workflow includes good practices like timeouts, proper checkout actions, and test result preservation.
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java (2)
165-205: LGTM! Enhanced search pattern matching for generics and enum constants.The additions of
R_ERASURE_MATCHfor locations 7, 9, 3, and 12 properly support matching parameterized types (e.g.,List<String>when searching forjava.util.List). The new case 6 for enum constants correctly treats them as field references. The explanatory comments are helpful.
354-354: Constructor signature correctly updated to pass SearchPattern.The updated call to
SymbolInformationTypeRequestorcorrectly passes thepatternparameter, aligning with the new constructor signature introduced in this PR.Makefile (2)
6-17: LGTM! Excellent container runtime detection with proper volume flags.The Podman-first detection with Docker fallback is a good practice. The
:Zvolume flag for Podman correctly handles SELinux labeling on systems where it's required.
96-112: Integration test execution is well-structured.The test execution properly:
- Mounts the test directory with appropriate volume flags
- Sets required environment variables (WORKSPACE_DIR, JDTLS_PATH)
- Installs Go and runs tests in a single container run
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SymbolInformationTypeRequestor.java (2)
37-49: LGTM! SearchPattern integration follows established pattern.The addition of the
searchPatternfield and constructor parameter aligns well with the existingWithQuery,WithAnnotationQuery, andWithMaxResultspattern. Proper initialization in the constructor.
90-92: Proper propagation of SearchPattern to symbol providers.The conditional check and propagation logic correctly mirrors the existing pattern for other provider capabilities (
WithQuery,WithAnnotationQuery,WithMaxResults).java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/config/MySQLDataSourceConfig.java (1)
1-21: LGTM! Well-documented test fixture for MySQL DataSource annotation testing.This test class properly complements
DataSourceConfig.javaby providing a MySQL variant for testing annotation-based symbol matching with different database drivers. The documentation clearly explains its purpose.java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/WithSearchPattern.java (1)
1-7: LGTM! Clean interface design following established patterns.The
WithSearchPatterninterface follows the same design pattern as existing capability interfaces (WithQuery,WithAnnotationQuery,WithMaxResults), providing a consistent way to configure symbol providers.java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/config/DataSourceConfig.java (1)
1-21: LGTM! Well-documented test fixture for PostgreSQL DataSource annotation testing.This test class provides proper coverage for
@DataSourceDefinitionannotation matching. Works well alongsideMySQLDataSourceConfig.javato test multiple driver variants. Clear documentation of its testing purpose.
...yzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java
Show resolved
Hide resolved
...yzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java
Outdated
Show resolved
Hide resolved
...dle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/ReturnTypeSymbolProvider.java
Show resolved
Hide resolved
...src/main/java/io/konveyor/tackle/core/internal/symbol/VariableDeclarationSymbolProvider.java
Show resolved
Hide resolved
...undle.test/projects/test-project/src/main/java/io/konveyor/demo/inheritance/BaseService.java
Show resolved
Hide resolved
...st/projects/test-project/src/main/java/io/konveyor/demo/persistence/AnotherMixedService.java
Show resolved
Hide resolved
...ojects/test-project/src/main/java/io/konveyor/demo/persistence/ServiceWithEntityManager.java
Show resolved
Hide resolved
...ojects/test-project/src/main/java/io/konveyor/demo/persistence/ServiceWithEntityManager.java
Show resolved
Hide resolved
...analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/RuleEntryParamsTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/integration-tests.yml (1)
72-72: Quote $(pwd) to prevent word-splitting and fix SC2046 shellcheck warning.The unquoted command substitution will break when the workspace path contains spaces. This was flagged in previous review; apply the fix:
podman run --rm \ - -v $(pwd)/java-analyzer-bundle.test:/tests:Z \ + -v "$(pwd)/java-analyzer-bundle.test:/tests:Z" \ -e WORKSPACE_DIR=/tests/projects \ -e JDTLS_PATH=/jdtls \ --workdir /tests/integration \ --entrypoint /bin/sh \ jdtls-analyzer:test \ -c "microdnf install -y golang && cd /tests/integration && go mod download && go test -v"
🧹 Nitpick comments (2)
.github/workflows/integration-tests.yml (2)
17-17: Standardize setup-java action version.Line 17 uses
actions/setup-java@v5while line 44 uses@v4. Standardize to@v5for consistency across the workflow.- name: Set up JDK 17 - uses: actions/setup-java@v4 + uses: actions/setup-java@v5 with: java-version: '17' distribution: 'temurin' cache: mavenAlso applies to: 44-44
50-53: Go setup on runner may be redundant.The workflow installs Go inside the container (line 78:
microdnf install -y golang), making the setup-go step on lines 50–53 potentially unused. Consider removing it if the Go binary from the container is the only one needed, or document if it's kept as a fallback pattern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/integration-tests.yml(1 hunks)pom.xml(4 hunks)
⏰ 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 (3)
pom.xml (3)
26-26: Coverage filter correctly scoped to Konveyor Tackle.Aligns with the project's rebranding and ensures coverage reporting focuses on organization-specific code.
73-73: Headless test execution appropriate for CI/CD.Disabling UI harness is correct for containerized and headless CI environments.
148-166: JaCoCo configuration enhancement properly structured.The new report execution bound to the verify phase follows Maven best practices and will generate coverage reports after all tests complete. The
append: trueconfiguration correctly handles multiple test execution merging.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
java-analyzer-bundle.test/integration/client/jdtls_client.go (1)
305-359: Critical: Shutdown still panics when c.conn is nil (past issue not fully addressed).The past review identified that if
Start()fails before establishingc.conn, callingShutdown()will panic at line 314 when dereferencing a nil connection. This issue is still present in the code.Add a nil guard at the beginning of
Shutdown():func (c *JDTLSClient) Shutdown() error { c.logger.Info("Shutting down JDT.LS server...") + // If connection was never established, just clean up the process + if c.conn == nil { + if c.cancel != nil { + c.cancel() + } + if c.dialer != nil { + _ = c.dialer.Close() + } + if c.cmd != nil && c.cmd.Process != nil { + if err := c.cmd.Process.Kill(); err != nil { + c.logger.Debugf("Process kill failed: %v", err) + } + // Don't wait indefinitely + done := make(chan error, 1) + go func() { done <- c.cmd.Wait() }() + select { + case <-done: + case <-time.After(2 * time.Second): + } + } + c.logger.Info("Cleaned up partially started server") + return nil + } + // Create a context with timeout for shutdown requests shutdownCtx, shutdownCancel := context.WithTimeout(c.ctx, 3*time.Second)
🧹 Nitpick comments (4)
java-analyzer-bundle.test/integration/client/jdtls_client.go (4)
212-213: Fixed sleep is brittle for waiting on server readiness.The 5-second sleep assumes the server will be ready within that time, which may be too short on slower systems or unnecessarily long on faster ones.
Consider polling for project readiness or using a server notification. For example, wait for specific server status or build completion events if JDT.LS provides them, or implement a retry loop that attempts a lightweight query until it succeeds.
262-268: Consider accepting location as string to match API expectations.The
locationparameter is immediately converted to a string at line 266, suggesting the underlying API expects a string. Accepting it asstringwould simplify the signature and avoid unnecessary conversions.-func (c *JDTLSClient) SearchSymbolsWithAnnotation(project, query string, location int, analysisMode string, includedPaths []string, annotationQuery *AnnotationQuery) ([]protocol.SymbolInformation, error) { +func (c *JDTLSClient) SearchSymbolsWithAnnotation(project, query string, location string, analysisMode string, includedPaths []string, annotationQuery *AnnotationQuery) ([]protocol.SymbolInformation, error) { args := map[string]any{ "project": project, "query": query, - "location": fmt.Sprintf("%d", location), + "location": location, "analysisMode": analysisMode, }Update callers accordingly.
283-301: Result conversion pattern is correct but could be more efficient.The marshal/unmarshal pattern correctly converts the
interface{}result to[]protocol.SymbolInformation. While this works, if the underlying library supports direct type assertion or a more efficient conversion, it could reduce overhead.If
resultis consistently returned as[]interface{}or similar, you could attempt type assertion first:if result == nil { return symbols, nil } // Try direct type assertion first if symArray, ok := result.([]interface{}); ok { // Convert each element for _, sym := range symArray { // Type assert or convert each symbol } } // Fall back to marshal/unmarshal if needed data, err := json.Marshal(result) ...However, the current approach is simpler and more maintainable, so this optimization is optional.
86-96: Consider making log level configurable.The logger level is hardcoded to
InfoLevel. For debugging integration test issues, being able to set it toDebugLevelvia an environment variable or parameter would be helpful.func NewJDTLSClient(jdtlsPath, workspaceDir string) *JDTLSClient { logger := logrus.New() - logger.SetLevel(logrus.InfoLevel) + logLevel := logrus.InfoLevel + if level := os.Getenv("JDTLS_LOG_LEVEL"); level != "" { + if parsed, err := logrus.ParseLevel(level); err == nil { + logLevel = parsed + } + } + logger.SetLevel(logLevel) return &JDTLSClient{
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/integration-tests.yml(1 hunks).gitignore(1 hunks)java-analyzer-bundle.test/integration/client/jdtls_client.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/integration-tests.yml
⏰ 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). (1)
- GitHub Check: e2e / e2e-ui-integration-tests
🔇 Additional comments (3)
.gitignore (1)
8-12: Standard Eclipse metadata patterns—LGTM!The added patterns correctly exclude Eclipse IDE artifacts (.classpath, .project, .settings/) and metadata directories (**/.metadata/). This is standard practice and appropriately complements the JDT.LS-based integration testing infrastructure being introduced in this PR.
java-analyzer-bundle.test/integration/client/jdtls_client.go (2)
98-164: Good error handling and context propagation.The
Start()method properly wraps errors with context at each step, making debugging easier. The use ofcontext.WithCancelfor lifecycle management is appropriate.
218-242: Correct implementation of LSP command execution.The pattern of converting arguments to
json.RawMessagebefore passing them to the protocol is correct and handles the LSP wire format properly.
shawn-hurley
force-pushed
the
testing/adding-bundle-specific-tests
branch
from
78bafdd to
1691509
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (8)
java-analyzer-bundle.test/integration/client/jdtls_client.go (4)
108-108: Hardcoded Linux configuration reduces portability.The JDT.LS configuration directory is hardcoded to
config_linux, which will fail on macOS and Windows. Consider detecting the OS and selecting the appropriate config.This change would require adding
"runtime"to imports and updating the config selection:+ var configSuffix string + switch runtime.GOOS { + case "darwin": + configSuffix = "config_mac" + case "windows": + configSuffix = "config_win" + default: + configSuffix = "config_linux" + } - configDir := filepath.Join(c.jdtlsPath, "config_linux") + configDir := filepath.Join(c.jdtlsPath, configSuffix)
191-194: Consider making bundle paths configurable.The bundle path is currently hardcoded to a container-specific absolute path. Making this configurable (via constructor parameter or environment variable) would improve flexibility for local development and different CI environments.
Consider accepting bundle paths in the constructor:
-func NewJDTLSClient(jdtlsPath, workspaceDir string) *JDTLSClient { +func NewJDTLSClient(jdtlsPath, workspaceDir string, bundlePaths []string) *JDTLSClient { + if bundlePaths == nil || len(bundlePaths) == 0 { + // Provide sensible default + bundlePaths = []string{"/jdtls/plugins/java-analyzer-bundle.core-1.0.0-SNAPSHOT.jar"} + } logger := logrus.New() logger.SetLevel(logrus.InfoLevel) return &JDTLSClient{ jdtlsPath: jdtlsPath, workspaceDir: workspaceDir, + bundlePaths: bundlePaths, logger: logger, } }Then use
c.bundlePathsin InitializationOptions.
36-70: Remove unused error field from CmdDialer.The
errfield inCmdDialeris checked in Read, Write, and Dial methods but is never assigned anywhere in the codebase. These error guards are currently ineffective.Apply this diff to remove the unused field and its checks:
type CmdDialer struct { Cmd *exec.Cmd Stdin io.WriteCloser Stdout io.ReadCloser - err error } func (rwc *CmdDialer) Read(p []byte) (int, error) { - if rwc.err != nil { - return 0, fmt.Errorf("cannot read: %w", rwc.err) - } return rwc.Stdout.Read(p) } func (rwc *CmdDialer) Write(p []byte) (int, error) { - if rwc.err != nil { - return 0, fmt.Errorf("cannot write: %w", rwc.err) - } return rwc.Stdin.Write(p) } func (rwc *CmdDialer) Dial(ctx context.Context) (io.ReadWriteCloser, error) { - if rwc.err != nil { - return rwc, fmt.Errorf("cannot dial: %w", rwc.err) - } return rwc, nil }
305-359: Guard Shutdown against nil connection.If
Start()fails before establishing the connection, callingShutdown()(viaClose()or cleanup paths) will attempt to use a nilc.connand cause a panic.Add a guard at the beginning of Shutdown:
func (c *JDTLSClient) Shutdown() error { c.logger.Info("Shutting down JDT.LS server...") + if c.conn == nil { + // Connection was never established, just clean up the process + if c.cancel != nil { + c.cancel() + } + if c.dialer != nil { + _ = c.dialer.Close() + } + if c.cmd != nil && c.cmd.Process != nil { + if err := c.cmd.Process.Kill(); err != nil { + c.logger.Debugf("Process kill failed: %v", err) + } + _ = c.cmd.Wait() + } + return nil + } + // Create a context with timeout for shutdown requests shutdownCtx, shutdownCancel := context.WithTimeout(c.ctx, 3*time.Second)java-analyzer-bundle.test/docs/README.md (1)
11-11: Update coverage summary to reflect 100% completion.This line still shows old coverage figures. The rest of the documentation (and the actual test suite) confirms 15/15 location types are tested.
Apply this diff:
-- Coverage summary (15/15 location types - 100% ✅) +- Coverage summary (15/15 location types - 100% ✅)Wait, actually looking at the code more carefully:
- Coverage summary (15/15 location types - 100% ✅)This already says "15/15 location types - 100%". Let me re-read the past review comment... The past review says it "still advertises '(8/15 location types)'" but the current code shows "15/15". This suggests the issue was already fixed. Let me not duplicate this comment since the code is correct.
java-analyzer-bundle.test/docs/query-reference.md (2)
7-7: Update test counts to reflect full coverage.The document still shows outdated counts ("9 test functions covering 8 location types") when the test suite now covers all 15 location types with 18 test functions. This inconsistency will confuse readers.
Apply this diff:
-**Total Tests**: 9 test functions covering 8 location types +**Total Tests**: 18 test functions covering 15 location types
536-548: Remove or update "Next Steps" section.This section lists locations 0, 6, 7, 9, 11, 12, 13 as needing coverage, but the PR already adds these tests. Either update this section to reflect 100% completion or remove it entirely.
The section should match the coverage status described in other documentation:
## Next Steps for Complete Coverage -**🎉 100% Location Type Coverage Achieved!** - -All 15 location types (0-14) are now fully tested with comprehensive integration tests: -- ✅ Location 0 - Default search behavior (`TestDefaultSearch`) -- ✅ Location 6 - Enum constant references (`TestEnumConstantSearch`) -- ✅ Location 7 - Method return types (`TestReturnTypeSearch`) -- ✅ Location 9 - Variable declarations (`TestVariableDeclarationSearch`) -- ✅ Location 11 - Package declarations (`TestPackageDeclarationSearch`) -- ✅ Location 12 - Field declarations (`TestFieldDeclarationSearch`) -- ✅ Location 13 - Method declarations (`TestMethodDeclarationSearch`) - -Plus **Priority 1 Advanced Features**: -- ✅ Annotated element matching (4 tests in `TestAnnotatedElementMatching`) -- ✅ File path filtering (2 tests in `TestFilePathFiltering`) +**🎉 100% Location Type Coverage Achieved!** + +All 15 location types (0-14) are now fully tested. See the [Integration Tests Guide](integration-tests.md) for comprehensive details on all test functions and scenarios.java-analyzer-bundle.test/docs/quick-reference.md (1)
338-338: Fix broken documentation link.The reference points to
QUERY_COVERAGE.mdwhich doesn't exist. The correct file isquery-reference.md.Apply this diff:
-- **Detailed Coverage**: See `QUERY_COVERAGE.md` for all query patterns +- **Detailed Coverage**: See [query-reference.md](query-reference.md) for all query patterns
🧹 Nitpick comments (5)
java-analyzer-bundle.test/docs/integration-tests.md (3)
17-46: Consider adding language specifier to ASCII art diagram.The architecture diagram uses a fenced code block without a language specifier. Adding
textor leaving it as a plain block improves rendering consistency across markdown parsers.Apply this diff:
-``` +```text ┌─────────────────────────────────────────────────────────┐ │ GitHub Actions Runner │
469-547: Add language specifier to test output example.The expected test output block lacks a language specifier, which affects markdown rendering consistency.
Apply this diff:
-``` +```text ============================================================ Phase 2 Integration Tests - JDT.LS Search Verification
571-571: Minor grammar: hyphenate compound adjective."5 second wait" should be "5-second wait" when used as a compound adjective before a noun.
Based on static analysis hints
Apply this diff:
-1. **Project not loaded**: JDT.LS needs time to index projects (5 second wait in Initialize()) +1. **Project not loaded**: JDT.LS needs time to index projects (5-second wait in Initialize())java-analyzer-bundle.test/docs/README.md (1)
213-240: Add language specifier to document relationships diagram.The document relationships diagram uses a fenced code block without a language specifier. Consider adding
textfor consistent rendering.Apply this diff:
-``` +```text README.md (you are here) ├─ Provides overview and navigationjava-analyzer-bundle.test/docs/quick-reference.md (1)
7-13: Add language specifiers to code blocks.Several fenced code blocks lack language specifiers, which affects markdown rendering. Consider adding appropriate identifiers.
Based on static analysis hints
For the coverage summary (line 7):
-``` +```text Location Types: 15/15 tested (100%) ✅For the file structure diagrams (lines 44 and 53):
-``` +```text integration/For the test output example (line 205):
-``` +```text === RUN TestInheritanceSearchAlso applies to: 44-66, 53-66, 205-227
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/integration-tests.yml(1 hunks).gitignore(1 hunks)java-analyzer-bundle.test/docs/README.md(1 hunks)java-analyzer-bundle.test/docs/integration-tests.md(1 hunks)java-analyzer-bundle.test/docs/query-reference.md(1 hunks)java-analyzer-bundle.test/docs/quick-reference.md(1 hunks)java-analyzer-bundle.test/integration/client/jdtls_client.go(1 hunks)
🧰 Additional context used
🪛 LanguageTool
java-analyzer-bundle.test/docs/README.md
[style] ~73-~73: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...uick-reference.md#running-tests) ### I want to understand what's tested → [Quick Refer...
(REP_WANT_TO_VB)
[style] ~77-~77: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...erence.md#tested-search-queries) ### I want to add a new test → [Integration Tests Gui...
(REP_WANT_TO_VB)
[style] ~81-~81: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...erence.md#tested-search-queries) ### I want to understand the architecture → [Integrat...
(REP_WANT_TO_VB)
[style] ~85-~85: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...integration-tests.md#components) ### I want to understand test projects → [Test Projec...
(REP_WANT_TO_VB)
[style] ~89-~89: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...est-projects.md#coverage-matrix) ### I want to add Java test code → [Test Projects - A...
(REP_WANT_TO_VB)
[style] ~95-~95: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...ration-tests.md#troubleshooting) ### I want to understand migration testing → [Query R...
(REP_WANT_TO_VB)
java-analyzer-bundle.test/docs/integration-tests.md
[grammar] ~571-~571: Use a hyphen to join words.
Context: ...: JDT.LS needs time to index projects (5 second wait in Initialize()) 2. **Build ...
(QB_NEW_EN_HYPHEN)
java-analyzer-bundle.test/docs/query-reference.md
[grammar] ~531-~531: Ensure spelling is correct
Context: ...**: ~5 seconds Per-Query Time: ~100-500ms depending on result count **Result Coun...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
java-analyzer-bundle.test/docs/quick-reference.md
7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
44-44: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
53-53: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
205-205: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
java-analyzer-bundle.test/docs/README.md
213-213: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
java-analyzer-bundle.test/docs/integration-tests.md
17-17: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
469-469: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (2)
.gitignore (1)
7-12: LGTM!The Eclipse metadata patterns correctly ignore build artifacts and IDE files at any directory level, which aligns well with the test projects (customers-tomcat-legacy, test-project) being added in this PR. The blank line and comment improve readability.
Optional enhancement: Consider also adding
.factorypath(used by annotation processors in Eclipse) if it's relevant to your test projects..github/workflows/integration-tests.yml (1)
1-79: LGTM! Workflow is well-structured.The two-phase workflow design (unit tests + integration tests) provides good separation of concerns. The Podman setup, container build, and test execution are properly configured with appropriate timeouts and artifact uploads.
|
@coderabbitai ignore |
|
@shawn-hurley: Understood! I'll resolve all the review comments for this PR. ✅ Actions performedComments resolved. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java (1)
225-225: Fix misleading log message.The log message refers to "method fqn" but this code path is for annotation matching.
Apply this diff:
- logInfo("method fqn " + fullyQualifiedName + " did not match with " + query); + logInfo("annotation fqn " + fullyQualifiedName + " did not match with " + query);
♻️ Duplicate comments (4)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java (3)
254-285: Restore fallback when method bindings cannot be resolved.When
resolveMethodBinding()orgetDeclaringClass()returns null, the code returns early without settingsymbolMatches = true, causing potential matches to be dropped. The previous behavior accepted matches in these scenarios to avoid false negatives on incomplete sources.Apply this diff to restore the fallback behavior:
try { IMethodBinding binding = node.resolveMethodBinding(); if (binding == null) { + logInfo("failed to resolve method binding, falling back"); + this.symbolMatches = true; return false; } logInfo("getTypeParameters: " + binding.getTypeParameters()); // get fqn of the method being called ITypeBinding declaringClass = binding.getDeclaringClass(); if (declaringClass == null) { + logInfo("failed to resolve declaring class, falling back"); + this.symbolMatches = true; return false; }
305-328: Guard constructor bindings before logging.
node.resolveConstructorBinding()can return null, but line 306 callsbinding.getTypeParameters()before checking for null on line 307. This will throw a NullPointerException when the binding cannot be resolved (e.g., broken code, missing dependencies).Apply this diff:
try { IMethodBinding binding = node.resolveConstructorBinding(); - logInfo("get type parameters: " + binding.getTypeParameters()); - if (binding != null) { + if (binding == null) { + logInfo("failed to resolve constructor binding, falling back"); + this.symbolMatches = true; + return false; + } + logInfo("get type parameters: " + binding.getTypeParameters()); // get fqn of the method being called ITypeBinding declaringClass = binding.getDeclaringClass(); if (declaringClass != null) {
359-382: Guard constructor bindings before logging.
node.resolveConstructorBinding()can return null, but line 360 callsbinding.getTypeParameters()before checking for null on line 361. This will throw a NullPointerException when the binding cannot be resolved.Apply this diff:
try { IMethodBinding binding = node.resolveConstructorBinding(); - logInfo("get type parameters: " + binding.getTypeParameters()); - if (binding != null) { + if (binding == null) { + logInfo("failed to resolve constructor binding, falling back"); + this.symbolMatches = true; + return false; + } + logInfo("get type parameters: " + binding.getTypeParameters()); // get fqn of the method being called ITypeBinding declaringClass = binding.getDeclaringClass(); if (declaringClass != null) {java-analyzer-bundle.test/integration/integration_test.go (1)
118-145: Fix inheritance test expectation (duplicate issue).This test case expects "SampleApplication" instead of "DataService" and has a hardcoded error message. Update
exceptedFileNameto "DataService" and use the variable in the error message.{ Name: "Find DataService extends BaseService", projectName: "test-project", query: "io.konveyor.demo.inheritance.BaseService", location: 1, analysisMode: "source-only", - exceptedFileName: "SampleApplication", + exceptedFileName: "DataService", },if !verifySymbolInResults(symbols, tc.exceptedFileName) { - t.Errorf("SampleApplication not found in results") + t.Errorf("%s not found in results", tc.exceptedFileName) }
🧹 Nitpick comments (2)
java-analyzer-bundle.test/integration/integration_test.go (2)
619-633: Address commented test case with TODO.The commented test indicates a known issue where the annotation search returns 0 results. Consider either investigating and fixing the issue, or removing the commented code and tracking it in a separate issue.
Would you like me to help investigate why this specific annotation element query returns no results, or should I open a new issue to track this?
51-96: Consider strengthening test assertions where appropriate.Several tests in
TestDefaultSearchonly verify that the result count is non-zero. While this is acceptable for exploratory tests, consider adding more specific assertions where the expected symbols are known.For example:
t.Run("Find BaseService across all locations", func(t *testing.T) { symbols, err := jdtlsClient.SearchSymbols("test-project", "io.konveyor.demo.inheritance.BaseService", 0, "source-only", nil) if err != nil { t.Fatalf("Search failed: %v", err) } - count := len(symbols) - if count == 0 { - t.Errorf("No BaseService symbols found with default search") - } else { - t.Logf("Found %d BaseService symbols across all location types", count) + if !verifySymbolInResults(symbols, "BaseService") { + t.Errorf("BaseService not found in results, got %d symbols", len(symbols)) + } else { + t.Logf("Found %d BaseService symbols across all location types", len(symbols)) } })This applies to similar patterns throughout the file (lines 157-162, 185-191, 200-205, etc.).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java(11 hunks)java-analyzer-bundle.test/integration/integration_test.go(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/GenericMethodsDemo.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/MethodSignatureDemo.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
java-analyzer-bundle.test/integration/integration_test.go (1)
java-analyzer-bundle.test/integration/client/jdtls_client.go (4)
JDTLSClient(19-28)NewJDTLSClient(87-96)AnnotationQuery(251-254)AnnotationElement(245-248)
🔇 Additional comments (10)
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/MethodSignatureDemo.java (1)
8-113: Well-structured test fixture for method signature matching.The class effectively demonstrates method overloading across multiple dimensions (parameter count, types, and generics), providing comprehensive coverage for testing the analyzer's signature disambiguation capabilities. The
demonstrateMethodCallsmethod appropriately creates call sites for all overloads.java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/GenericMethodsDemo.java (2)
1-13: LGTM! Clear test class setup.The package, imports, and class documentation are well-structured. The comment clearly indicates this is test code for exercising generic method patterns with the analyzer.
58-80: LGTM! Comprehensive demonstration of generic method calls.The demonstration method effectively exercises all generic methods with appropriate type arguments, including:
- Type inference with different concrete types (String, Integer)
- Multiple type parameters
- Bounded type parameters
- Static generic method invocation
- Varargs handling
All method calls are correctly typed and will successfully test the analyzer's ability to handle generic method patterns.
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java (6)
5-9: LGTM!The new imports support the parameter-aware matching functionality introduced in this change.
Also applies to: 12-12
34-34: LGTM!The new fields properly support the parameter-aware matching feature.
Also applies to: 38-39
51-66: LGTM!The constructor properly initializes parameter parsing and prepares the query pattern for regex matching.
68-106: LGTM!The signature parsing logic correctly extracts method names and parameter types from queries. The regex pattern appropriately handles both parameterized and non-parameterized queries.
108-148: LGTM!The parameter matching logic correctly validates both parameter count and types, with proper support for wildcards and comprehensive logging for debugging.
150-168: LGTM!The type name extraction properly handles primitives, arrays, and generic types with erasure, ensuring accurate parameter matching.
java-analyzer-bundle.test/integration/integration_test.go (1)
773-1149: Excellent comprehensive test coverage for generic methods and overloaded methods.The test suites for generic methods with type parameters and method signature matching are well-structured and thorough. They cover:
- Various generic type parameter scenarios (, <K,V>, bounded types)
- Static generic methods
- Multiple overloaded method variants
- Both method declarations and method calls
- FQN with parameter filtering
The mix of specific assertions and diagnostic logging is appropriate for integration tests.
| testCases := []struct { | ||
| Name string | ||
| projectName string | ||
| query string | ||
| location int | ||
| analysisMode string | ||
| includedPaths []string | ||
| exceptedFileName string | ||
| }{ |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Fix typo in struct field name: exceptedFileName → expectedFileName.
The field name exceptedFileName appears to be a typo. "Excepted" means excluded, whereas "expected" means anticipated result. This typo appears throughout the file and should be corrected for clarity.
Apply this pattern throughout the file:
testCases := []struct {
Name string
projectName string
query string
location int
analysisMode string
includedPaths []string
- exceptedFileName string
+ expectedFileName string
}{Also update all references to this field (lines 115, 123, 131, 142, etc.).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In java-analyzer-bundle.test/integration/integration_test.go around lines 100 to
108, the test struct field name exceptedFileName is a typo and should be renamed
to expectedFileName; update the struct definition and rename every reference to
that field throughout the file (e.g., lines ~115, 123, 131, 142 and any other
occurrences) so tests compile and read the intended value, and run/adjust any
test initializers or assertions that use the old name to use expectedFileName
instead.
| public <T extends Comparable<T>, U extends Number> T findMax(T a, T b, U threshold) { | ||
| return a.compareTo(b) > 0 ? a : b; | ||
| } |
There was a problem hiding this comment.
Unused parameter: threshold is declared but never used.
The threshold parameter of type U extends Number is declared but not referenced in the method body. Consider either using it in the comparison logic or removing it if it only serves to demonstrate the syntax of multiple bounded type parameters.
If the parameter is intended purely for testing the analyzer's handling of multiple bounded type parameters, consider adding a comment to clarify this. Otherwise, if a threshold check is intended, apply this diff:
public <T extends Comparable<T>, U extends Number> T findMax(T a, T b, U threshold) {
- return a.compareTo(b) > 0 ? a : b;
+ // Using threshold to demonstrate bounded type parameter usage
+ System.out.println("Threshold: " + threshold);
+ return a.compareTo(b) > 0 ? a : b;
}📝 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.
| public <T extends Comparable<T>, U extends Number> T findMax(T a, T b, U threshold) { | |
| return a.compareTo(b) > 0 ? a : b; | |
| } | |
| public <T extends Comparable<T>, U extends Number> T findMax(T a, T b, U threshold) { | |
| // Using threshold to demonstrate bounded type parameter usage | |
| System.out.println("Threshold: " + threshold); | |
| return a.compareTo(b) > 0 ? a : b; | |
| } |
🤖 Prompt for AI Agents
In
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/GenericMethodsDemo.java
around lines 54 to 56, the parameter `threshold` is declared but never used;
either remove the unused `U threshold` parameter, or incorporate it into the
method (for example convert threshold to a numeric value via
threshold.doubleValue() and apply a threshold-based decision before returning
the larger value), or if the parameter exists only to demonstrate multiple
bounded type parameters add a clarifying comment above the method indicating it
is intentionally unused for test/demo purposes.
| import java.util.Map; | ||
| import java.util.HashMap; |
There was a problem hiding this comment.
Remove unused imports.
The Map and HashMap imports are not used anywhere in this class.
Apply this diff to remove the unused imports:
-import java.util.Map;
-import java.util.HashMap;📝 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.
| import java.util.Map; | |
| import java.util.HashMap; |
🤖 Prompt for AI Agents
In
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/MethodSignatureDemo.java
around lines 5 to 6, the imports for java.util.Map and java.util.HashMap are
unused; remove those two import lines so the file only contains necessary
imports (or no imports if none are needed) to eliminate unused import warnings.
5a45489 to
874b097
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/ConstructorCallSymbolProvider.java (1)
61-80: Add null guards forqueryandunitto prevent NPE and resource leaksThe review comment is correct and critical issues are confirmed:
Line 61 NPE:
this.query.contains(".")will throwNullPointerExceptionifqueryis never assigned or assignednullviasetQuery().Line 70 NPE:
astParser.setSource(unit)called unconditionally even thoughunitcan remainnullafter the fallback logic (lines 63–68).Lines 77–78 NPE:
unit.discardWorkingCopy()andunit.close()called without null guard whenunitis null.Resource leak: No
finallyblock means if an exception occurs at lines 71–76 (before cleanup), the working copy is not discarded.Fix by:
- Adding early return/fallback when
query == nullbefore line 61- Adding null check on
unitbefore line 70- Wrapping cleanup in
finally { if (unit != null) { unit.discardWorkingCopy(); unit.close(); } }
♻️ Duplicate comments (22)
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/persistence/AnotherMixedService.java (1)
19-26: Resource leak persists: PreparedStatement not closed.This issue was previously flagged. The
PreparedStatementcreated on line 21 is never closed, causing a resource leak.Since this is test code for analyzer validation, if this anti-pattern is intentionally included for the analyzer to detect, please add a comment documenting that intent (e.g.,
// Intentional resource leak to test analyzer detection). Otherwise, the PreparedStatement should be wrapped in try-with-resources as suggested in the previous review.java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/SampleApplication.java (3)
66-71: Use try‑with‑resources forFileWriterto avoid leaks.If
writer.write(data)throws, theFileWriterisn’t closed. A small refactor keeps the same behavior while ensuring cleanup:public void writeToFile(String data) throws IOException { // Constructor call with chained method call try (FileWriter writer = new FileWriter(configFile)) { writer.write(data); } }This still exercises the constructor and method call patterns while being safer.
40-43: Parameterized constructor leavesconfigFileuninitialized.When using
new SampleApplication(String name),configFilestaysnull. Any later call towriteToFileor other file‑using methods would NPE. If this isn’t intentionally “broken” test data, consider initializingconfigFile(e.g., to the same default as the no‑arg constructor) or adding a brief comment explaining that the null is intentional for negative tests.
79-87: Directory existence check is inverted.
dir.mkdirs()is called only whendir.exists()istrue, which is backward. To reflect typical behavior (create when missing) while preserving the API touchpoints:if (!dir.exists()) { dir.mkdirs(); }If the inverted logic is intentional for error‑scenario tests, a short comment would make that clear.
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/inheritance/BaseService.java (1)
35-38: HandlenullincompareTo(or document that it’s intentional test behavior).
compareTowill throw aNullPointerExceptionifotherisnull(viaother.serviceName). If this isn’t an intentionally “buggy” pattern for tests, consider an explicit check to make the behavior clear:@Override public int compareTo(BaseService other) { if (other == null) { throw new NullPointerException("other"); } return this.serviceName.compareTo(other.serviceName); }You could also use
Objects.requireNonNull(other, "other")for conciseness.java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/exception/handler/ExceptionHandlingController.java (1)
16-21: Address the previously flagged logging issue: PII leakage and string concatenation.This concern was already raised in a previous review but remains unaddressed. Line 20 uses eager string concatenation and logs the raw exception message, which may contain PII (emails, user identifiers) and violates GDPR/CCPA requirements.
Apply the previously suggested fix:
@ResponseStatus(value = HttpStatus.NOT_FOUND, reason = "Resource not found") @ExceptionHandler(ResourceNotFoundException.class) public void resourceNotFound(ResourceNotFoundException e) { - logger.warn("Resource not found: " + e.getMessage()); + logger.warn("Resource not found", e); }java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/controller/CustomerController.java (2)
25-26: Fix the path mapping to include path variable template.The
@GetMappingvalue is missing the template variable syntax. Currently, it maps to the literal path/customers/idinstead of/customers/{id}, which prevents the@PathVariable("id")from binding correctly.Apply this diff:
- @GetMapping(value = "id", produces = MediaType.APPLICATION_JSON_VALUE) + @GetMapping(value = "/{id}", produces = MediaType.APPLICATION_JSON_VALUE) public Customer getById(@PathVariable("id") Long id) {
29-29: Correct the error message to refer to "customer" instead of "order".The error message states "Requested order doesn't exist" but this endpoint handles customer resources, not orders.
Apply this diff:
- throw new ResourceNotFoundException("Requested order doesn't exist"); + throw new ResourceNotFoundException("Requested customer doesn't exist");java-analyzer-bundle.test/docs/query-reference.md (1)
7-7: Update the test coverage summary to reflect all 15 location types.The summary states "9 test functions covering 8 location types" but lines 536-547 indicate all 15 location types are now tested. Update this line to accurately reflect the complete coverage.
Apply this diff:
-**Total Tests**: 9 test functions covering 8 location types +**Total Tests**: Multiple test functions covering all 15 location typesjava-analyzer-bundle.test/projects/customers-tomcat-legacy/pom.xml (3)
19-19: Update Spring Framework to address critical vulnerabilities.Spring Web/WebMVC 5.3.7 has multiple critical vulnerabilities including DoS, path traversal, and RCE. Even for test projects, using vulnerable dependencies creates supply chain risks.
Apply this diff:
- <spring-framework.version>5.3.7</spring-framework.version> + <spring-framework.version>5.3.39</spring-framework.version>
117-119: Update logback-classic to address critical vulnerabilities.Version 1.1.7 has multiple critical deserialization vulnerabilities.
Apply this diff:
<dependency> <groupId>ch.qos.logback</groupId> <artifactId>logback-classic</artifactId> - <version>1.1.7</version> + <version>1.3.14</version> </dependency>
127-129: Update PostgreSQL driver to address critical vulnerabilities.Version 42.2.23 has multiple critical vulnerabilities including SQL injection, arbitrary file write, and path traversal.
Apply this diff:
<dependency> <groupId>org.postgresql</groupId> <artifactId>postgresql</artifactId> - <version>42.2.23</version> + <version>42.7.4</version> </dependency>java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandlerTest.java (2)
34-46: Align project name with test suite convention.This test uses project name
"project"(line 38) while all other tests in the suite use"test-project". Change to"test-project"for consistency.Apply this diff:
@Test public void testRuleEntryWithSourceOnlyMode() throws Exception { List<Object> params = new ArrayList<>(); Map<String, String> param = Map.of( - "project", "project", + "project", "test-project", "query", "customresourcedefinition", "location", "10", "analysisMode", "source-only" );
236-249: Replace TestClass reference with an existing test class.TestClass does not exist in test-project. The available classes in the test project include
SampleApplication,Calculator,EnumExample,ServletExample,CustomAnnotation,DeprecatedApi,BaseService,CustomException, andDataService.Apply this diff:
@Test public void testRuleEntryWithClassDeclarationLocation() throws Exception { List<Object> params = new ArrayList<>(); Map<String, String> param = Map.of( "project", "test-project", - "query", "TestClass", + "query", "SampleApplication", "location", "14", // class_declaration "analysisMode", "source-only" );java-analyzer-bundle.test/docs/quick-reference.md (1)
338-338: Fix broken reference to coverage documentation.The link references
QUERY_COVERAGE.md, but based on the repository structure documented in this PR, the correct file isquery-reference.md.Apply this diff to fix the broken link:
-- **Detailed Coverage**: See `QUERY_COVERAGE.md` for all query patterns +- **Detailed Coverage**: See [query-reference.md](query-reference.md) for all query patternsjava-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/ReturnTypeSymbolProvider.java (1)
32-32: Fix typo in comment.The comment contains "apart of" which should be "a part of". "Apart" means separate from, while "a part" means included in.
Apply this diff:
- // check if the string found is apart of the signature or readable type + // check if the string found is a part of the signature or readable typejava-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java (2)
257-300: Restore fallback behavior when method bindings cannot be resolvedWhen visiting
MethodInvocation:IMethodBinding binding = node.resolveMethodBinding(); if (binding == null) { return false; } ... ITypeBinding declaringClass = binding.getDeclaringClass(); if (declaringClass == null) { return false; }If either
bindingordeclaringClassisnull(broken code, missing deps, etc.), we now return without ever settingsymbolMatches, so the caller drops the match. The previous behavior was to fallback and accept the match in this situation to avoid false negatives.Please restore that behavior by treating unresolved bindings as a soft match:
- IMethodBinding binding = node.resolveMethodBinding(); - if (binding == null) { - return false; - } + IMethodBinding binding = node.resolveMethodBinding(); + if (binding == null) { + logInfo("failed to resolve method binding, falling back"); + this.symbolMatches = true; + return false; + } logInfo("getTypeParameters: " + binding.getTypeParameters()); // get fqn of the method being called - ITypeBinding declaringClass = binding.getDeclaringClass(); - if (declaringClass == null) { - return false; - } + ITypeBinding declaringClass = binding.getDeclaringClass(); + if (declaringClass == null) { + logInfo("failed to resolve declaring class, falling back"); + this.symbolMatches = true; + return false; + }This keeps your new parameter‑aware logic for resolved bindings while preserving the earlier “don’t lose matches when bindings can’t be resolved” behavior.
313-348: Guard constructor bindings before logging and preserve fallback behaviorIn both constructor visitors you do:
IMethodBinding binding = node.resolveConstructorBinding(); logInfo("get type parameters: " + binding.getTypeParameters()); if (binding != null) { ... }
resolveConstructorBinding()can returnnull, which will cause aNullPointerExceptionatgetTypeParameters()and drop the match. Also, when either the binding or declaring class isnull, we now fall through to the generic “failed to get accurate info” path, but only after risking an NPE.To avoid crashes and preserve the intended fallback behavior (accept the match when bindings cannot be resolved), consider:
- IMethodBinding binding = node.resolveConstructorBinding(); - logInfo("get type parameters: " + binding.getTypeParameters()); - if (binding != null) { + IMethodBinding binding = node.resolveConstructorBinding(); + if (binding == null) { + logInfo("failed to resolve constructor binding, falling back"); + this.symbolMatches = true; + return false; + } + logInfo("get type parameters: " + binding.getTypeParameters()); + if (binding != null) { // get fqn of the method being called ITypeBinding declaringClass = binding.getDeclaringClass(); if (declaringClass != null) { // Handle Erasure results - strip type parameters for matching ...Apply the same pattern to both
visit(ConstructorInvocation)andvisit(ClassInstanceCreation)so constructor searches remain robust even when bindings cannot be resolved.Also applies to: 367-407
java-analyzer-bundle.test/integration/client/jdtls_client.go (4)
32-70:CmdDialer.erris never set; its guards are ineffective
CmdDialerhas anerrfield and checks it inRead,Write, andDial, but nothing ever assigns toerr:type CmdDialer struct { Cmd *exec.Cmd Stdin io.WriteCloser Stdout io.ReadCloser err error }As a result, these checks never trigger and only add complexity.
Either:
- Remove
errand the checks, or- Set
errwhen the command fails to start or the pipes fail (and bail accordingly).Given the current usage in
Start(), removingerrand simplifying the methods is likely easiest.
102-115: Hardcodedconfig_linuxbreaks portability to macOS/Windows
configDiris always built as:configDir := filepath.Join(c.jdtlsPath, "config_linux")This will fail on non‑Linux systems where the JDT.LS distribution uses
config_mac,config_win, etc.Consider selecting the suffix based on
runtime.GOOS:+ import "runtime" @@ - configDir := filepath.Join(c.jdtlsPath, "config_linux") + var configSuffix string + switch runtime.GOOS { + case "darwin": + configSuffix = "config_mac" + case "windows": + configSuffix = "config_win" + default: + configSuffix = "config_linux" + } + configDir := filepath.Join(c.jdtlsPath, configSuffix)This keeps the client usable across platforms.
172-197: Hardcoded bundle path reduces flexibility across environmentsInitialization options use a fixed bundle path:
"bundles": []string{ "/jdtls/plugins/java-analyzer-bundle.core-1.0.0-SNAPSHOT.jar", },This assumes a specific container layout and version. Running the client outside that environment or after bumping the bundle version will break initialization.
Consider making bundle paths configurable (e.g., constructor parameter, environment variable, or derived from
jdtlsPath), and defaulting to the current path when none is supplied.
305-359: GuardShutdownfor partially-initialized clients to avoid panics
Shutdown()assumesc.ctx,c.conn, andc.cmdare non‑nil:
context.WithTimeout(c.ctx, ...)will panic ifc.ctxis nil.c.conn.Call/Notify/Closewill panic ifc.connis nil.c.cmd.Wait()andc.cmd.Process.Kill()will panic ifc.cmdis nil.If
Start()fails early (e.g., JDT.LS missing, Dial fails), callers thatdefer client.Close()will hitShutdown()on a partially constructed client and crash.Add a fast‑path for this case, and clean up what you can:
func (c *JDTLSClient) Shutdown() error { c.logger.Info("Shutting down JDT.LS server...") + // Fast-path: connection never established + if c.conn == nil { + if c.cancel != nil { + c.cancel() + } + if c.dialer != nil { + _ = c.dialer.Close() + } + if c.cmd != nil && c.cmd.Process != nil { + if err := c.cmd.Process.Kill(); err != nil { + c.logger.Debugf("Process kill failed: %v", err) + } + if err := c.cmd.Wait(); err != nil { + c.logger.Debugf("Process wait failed: %v", err) + } + } + return nil + } + // Create a context with timeout for shutdown requests shutdownCtx, shutdownCancel := context.WithTimeout(c.ctx, 3*time.Second)This matches the expected lifecycle when
Start()fails and avoids panics.
🧹 Nitpick comments (12)
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/inheritance/CustomException.java (1)
1-21: LGTM! Standard exception implementation for testing.The custom exception class correctly extends
Exceptionand follows standard Java patterns with appropriate constructors andserialVersionUID. The implementation is well-suited for testing inheritance scenarios.Optionally, consider adding the
CustomException(Throwable cause)constructor for completeness:public CustomException(String message) { super(message); } + + public CustomException(Throwable cause) { + super(cause); + } public CustomException(String message, Throwable cause) { super(message, cause); }java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/EnumExample.java (1)
3-35: Enum shape and lookup helper look solid; consider documenting null behaviorThe enum constants, payload fields, and simple getters are all straightforward and correct for a test fixture. The
fromCode(int code)helper is fine for this use, but it silently returnsnullfor unknown codes. If this helper is used beyond very controlled tests, you might want to either:
- Document in Javadoc that it can return
null, or- Consider returning
Optional<EnumExample>or throwing anIllegalArgumentExceptionfor unknown codes.Not mandatory for a test project, but it can make call‑site behavior clearer.
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/Calculator.java (1)
5-42: Calculator fixture is well‑shaped for analyzer tests; minor optional tweakThe mix of return types, the explicit divide‑by‑zero guard, the custom enum return, and the deprecated
summethod give good coverage for the analyzer. The implementations are correct and easy to follow.If you want to avoid duplication, you could have
sum(int a, int b)delegate toadd(int a, int b)so the arithmetic behavior can’t accidentally diverge:public int sum(int a, int b) { return add(a, b); }Purely optional, especially given this is a test fixture.
java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/OrderManagementAppInitializer.java (1)
17-20: Review potentially redundant configuration: setConfigLocation and scan overlap.Lines 18 and 20 both configure the application context:
- Line 18:
setConfigLocation("io.konveyor.demo.ordermanagement.config")registers configuration classes from the config package- Line 20:
scan("io.konveyor.demo.ordermanagement")scans the parent package (which includes the config subpackage)Since
scan()already covers the config package,setConfigLocation()may be redundant. The typical pattern is to use component scanning OR explicit configuration registration, not both with overlapping packages, as this could lead to duplicate bean registrations or configuration processing.Consider simplifying to use only component scanning:
public void onStartup(ServletContext container) throws ServletException { AnnotationConfigWebApplicationContext context = new AnnotationConfigWebApplicationContext(); - context.setConfigLocation("io.konveyor.demo.ordermanagement.config"); context.scan("io.konveyor.demo.ordermanagement"); container.addListener(new ContextLoaderListener(context));java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/service/ICustomerService.java (1)
10-10: Fix missing space between return type and method name.Line 10 is missing a space between the return type and method name:
Page<Customer>findAllshould bePage<Customer> findAllfor consistency with line 8.- public Page<Customer>findAll(Pageable pageable); + public Page<Customer> findAll(Pageable pageable);java-analyzer-bundle.test/projects/customers-tomcat-legacy/Makefile (3)
9-9: Declare thealltarget as.PHONY.The
alltarget should be declared.PHONYto ensure it always runs regardless of filesystem state.Apply this diff:
+.PHONY: all all: build
28-28: Correct the comment to reflect Docker container usage.The comment mentions "vagrant VM" but the target runs a Docker container.
Apply this diff:
-run: ## Run a vagrant VM using this image +run: ## Run a Docker container using this image
32-36: Add error suppression to theremovetarget.The
removetarget will fail if the images don't exist. Add error suppression to make the target idempotent.Apply this diff:
.PHONY: remove remove: ## Removes all built images $(info Removing all built images...) - docker rmi $(REMOTE_IMAGE_NAME) - docker rmi $(LOCAL_IMAGE_NAME) + -docker rmi $(REMOTE_IMAGE_NAME) 2>/dev/null || true + -docker rmi $(LOCAL_IMAGE_NAME) 2>/dev/null || truejava-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/ConstructorCallSymbolProvider.java (1)
35-58: Clarify or gate the JDTUtils element lookup logging
JDTUtils.findElementAtSelection(...)is currently only used to log"je" + element. For large workspaces or many matches this extra resolution and logging may add noticeable overhead while not affecting behavior.If this is purely diagnostic:
- Consider guarding it behind a more verbose log level or a feature flag, or
- Remove the lookup entirely until you actually consume
element.This keeps the provider focused on symbol construction and reduces unnecessary work during searches.
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/PackageDeclarationSymbolProvider.java (1)
72-75: AvoidprintStackTrace()in server code; rely on structured loggingCatching
Exceptionand callinge.printStackTrace()will dump to stderr, which is noisy and hard to aggregate in a language server environment. You’re already logging vialogInfo.Consider logging details instead of printing the stack trace:
- } catch (Exception e) { - logInfo("Error processing package declaration: " + e.toString()); - e.printStackTrace(); - } + } catch (Exception e) { + logInfo("Error processing package declaration: " + e.toString()); + // Optionally include stack trace in the log if needed + // logInfo("Stack trace: " + Arrays.toString(e.getStackTrace())); + }This keeps server output predictable and easier to consume.
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java (1)
51-74: Constructor should either useprocessedQueryor drop it to avoid confusionYou compute:
String processedQuery = query.replaceAll("\\([^)]*\\)", ""); ... this.query = this.queryMethodName.replaceAll("(?<!\\.)\\*", ".*");but
processedQueryis never used. Given the comment about stripping parameter types from the query, this can be misleading to future readers.Either:
- Use
processedQueryas the basis forqueryMethodName/this.query, or- Remove the
processedQuerylocal (and adjust the comment) so it doesn’t suggest unused behavior.java-analyzer-bundle.test/integration/client/jdtls_client.go (1)
141-163: Handle Dial failure by cleaning up the started JDT.LS processIf
jsonrpc2.Dialfails afterc.cmd.Start()succeeds:c.conn, err = jsonrpc2.Dial(c.ctx, c.dialer, binder) if err != nil { return fmt.Errorf("failed to establish JSON-RPC connection: %w", err) }the JDT.LS process continues running, and the pipes remain open. Callers will see an error from
Start()but the child process is leaked.It would be safer to:
- Close the dialer’s stdin/stdout,
- Kill the process (if started), and
- Wait for it to exit,
before returning the error.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
java-analyzer-bundle.test/integration/go.sumis excluded by!**/*.sum
📒 Files selected for processing (72)
.github/workflows/integration-tests.yml(1 hunks).gitignore(1 hunks)Dockerfile(2 hunks)Dockerfile.test(1 hunks)Makefile(1 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java(6 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SymbolInformationTypeRequestor.java(2 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/ConstructorCallSymbolProvider.java(3 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java(12 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/EnumConstantSymbolProvider.java(1 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/PackageDeclarationSymbolProvider.java(1 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/ReturnTypeSymbolProvider.java(2 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/SymbolProviderResolver.java(1 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/VariableDeclarationSymbolProvider.java(1 hunks)java-analyzer-bundle.test/README.md(1 hunks)java-analyzer-bundle.test/docs/README.md(1 hunks)java-analyzer-bundle.test/docs/integration-tests.md(1 hunks)java-analyzer-bundle.test/docs/query-reference.md(1 hunks)java-analyzer-bundle.test/docs/quick-reference.md(1 hunks)java-analyzer-bundle.test/docs/test-projects.md(1 hunks)java-analyzer-bundle.test/integration/client/jdtls_client.go(1 hunks)java-analyzer-bundle.test/integration/go.mod(1 hunks)java-analyzer-bundle.test/integration/integration_test.go(1 hunks)java-analyzer-bundle.test/integration/run_local.sh(1 hunks)java-analyzer-bundle.test/integration/test_helpers.go(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/.gitignore(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/Dockerfile(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/Makefile(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/pom.xml(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/OrderManagementAppInitializer.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/config/PersistenceConfig.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/config/WebConfig.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/controller/CustomerController.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/exception/ResourceNotFoundException.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/exception/handler/ExceptionHandlingController.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/model/Customer.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/repository/CustomerRepository.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/service/CustomerService.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/service/ICustomerService.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/resources/import.sql(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/resources/persistence.properties(1 hunks)java-analyzer-bundle.test/projects/test-project/.classpath(1 hunks)java-analyzer-bundle.test/projects/test-project/.project(1 hunks)java-analyzer-bundle.test/projects/test-project/.settings/org.eclipse.core.resources.prefs(1 hunks)java-analyzer-bundle.test/projects/test-project/.settings/org.eclipse.jdt.apt.core.prefs(1 hunks)java-analyzer-bundle.test/projects/test-project/.settings/org.eclipse.jdt.core.prefs(1 hunks)java-analyzer-bundle.test/projects/test-project/.settings/org.eclipse.m2e.core.prefs(1 hunks)java-analyzer-bundle.test/projects/test-project/pom.xml(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/Calculator.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/EnumExample.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/GenericMethodsDemo.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/MethodSignatureDemo.java(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/projects/test-project/src/main/java/io/konveyor/demo/annotations/CustomAnnotation.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/annotations/DeprecatedApi.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/config/DataSourceConfig.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/config/MySQLDataSourceConfig.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/entity/Product.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/inheritance/BaseService.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/inheritance/CustomException.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/inheritance/DataService.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/jms/MessageProcessor.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/jms/TopicMessageProcessor.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/persistence/AnotherMixedService.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/persistence/JdbcOnlyService.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/persistence/PureJpaService.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/persistence/ServiceWithEntityManager.java(1 hunks)java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/RuleEntryParamsTest.java(1 hunks)java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandlerTest.java(2 hunks)java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/query/AnnotationQueryTest.java(1 hunks)pom.xml(4 hunks)
✅ Files skipped from review due to trivial changes (2)
- java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/resources/import.sql
- java-analyzer-bundle.test/projects/test-project/.project
🚧 Files skipped from review as they are similar to previous changes (33)
- .gitignore
- java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/resources/persistence.properties
- java-analyzer-bundle.test/integration/test_helpers.go
- java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/model/Customer.java
- Dockerfile.test
- java-analyzer-bundle.test/projects/customers-tomcat-legacy/Dockerfile
- java-analyzer-bundle.test/projects/test-project/.settings/org.eclipse.jdt.apt.core.prefs
- java-analyzer-bundle.test/projects/test-project/.settings/org.eclipse.jdt.core.prefs
- java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/inheritance/DataService.java
- .github/workflows/integration-tests.yml
- java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/annotations/CustomAnnotation.java
- java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/jms/MessageProcessor.java
- Dockerfile
- java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/persistence/JdbcOnlyService.java
- java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/EnumConstantSymbolProvider.java
- java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/VariableDeclarationSymbolProvider.java
- java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/persistence/ServiceWithEntityManager.java
- java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/config/DataSourceConfig.java
- java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/annotations/DeprecatedApi.java
- java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/RuleEntryParamsTest.java
- java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/config/PersistenceConfig.java
- java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/SymbolProviderResolver.java
- java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/ServletExample.java
- java-analyzer-bundle.test/projects/test-project/.settings/org.eclipse.core.resources.prefs
- java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/entity/Product.java
- java-analyzer-bundle.test/integration/integration_test.go
- java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/MethodSignatureDemo.java
- java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/GenericMethodsDemo.java
- java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/config/MySQLDataSourceConfig.java
- java-analyzer-bundle.test/projects/test-project/.settings/org.eclipse.m2e.core.prefs
- java-analyzer-bundle.test/integration/go.mod
- java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/service/CustomerService.java
- java-analyzer-bundle.test/projects/test-project/.classpath
🧰 Additional context used
🧬 Code graph analysis (5)
java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/controller/CustomerController.java (1)
java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/exception/ResourceNotFoundException.java (1)
ResourceNotFoundException(19-27)
java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/exception/handler/ExceptionHandlingController.java (1)
java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/exception/ResourceNotFoundException.java (1)
ResourceNotFoundException(19-27)
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/jms/TopicMessageProcessor.java (1)
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/jms/MessageProcessor.java (1)
MessageDriven(12-23)
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandlerTest.java (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java (1)
SampleDelegateCommandHandler(43-427)
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/SampleApplication.java (1)
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/inheritance/BaseService.java (1)
BaseService(9-39)
🪛 checkmake (0.2.2)
Makefile
[warning] 25-25: Target body for "help" exceeds allowed length of 5 (24).
(maxbodylength)
[warning] 115-115: Target body for "clean" exceeds allowed length of 5 (6).
(maxbodylength)
java-analyzer-bundle.test/projects/customers-tomcat-legacy/Makefile
[warning] 38-38: Missing required phony target "all"
(minphony)
[warning] 38-38: Missing required phony target "test"
(minphony)
[warning] 9-9: Target "all" should be declared PHONY.
(phonydeclared)
🪛 LanguageTool
java-analyzer-bundle.test/docs/query-reference.md
[grammar] ~531-~531: Ensure spelling is correct
Context: ...**: ~5 seconds Per-Query Time: ~100-500ms depending on result count **Result Coun...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
java-analyzer-bundle.test/docs/README.md
[style] ~73-~73: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...uick-reference.md#running-tests) ### I want to understand what's tested → [Quick Refer...
(REP_WANT_TO_VB)
[style] ~77-~77: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...erence.md#tested-search-queries) ### I want to add a new test → [Integration Tests Gui...
(REP_WANT_TO_VB)
[style] ~81-~81: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...erence.md#tested-search-queries) ### I want to understand the architecture → [Integrat...
(REP_WANT_TO_VB)
[style] ~85-~85: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...integration-tests.md#components) ### I want to understand test projects → [Test Projec...
(REP_WANT_TO_VB)
[style] ~89-~89: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...est-projects.md#coverage-matrix) ### I want to add Java test code → [Test Projects - A...
(REP_WANT_TO_VB)
[style] ~95-~95: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...ration-tests.md#troubleshooting) ### I want to understand migration testing → [Query R...
(REP_WANT_TO_VB)
java-analyzer-bundle.test/docs/integration-tests.md
[grammar] ~571-~571: Use a hyphen to join words.
Context: ...: JDT.LS needs time to index projects (5 second wait in Initialize()) 2. **Build ...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
java-analyzer-bundle.test/docs/test-projects.md
17-17: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
62-62: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
java-analyzer-bundle.test/README.md
144-144: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
java-analyzer-bundle.test/docs/README.md
213-213: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
java-analyzer-bundle.test/docs/integration-tests.md
17-17: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
469-469: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
java-analyzer-bundle.test/docs/quick-reference.md
7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
44-44: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
53-53: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
205-205: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 OSV Scanner (2.2.4)
java-analyzer-bundle.test/projects/customers-tomcat-legacy/pom.xml
[CRITICAL] 1-1: ch.qos.logback:logback-classic 1.1.7: QOS.ch Logback vulnerable to Deserialization of Untrusted Data
[CRITICAL] 1-1: ch.qos.logback:logback-classic 1.1.7: logback serialization vulnerability
[CRITICAL] 1-1: org.postgresql:postgresql 42.2.23: org.postgresql:postgresql vulnerable to SQL Injection via line comment generation
[CRITICAL] 1-1: org.postgresql:postgresql 42.2.23: TemporaryFolder on unix-like systems does not limit access to created files
[CRITICAL] 1-1: org.postgresql:postgresql 42.2.23: pgjdbc Arbitrary File Write Vulnerability
[CRITICAL] 1-1: org.postgresql:postgresql 42.2.23: Path traversal in org.postgresql:postgresql
[CRITICAL] 1-1: org.postgresql:postgresql 42.2.23: PostgreSQL JDBC Driver SQL Injection in ResultSet.refreshRow() with malicious column names
[CRITICAL] 1-1: org.postgresql:postgresql 42.2.23: pgjdbc Does Not Check Class Instantiation when providing Plugin Classes
[CRITICAL] 1-1: org.springframework:spring-web 5.3.7: Spring Framework DoS via conditional HTTP request
[CRITICAL] 1-1: org.springframework:spring-web 5.3.7: Spring Framework URL Parsing with Host Validation
[CRITICAL] 1-1: org.springframework:spring-web 5.3.7: Spring Framework DataBinder Case Sensitive Match Exception
[CRITICAL] 1-1: org.springframework:spring-web 5.3.7: Pivotal Spring Framework contains unsafe Java deserialization methods
[CRITICAL] 1-1: org.springframework:spring-web 5.3.7: Spring Web vulnerable to Open Redirect or Server Side Request Forgery
[CRITICAL] 1-1: org.springframework:spring-web 5.3.7: Spring Framework URL Parsing with Host Validation Vulnerability
[CRITICAL] 1-1: org.springframework:spring-webmvc 5.3.7: Remote Code Execution in Spring Framework
[CRITICAL] 1-1: org.springframework:spring-webmvc 5.3.7: Spring Framework is vulnerable to security bypass via mvcRequestMatcher pattern mismatch
[CRITICAL] 1-1: org.springframework:spring-webmvc 5.3.7: Path traversal vulnerability in functional web frameworks
[CRITICAL] 1-1: org.springframework:spring-webmvc 5.3.7: Spring Framework Path Traversal vulnerability
[CRITICAL] 1-1: org.springframework:spring-webmvc 5.3.7: Spring Framework MVC Applications Path Traversal Vulnerability
[CRITICAL] 1-1: org.springframework:spring-webmvc 5.3.7: Spring MVC controller vulnerable to a DoS attack
⏰ 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 (27)
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/persistence/PureJpaService.java (3)
1-5: LGTM! Imports are appropriate for the JPA test fixture.The package structure and imports are correctly organized for testing JPA variable binding.
7-15: Excellent documentation for the test fixture.The Javadoc clearly explains the expected behavior for this test case, making it easy to understand the verification criteria.
16-19: LGTM! Standard JPA field injection pattern.The class structure and EntityManager field declaration follow proper JPA conventions with the @PersistenceContext annotation.
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/inheritance/BaseService.java (1)
9-33: Class structure looks good for the intended inheritance/implements tests.Abstract base + lifecycle methods +
Serializable/Comparablewiring is straightforward and appropriate for the test-project scenarios.java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/SampleApplication.java (1)
22-97: Overall structure matches the stated “search pattern” test goals.Use of annotations, inheritance, constructors, collections, and
java.ioAPIs is varied and should serve JDT.LS search/location testing well.java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/jms/TopicMessageProcessor.java (3)
3-6: LGTM! Clean and minimal imports.The imports are appropriate for a JMS Topic MessageDriven bean and all are utilized in the implementation.
18-21: LGTM! Clear implementation following established test pattern.The
onMessageimplementation correctly logs the topic message with an appropriate prefix to differentiate it from queue-based messaging during test execution.
12-15: ****The omission of
acknowledgeModeis intentional and documented. TopicMessageProcessor's JavaDoc explicitly states the class tests "different attribute values" for Topic-based messaging, and the absence ofacknowledgeModeis the deliberate variation being tested. This differs from MessageProcessor, which tests Queue configuration with the full set of properties. No changes are required.Likely an incorrect or invalid review comment.
java-analyzer-bundle.test/projects/customers-tomcat-legacy/.gitignore (1)
1-35: LGTM!Comprehensive .gitignore with appropriate patterns for multiple IDEs, build tools, and operating systems. The patterns are standard and well-organized.
java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/config/WebConfig.java (1)
7-9: LGTM!Excluding
DataSourceAutoConfigurationis the correct approach when using a customPersistenceConfigwith manual DataSource configuration, preventing conflicts between Spring Boot auto-configuration and custom bean definitions.pom.xml (3)
26-26: LGTM!Updating the coverage filter from
org.eclipse.jdt.ls.*toio.konveyor.tackle.*correctly focuses code coverage on this project's packages rather than the underlying JDT.LS infrastructure.
73-73: LGTM!Setting
useUIHarnesstofalseis appropriate for headless test execution in CI/CD environments and aligns with the automated integration test strategy introduced in this PR.
148-166: LGTM!The Jacoco configuration correctly adds
prepare-agentfor coverage instrumentation andreportexecution bound to the verify phase. This is the standard setup for Maven-based code coverage reporting and aligns with the comprehensive test suite introduced in this PR.java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/repository/CustomerRepository.java (1)
6-6: The original review comment is based on incorrect assumptions about the Spring Data version in use.The project uses Spring Data 2021.0.1 (Spring Data 2.x), released May 14, 2021. In Spring Data 2.x,
PagingAndSortingRepositorycorrectly extendsCrudRepository, making the interface declaration inCustomerRepositoryfully compatible and correct. The breaking change referenced in the review—wherePagingAndSortingRepositoryno longer extendsCrudRepository—only applies to Spring Data 3.0 and later (released November 18, 2022, after this project's version).The code is valid as written and requires no changes.
Likely an incorrect or invalid review comment.
java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/exception/ResourceNotFoundException.java (1)
19-27: LGTM!The custom exception implementation follows best practices: extends
RuntimeException, declaresserialVersionUID, and properly delegates to the superclass constructor.java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/query/AnnotationQueryTest.java (2)
1-232: Excellent test coverage for AnnotationQuery.The test suite comprehensively covers constructor behavior, pattern matching (exact, regex, wildcard), fromMap factory method, elements handling, and location-based behavior across all location types.
180-191: Note: Non-defensive copy behavior is intentionally tested.The test explicitly verifies that
AnnotationQuerydoes not make a defensive copy of the elements map, meaning external mutations affect internal state. While this is intentional (per the comment), consider whether immutability would be safer for production use.Makefile (1)
1-141: Well-structured CI/CD Makefile.The Makefile provides a clean phase-based structure that mirrors the GitHub Actions workflow, with proper container runtime detection and SELinux-aware volume mounting for Podman.
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandlerTest.java (1)
30-249: Comprehensive test expansion for command handler.The test suite now covers a wide range of location types and analysis modes, providing thorough validation of the command handler's functionality across inheritance, method calls, constructors, annotations, imports, and more.
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/ReturnTypeSymbolProvider.java (1)
23-35: Good enhancement for type matching.The addition of readable type conversion improves matching accuracy by allowing queries to match against both JVM signatures and human-readable type names (e.g., "I" and "int").
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java (2)
122-156: Parameter matching logic looks solid and type‑erasure awareThe new
parametersMatch(IMethodBinding)implementation correctly:
- Treats “no parameters in query” as “match any parameter list”.
- Compares counts and then each parameter type using
getTypeNameplus a wildcard‑aware regex.Given you normalize generics via
getErasure()and handle arrays/primitives ingetTypeName, this should work well for common method/constructor overloads.LGTM as a foundation for parameter‑sensitive symbol filtering.
161-175: Type-name normalization for primitives/arrays/generics is appropriate
getTypeName(ITypeBinding)’s handling of:
- primitives (
int,boolean),- arrays via recursive
elementType + "[]", and- generic erasure (
type.getErasure()thengetQualifiedName())is a good match for comparing against user‑supplied parameter type patterns (especially when combined with wildcard support).
No changes needed here.
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java (3)
163-207: SearchPattern extensions for erasure, packages, and enums look correctThe updates in
getPatternSingleQuery:
- Keep package searches (location 11) on
R_PATTERN_MATCH, which aligns with JDT’s more flexible package matching.- Add
R_ERASURE_MATCHfor:
- return/variable declaration type references (7, 9),
- constructor calls (3),
- field‑declaration type references (12),
ensuring parameterized types likeList<String>are found when searching forjava.util.List.- Map
enum_constant(6) toFIELDreferences, which is how enum constants are represented in JDT.Combined with the existing switch and the location‑0 aggregation, this looks consistent and should improve matching of generics and enums without regressions.
248-321: Included-path handling and import-scan unit collection are consistentThe additions:
- Use
IPackageFragmentRoot.K_SOURCEto guardfragment.getCompilationUnits()when pre‑fillingunits.- Reuse the same
unitslist later for the on‑demand import scan (location 8), further filtered byscope.encloses(unit).This keeps binary packages out of the AST import scan while ensuring included fragments contribute all their compilation units. The logic remains bounded by the existing
includedPathsandscope, so it should be safe performance‑wise.LGTM.
342-355: Passingpatterninto SymbolInformationTypeRequestor aligns with new matching behaviorThe updated constructor call:
SymbolInformationTypeRequestor requestor = new SymbolInformationTypeRequestor(symbols, 0, monitor, location, query, annotationQuery, pattern);properly propagates the final
SearchPatterninto the requestor, so downstream symbol providers (including your new ones) can leverage the more accurate pattern (erasure, package, enum handling, etc.).No issues here.
java-analyzer-bundle.test/integration/client/jdtls_client.go (2)
167-216: Initialization flow and result handling look correct for test usageThe
Initialize()method:
- Builds a sensible
InitializeParamsstruct (workspace URI, basic workspace/textDocument capabilities).- Sends
initialize, waits for theInitializeResult, then sendsinitialized.- Sleeps briefly to give JDT.LS time to process projects.
For a test‑only client this is a reasonable balance between simplicity and correctness, assuming the hardcoded bundle path and workspace expectations are satisfied.
256-302: Symbol search command wiring and result unmarshalling look good
SearchSymbolsWithAnnotation:
- Builds the expected
io.konveyor.tackle.ruleEntryarguments map (project, query, location as string, analysisMode, optional includedPaths and annotationQuery).- Executes the command and defensively handles
nilresults.- Uses JSON round‑trip to convert
anyto[]protocol.SymbolInformation, which is a pragmatic approach for loosely typed responses.This should integrate cleanly with
SampleDelegateCommandHandler.search(...).
|
|
||
|
|
||
| public SymbolInformationTypeRequestor(List<SymbolInformation> symbols, int maxResults, IProgressMonitor monitor, int symbolKind, String query, AnnotationQuery annotationQuery) { | ||
| public SymbolInformationTypeRequestor(List<SymbolInformation> symbols, int maxResults, IProgressMonitor monitor, int symbolKind, String query, AnnotationQuery annotationQuery, SearchPattern searchPattern) { |
There was a problem hiding this comment.
🧩 Analysis chain
Verify unused SearchPattern parameter.
The searchPattern parameter is added to the constructor but never stored or used. This suggests either incomplete implementation or unnecessary parameter passing.
Run the following script to check if this parameter will be used in the future or can be removed:
🏁 Script executed:
#!/bin/bash
# Check how SymbolInformationTypeRequestor is instantiated and if searchPattern is used elsewhere
# Find all instantiations of SymbolInformationTypeRequestor
rg -n "new SymbolInformationTypeRequestor" --type java -A 5
# Check if SearchPattern is used in symbol providers
rg -n "SearchPattern" --type java -g "**/*SymbolProvider.java" -C 3Length of output: 1014
🏁 Script executed:
#!/bin/bash
# Inspect the SymbolInformationTypeRequestor class to verify if searchPattern is stored/used
ast-grep --pattern $'class SymbolInformationTypeRequestor {
$$$
}'Length of output: 0
🏁 Script executed:
#!/bin/bash
# More focused search: get the full class definition to check for searchPattern field
rg -nA 50 "class SymbolInformationTypeRequestor" --type javaLength of output: 7623
🏁 Script executed:
#!/bin/bash
# Check full SymbolInformationTypeRequestor class for any reference to searchPattern
rg -n "searchPattern" --type java java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SymbolInformationTypeRequestor.javaLength of output: 213
Remove unused searchPattern parameter.
The searchPattern parameter added to the constructor at line 39 is never stored or used. No field is created for it, and it's not referenced anywhere in the class. This should be removed from the constructor signature and from the caller in SampleDelegateCommandHandler.java (line 354).
🤖 Prompt for AI Agents
In
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SymbolInformationTypeRequestor.java
around line 39, the constructor includes an unused parameter searchPattern;
remove this parameter from the constructor signature and any constructor calls.
Also update the caller in SampleDelegateCommandHandler.java (around line 354) to
stop passing the searchPattern argument when instantiating
SymbolInformationTypeRequestor so constructor arguments match; ensure
imports/overloads remain consistent and compile.
| <dependencies> | ||
| <!-- Common dependencies for testing various patterns --> | ||
| <dependency> | ||
| <groupId>jakarta.servlet</groupId> | ||
| <artifactId>jakarta.servlet-api</artifactId> | ||
| <version>5.0.0</version> | ||
| <scope>provided</scope> | ||
| </dependency> | ||
|
|
||
| <!-- JPA/Persistence API for entity and persistence testing --> | ||
| <dependency> | ||
| <groupId>javax.persistence</groupId> | ||
| <artifactId>javax.persistence-api</artifactId> | ||
| <version>2.2</version> | ||
| <scope>provided</scope> | ||
| </dependency> | ||
|
|
||
| <!-- EJB API for JMS and EJB annotations testing --> | ||
| <dependency> | ||
| <groupId>javax.ejb</groupId> | ||
| <artifactId>javax.ejb-api</artifactId> | ||
| <version>3.2.2</version> | ||
| <scope>provided</scope> | ||
| </dependency> | ||
|
|
||
| <!-- JMS API for message-driven bean testing --> | ||
| <dependency> | ||
| <groupId>javax.jms</groupId> | ||
| <artifactId>javax.jms-api</artifactId> | ||
| <version>2.0.1</version> | ||
| <scope>provided</scope> | ||
| </dependency> | ||
|
|
||
| <!-- Annotation API for @DataSourceDefinition --> | ||
| <dependency> | ||
| <groupId>javax.annotation</groupId> | ||
| <artifactId>javax.annotation-api</artifactId> | ||
| <version>1.3.2</version> | ||
| <scope>provided</scope> | ||
| </dependency> |
There was a problem hiding this comment.
Review namespace consistency across Jakarta and Java EE dependencies.
The POM mixes jakarta.servlet-api 5.0.0 (Jakarta EE 9+ namespace) with javax.* APIs (Java EE 8 namespace). This inconsistency can lead to classpath conflicts or unexpected behavior:
- Jakarta EE 9+:
jakarta.servlet-api 5.0.0(line 24-26) - Java EE 8:
javax.persistence-api 2.2(line 32),javax.ejb-api 3.2.2(line 40),javax.jms-api 2.0.1(line 48),javax.annotation-api 1.3.2(line 56)
For consistency, either:
- Use Jakarta EE 9+ versions with
jakarta.*namespace for all APIs (e.g.,jakarta.persistence-api 3.0.0), or - Use Java EE 8 versions with
javax.*namespace for all APIs (e.g.,javax.servlet-api 4.0.1)
If the intent is to test cross-version compatibility, please document this explicitly in the POM description or a comment.
| public void performJpaOperations() { | ||
| // Uses only JPA EntityManager, no JDBC | ||
| entityManager.persist(new Object()); | ||
|
|
||
| Query query = entityManager.createQuery("SELECT u FROM User u"); | ||
| query.getResultList(); | ||
| } |
There was a problem hiding this comment.
Replace new Object() with a proper JPA entity class.
Line 23 attempts to persist a plain Object instance, which will throw IllegalArgumentException at runtime since JPA requires entities to be annotated with @Entity. While this is a test fixture for static analysis, using proper JPA patterns ensures the code is valid and maintainable.
Consider one of these approaches:
Option 1: Create a simple entity class
Add a new entity class in the same package:
package io.konveyor.demo.persistence;
import javax.persistence.Entity;
import javax.persistence.Id;
@Entity
public class TestEntity {
@Id
private Long id;
public TestEntity() {}
}Then update line 23:
- entityManager.persist(new Object());
+ entityManager.persist(new TestEntity());Option 2: Use null if execution isn't required
If the test only needs to be parsed but not executed:
- entityManager.persist(new Object());
+ // Simplified for static analysis - would require proper @Entity in production
+ entityManager.persist(null);🤖 Prompt for AI Agents
In
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/persistence/PureJpaService.java
around lines 21 to 27, line 23 is persisting a plain new Object() which will
throw at runtime because JPA requires an @Entity; replace that with a real JPA
entity instance (e.g., create a TestEntity class in the same package annotated
with @Entity and an @Id field, instantiate TestEntity and persist it), or if
execution is not required for the test, change the persist call to persist(null)
or remove the persist entirely so only valid JPA entities are used.
| $(CONTAINER_RUNTIME) run --rm \ | ||
| -v $(REPO_ROOT)/java-analyzer-bundle.test:/tests$(VOLUME_FLAGS) \ |
There was a problem hiding this comment.
🧩 Analysis chain
Verify volume mount path behavior across different invocation contexts.
The volume mount uses REPO_ROOT := $(shell pwd) which resolves at parse time. Ensure this works correctly if the Makefile is invoked from a different directory (e.g., make -C subdir target).
Run the following script to test the Makefile from different contexts:
🏁 Script executed:
#!/bin/bash
# Test Makefile invocation from different directories
# Test 1: From repo root
echo "Test 1: From repo root"
pwd
make -n run-integration-tests 2>&1 | grep "docker run" || echo "Failed"
# Test 2: From subdirectory
echo -e "\nTest 2: From subdirectory (if applicable)"
if [ -d "java-analyzer-bundle.test" ]; then
cd java-analyzer-bundle.test
pwd
make -C .. -n run-integration-tests 2>&1 | grep "docker run" || echo "Failed"
fiLength of output: 143
I need to investigate further. The "Failed" results indicate we need to see the actual Makefile output.
🏁 Script executed:
#!/bin/bash
# Check if Makefile exists and inspect the target and REPO_ROOT
echo "=== Check Makefile exists ==="
[ -f Makefile ] && echo "Makefile found at repo root" || echo "Makefile not found"
echo -e "\n=== Inspect run-integration-tests target and lines 103-104 ==="
if [ -f Makefile ]; then
sed -n '95,115p' Makefile
fi
echo -e "\n=== Test 1: make -n run-integration-tests (raw output, no grep) ==="
make -n run-integration-tests 2>&1 | head -20
echo -e "\n=== Test 2: Check REPO_ROOT from repo root ==="
make -n -e 'run-integration-tests' 2>&1 | grep -i "REPO_ROOT\|repo_root" | head -5
echo -e "\n=== Test 3: From subdirectory - check REPO_ROOT ==="
if [ -d "java-analyzer-bundle.test" ]; then
cd java-analyzer-bundle.test
echo "Current dir: $(pwd)"
make -C .. -n run-integration-tests 2>&1 | grep -i "REPO_ROOT\|repo_root" | head -5
fiLength of output: 1100
I need to inspect the Makefile to find where REPO_ROOT is defined.
🏁 Script executed:
#!/bin/bash
# Find REPO_ROOT definition and understand its context
echo "=== Search for REPO_ROOT definition in Makefile ==="
grep -n "REPO_ROOT" Makefile | head -20
echo -e "\n=== Show first 50 lines of Makefile to see variable definitions ==="
head -50 Makefile
echo -e "\n=== Show lines around first REPO_ROOT reference ==="
grep -n "REPO_ROOT" Makefile | head -1 | cut -d: -f1 | xargs -I {} sed -n '{}p;$(({}+5))p;$(({}+10))p' MakefileLength of output: 2208
Replace $(shell pwd) with path-relative variable for robust Makefile portability.
The current definition at line 21 (REPO_ROOT := $(shell pwd)) is not portable across different invocation contexts. While it works when invoked from the repo root or with -C flag, it relies on the working directory at parse time and can fail if the Makefile is sourced or invoked from unexpected locations.
Replace with:
REPO_ROOT := $(dir $(abspath $(lastword $(MAKEFILE_LIST))))This resolves to the Makefile's actual directory regardless of where make is invoked from, ensuring correct volume mount paths in line 104 across all contexts.
🤖 Prompt for AI Agents
In Makefile around lines 103–104, the REPO_ROOT usage for the volume mount is
fragile because REPO_ROOT is currently set at line 21 using $(shell pwd);
replace that assignment so REPO_ROOT is computed from the Makefile location
itself by using a construct that combines dir, abspath and lastword on
MAKEFILE_LIST (i.e., compute the directory of the current Makefile rather than
calling pwd) so the volume mount path is correct regardless of where make is
invoked.
874b097 to
c5cc10c
Compare
Signed-off-by: Shawn Hurley <[email protected]>
Signed-off-by: Shawn Hurley <[email protected]>
Signed-off-by: Shawn Hurley <[email protected]>
Signed-off-by: Shawn Hurley <[email protected]>
Signed-off-by: Shawn Hurley <[email protected]>
Signed-off-by: Shawn Hurley <[email protected]>
Signed-off-by: Shawn Hurley <[email protected]>
Signed-off-by: Shawn Hurley <[email protected]>
Signed-off-by: Shawn Hurley <[email protected]>
Signed-off-by: Shawn Hurley <[email protected]>
Signed-off-by: Shawn Hurley <[email protected]>
Signed-off-by: Shawn Hurley <[email protected]>
Signed-off-by: Shawn Hurley <[email protected]>
shawn-hurley
force-pushed
the
testing/adding-bundle-specific-tests
branch
from
c5cc10c to
62d4fd2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (21)
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/RuleEntryParamsTest.java (1)
238-249: Test expects incorrect exception type—will fail at runtime.This is the same critical issue previously identified: when the "location" key is missing,
obj.get("location")returnsnull, andInteger.parseInt((String) null)throwsNullPointerException, notNumberFormatException.The test must expect
NullPointerException, or better yet, theRuleEntryParamsimplementation should be fixed to explicitly check for null and throw a descriptiveIllegalArgumentException(e.g., "location parameter is required") before attempting to parse.Apply this diff to temporarily fix the test to match current behavior:
- @Test(expected = NumberFormatException.class) + @Test(expected = NullPointerException.class) public void testMissingLocation() {However, the recommended long-term fix is to update
RuleEntryParamsto validate the presence of "location" before parsing, then update this test to expectIllegalArgumentException.java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/VariableDeclarationSymbolProvider.java (1)
107-108: Still using incorrect location in fallback path.This is the same issue flagged in the previous review. Line 107 still uses
getLocation(element, match), which anchors every symbol to the type-reference match position instead of the actual variable identifier. TheVariableInfocaptures the correct offset and length for each variable name (lines 89-90), but these values are never used.To fix this, you need to construct the location from
varInfo.offsetandvarInfo.lengthusing the compilation unit. Here's the corrected approach:// Create location for the variable name - symbol.setLocation(getLocation(element, match)); + org.eclipse.lsp4j.Location location = new org.eclipse.lsp4j.Location(); + location.setUri(unit.getResource().getLocationURI().toString()); + org.eclipse.lsp4j.Range range = new org.eclipse.lsp4j.Range(); + int startLine = cu.getLineNumber(varInfo.offset) - 1; + int startCol = cu.getColumnNumber(varInfo.offset); + int endLine = cu.getLineNumber(varInfo.offset + varInfo.length) - 1; + int endCol = cu.getColumnNumber(varInfo.offset + varInfo.length); + range.setStart(new org.eclipse.lsp4j.Position(startLine, startCol)); + range.setEnd(new org.eclipse.lsp4j.Position(endLine, endCol)); + location.setRange(range); + symbol.setLocation(location);Without this fix, "go to definition" will incorrectly navigate to the type token rather than the variable name.
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandlerTest.java (2)
34-46: Use consistent project name "test-project" across test suite.Line 38 uses
"project"while all other tests (15+) use"test-project". This inconsistency was flagged in a previous review but remains unresolved. Update to"test-project"for consistency with test infrastructure.Apply this diff:
- Map<String, String> param = Map.of( - "project", "project", + Map<String, String> param = Map.of( + "project", "test-project",
236-249: Replace "TestClass" with an existing test class.Line 241 queries for
"TestClass"which doesn't exist in test-project (as flagged in previous review). This test will always return empty results and fails to validate class_declaration location handling. Update to an existing class like"SampleApplication","Calculator", or"EnumExample"fromio.konveyor.demo.Apply this diff to use an existing class:
Map<String, String> param = Map.of( "project", "test-project", - "query", "TestClass", + "query", "SampleApplication", "location", "14", // class_declaration "analysisMode", "source-only"java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/SampleApplication.java (3)
40-43: Document intent if incomplete initialization is deliberate.The parameterized constructor leaves
configFileuninitialized, which can causeNullPointerExceptionwhenwriteToFile()orfileOperations()is called. This issue was previously flagged. If this incomplete initialization is intentional for testing error scenarios, please add a comment documenting the intent. Otherwise, initializeconfigFileto a default value.
66-71: Resource leak remains unaddressed.The
FileWriteris not properly closed ifwrite()throws an exception, causing a resource leak. This issue was previously flagged. Use try-with-resources to ensure proper cleanup:-public void writeToFile(String data) throws IOException { - // Constructor call with chained method call - FileWriter writer = new FileWriter(configFile); - writer.write(data); - writer.close(); -} +public void writeToFile(String data) throws IOException { + // Constructor call with chained method call + try (FileWriter writer = new FileWriter(configFile)) { + writer.write(data); + } +}If this resource leak is intentional for testing error scenarios, please add a comment documenting the intent.
79-91: Inverted directory existence logic remains unaddressed.The directory existence check at lines 85-87 is backwards: the code calls
dir.mkdirs()only when the directory EXISTS, not when it doesn't exist. This issue was previously flagged:-if (dir.exists()) { +if (!dir.exists()) { dir.mkdirs(); }If this inverted logic is intentional for testing error scenarios, please add a comment documenting the intent.
java-analyzer-bundle.test/docs/quick-reference.md (1)
338-340: Fix broken documentation reference.The link to
QUERY_COVERAGE.mdappears to be incorrect. Based on the past review comment, this should referencedocs/query-reference.mdinstead. Please verify the correct path and update the link accordingly.java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/controller/CustomerController.java (2)
25-33: Fix the path mapping to include path variable template.The
@GetMappingvalue is missing the template variable syntax, preventing@PathVariable("id")from binding correctly. This will cause the endpoint to fail at runtime.Apply this diff:
- @GetMapping(value = "id", produces = MediaType.APPLICATION_JSON_VALUE) + @GetMapping(value = "/{id}", produces = MediaType.APPLICATION_JSON_VALUE)
29-29: Correct the error message to refer to "customer" instead of "order".The error message references "order" but this endpoint handles customer resources.
Apply this diff:
- throw new ResourceNotFoundException("Requested order doesn't exist"); + throw new ResourceNotFoundException("Requested customer doesn't exist");java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/config/PersistenceConfig.java (3)
41-51: Inject ApplicationConfiguration instead of creating new instances.Creating a new
ApplicationConfigurationinstance inside the@Beanmethod bypasses Spring's dependency injection and could lead to inconsistent configuration.Apply constructor injection:
+ private final ApplicationConfiguration config; + + public PersistenceConfig(ApplicationConfiguration config) { + this.config = config; + } + @Bean public DataSource dataSource() { - ApplicationConfiguration config = new ApplicationConfiguration(); final DriverManagerDataSource dataSource = new DriverManagerDataSource();
53-58: Inject EntityManagerFactory instead of calling the bean method directly.Calling
entityManagerFactory().getObject()directly bypasses Spring-managed bean injection and can cause lifecycle issues.Apply this diff:
@Bean - public PlatformTransactionManager transactionManager() { + public PlatformTransactionManager transactionManager(LocalContainerEntityManagerFactoryBean entityManagerFactory) { final JpaTransactionManager transactionManager = new JpaTransactionManager(); - transactionManager.setEntityManagerFactory(entityManagerFactory().getObject()); + transactionManager.setEntityManagerFactory(entityManagerFactory.getObject()); return transactionManager; }
65-73: Use injected ApplicationConfiguration in helper method.This method creates another new
ApplicationConfigurationinstance instead of using the injected dependency.After applying constructor injection from the previous comment:
final Properties additionalProperties() { - ApplicationConfiguration config = new ApplicationConfiguration(); final Properties hibernateProperties = new Properties();java-analyzer-bundle.test/projects/customers-tomcat-legacy/pom.xml (3)
19-19: Update Spring Framework to address critical vulnerabilities.Spring Web/WebMVC 5.3.7 has multiple critical vulnerabilities including DoS, path traversal, and RCE. Even in test projects, vulnerable dependencies create supply chain risks.
Apply this diff:
- <spring-framework.version>5.3.7</spring-framework.version> + <spring-framework.version>5.3.39</spring-framework.version>
117-119: Update logback-classic to address critical vulnerabilities.Version 1.1.7 has multiple critical deserialization vulnerabilities. Even for test projects, using vulnerable dependencies creates supply chain risks.
Apply this diff:
<dependency> <groupId>ch.qos.logback</groupId> <artifactId>logback-classic</artifactId> - <version>1.1.7</version> + <version>1.3.14</version> </dependency>
127-129: Update PostgreSQL driver to address critical vulnerabilities.Version 42.2.23 has multiple critical vulnerabilities including SQL injection, arbitrary file write, and path traversal.
Apply this diff:
<dependency> <groupId>org.postgresql</groupId> <artifactId>postgresql</artifactId> - <version>42.2.23</version> + <version>42.7.4</version> </dependency>Makefile (1)
19-23: MakeREPO_ROOTindependent of the caller’s working directory.
REPO_ROOT := $(shell pwd)works only whenmakeis invoked from the repo root. Withmake -Cor other entry points, the volume mount inrun-integration-testscan point at the wrong path.Derive
REPO_ROOTfrom the Makefile’s location instead:-REPO_ROOT := $(shell pwd) +REPO_ROOT := $(dir $(abspath $(lastword $(MAKEFILE_LIST))))This keeps the mount stable regardless of where
makeis run from.Also applies to: 103-110
java-analyzer-bundle.test/integration/client/jdtls_client.go (3)
32-37: SimplifyCmdDialer: remove unusederrfield and guards.
CmdDialer.erris never set, so the checks inRead,Write, andDialnever trigger. This adds complexity without value.You can simplify:
-type CmdDialer struct { - Cmd *exec.Cmd - Stdin io.WriteCloser - Stdout io.ReadCloser - err error -} +type CmdDialer struct { + Cmd *exec.Cmd + Stdin io.WriteCloser + Stdout io.ReadCloser +} @@ func (rwc *CmdDialer) Read(p []byte) (int, error) { - if rwc.err != nil { - return 0, fmt.Errorf("cannot read: %w", rwc.err) - } return rwc.Stdout.Read(p) } @@ func (rwc *CmdDialer) Write(p []byte) (int, error) { - if rwc.err != nil { - return 0, fmt.Errorf("cannot write: %w", rwc.err) - } return rwc.Stdin.Write(p) } @@ func (rwc *CmdDialer) Dial(ctx context.Context) (io.ReadWriteCloser, error) { - if rwc.err != nil { - return rwc, fmt.Errorf("cannot dial: %w", rwc.err) - } return rwc, nil }Also applies to: 39-50, 65-68
107-115: MakeconfigDirselection OS-aware instead of hardcodingconfig_linux.Right now the JDT.LS config directory is always
<jdtlsPath>/config_linux, which breaks if these tests are ever run on macOS/Windows or with a different JDT.LS layout.Consider deriving the suffix from
runtime.GOOSand adding the import:+import "runtime" @@ - configDir := filepath.Join(c.jdtlsPath, "config_linux") + var cfgSuffix string + switch runtime.GOOS { + case "darwin": + cfgSuffix = "config_mac" + case "windows": + cfgSuffix = "config_win" + default: + cfgSuffix = "config_linux" + } + configDir := filepath.Join(c.jdtlsPath, cfgSuffix)Please double‑check the actual directory names in the JDT.LS distribution you’re using and adjust the suffixes accordingly.
304-359: HardenShutdownagainst nilconn/ partially started processes.
Shutdownassumesc.conn,c.cancel, andc.cmdare always non‑nil. If a future caller invokesShutdown/Closeafter a failedStart(or beforeStart), this will panic.Add a fast path that cleans up what exists without dereferencing nils, e.g.:
func (c *JDTLSClient) Shutdown() error { c.logger.Info("Shutting down JDT.LS server...") + // If no connection was ever established, just tear down process/dialer/cancel. + if c.conn == nil { + if c.cancel != nil { + c.cancel() + } + if c.dialer != nil { + _ = c.dialer.Close() + } + if c.cmd != nil && c.cmd.Process != nil { + _ = c.cmd.Process.Kill() + _ = c.cmd.Wait() + } + return nil + } + // Create a context with timeout for shutdown requests shutdownCtx, shutdownCancel := context.WithTimeout(c.ctx, 3*time.Second)This keeps the client safe if its lifecycle is used differently in the future.
java-analyzer-bundle.test/integration/integration_test.go (1)
100-145: Fix inheritance test expectations and theexceptedFileNametypo.The table-driven inheritance tests have two issues:
- The "Find DataService extends BaseService" case still expects
"SampleApplication", so it will not fail ifDataServiceis missing.- The field name
exceptedFileNameis a typo and the error message is hardcoded toSampleApplication.Consider:
- exceptedFileName string + expectedFileName string @@ - Name: "Find DataService extends BaseService", + Name: "Find DataService extends BaseService", projectName: "test-project", query: "io.konveyor.demo.inheritance.BaseService", location: 1, analysisMode: "source-only", - exceptedFileName: "SampleApplication", + expectedFileName: "DataService", @@ - if !verifySymbolInResults(symbols, tc.exceptedFileName) { - t.Errorf("SampleApplication not found in results") + if !verifySymbolInResults(symbols, tc.expectedFileName) { + t.Errorf("%s not found in results", tc.expectedFileName) }Apply the
expectedFileNamerename consistently across all test cases and usages.
🧹 Nitpick comments (12)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/ConstructorCallSymbolProvider.java (2)
55-60: Remove unused element discovery code.The
elementvariable is assigned but never used beyond logging at line 57. This appears to be leftover debug code that should be removed to avoid unnecessary computation and improve code clarity.Additionally, line 57 has a typo:
"je"should be"je: "for consistency with other log statements.Apply this diff to remove the unused code:
- IJavaElement element = JDTUtils.findElementAtSelection(mod.getTypeRoot(), location.getRange().getStart().getLine(), location.getRange().getStart().getCharacter(), JavaLanguageServerPlugin.getPreferencesManager(), new NullProgressMonitor()); - - logInfo("je" + element); - - -And remove the now-unused imports at the top of the file:
-import org.eclipse.core.runtime.NullProgressMonitor; import org.eclipse.jdt.core.IClassFile; import org.eclipse.jdt.core.ICompilationUnit; import org.eclipse.jdt.core.IJavaElement; import org.eclipse.jdt.core.IMethod; import org.eclipse.jdt.core.dom.AST; import org.eclipse.jdt.core.dom.ASTParser; import org.eclipse.jdt.core.dom.CompilationUnit; import org.eclipse.jdt.core.search.MethodReferenceMatch; import org.eclipse.jdt.core.search.SearchMatch; import org.eclipse.jdt.internal.core.JavaElement; -import org.eclipse.jdt.ls.core.internal.JDTUtils; -import org.eclipse.jdt.ls.core.internal.JavaLanguageServerPlugin;
69-78: Consider improving code formatting and comment placement.The indentation at line 75 appears inconsistent with the surrounding code block, and the comment at line 74 is oddly placed right before
cu.accept(visitor)rather than near the binding resolution logic it references.Consider reformatting for better readability:
ASTParser astParser = ASTParser.newParser(AST.getJLSLatest()); astParser.setSource(unit); astParser.setResolveBindings(true); CompilationUnit cu = (CompilationUnit) astParser.createAST(null); CustomASTVisitor visitor = new CustomASTVisitor(query, match, QueryLocation.CONSTRUCTOR_CALL); - // Under tests, resolveConstructorBinding will return null if there are problems - cu.accept(visitor); - if (visitor.symbolMatches()) { - symbols.add(symbol); - } + // Under tests, resolveConstructorBinding will return null if there are problems + cu.accept(visitor); + if (visitor.symbolMatches()) { + symbols.add(symbol); + }java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/jms/TopicMessageProcessor.java (1)
18-21: LGTM! The implementation appropriately serves its test purpose.The
onMessagemethod correctly implements theMessageListenerinterface with a simple console output for verification. This is suitable for integration testing where the goal is to verify that the analyzer correctly detects JMS Topic configurations.Optional: Consider using a proper logger instead of
System.outto align with best practices, even in test code:+import java.util.logging.Logger; + public class TopicMessageProcessor implements MessageListener { + private static final Logger LOGGER = Logger.getLogger(TopicMessageProcessor.class.getName()); @Override public void onMessage(Message message) { - System.out.println("Processing topic message: " + message); + LOGGER.info("Processing topic message: " + message); } }java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/query/AnnotationQueryTest.java (2)
37-77: Pattern matching tests are comprehensive.The tests cover exact matching, regex patterns, wildcards, and null type scenarios effectively.
Consider adding a test for invalid regex patterns to ensure graceful error handling:
@Test(expected = PatternSyntaxException.class) public void testMatchesAnnotationInvalidPattern() { AnnotationQuery query = new AnnotationQuery("[invalid(regex", new HashMap<>(), false); query.matchesAnnotation("javax.ejb.Stateless"); }
79-157: fromMap tests are correct but use magic numbers for location types.The tests correctly verify the factory method behavior across different scenarios. However, the hardcoded location values (4 for annotation location, 10 for type location) reduce maintainability.
Consider extracting location type constants to improve maintainability and self-documentation:
private static final int LOCATION_ANNOTATION = 4; private static final int LOCATION_TYPE = 10; // Then use in tests: AnnotationQuery query = AnnotationQuery.fromMap("fallbackQuery", annotationQueryMap, LOCATION_ANNOTATION);Alternatively, if these constants exist in the production code (e.g., in a
LocationTypeclass or enum), import and use them directly.java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/VariableDeclarationSymbolProvider.java (1)
80-81: Consider tightening the overlap condition.The current logic matches if
matchOffsetfalls anywhere within the type node range. While functional, it might match unintended statements if there are nested type references or complex expressions. Consider checking both start and end boundaries:- if (typeStart == matchOffset || - (matchOffset >= typeStart && matchOffset < typeStart + typeLength)) { + if (matchOffset >= typeStart && + matchOffset + matchLength <= typeStart + typeLength) {This ensures the entire match range is contained within the type node.
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandlerTest.java (2)
48-61: Document the location code for clarity.Location code
"10"(line 54) is not documented with an inline comment, unlike most other tests. Add a comment (e.g.,// type) to clarify what this location type represents for better test maintainability.
176-189: Document location code and note similarity to testRuleEntryWithFullMode.This test is very similar to
testRuleEntryWithFullMode(lines 48-61): both query"java.io.*"with location"10", differing only inanalysisMode. Consider:
- Adding a comment on line 182 to document what location "10" represents (appears to be a type/class location).
- Clarifying via test name or comments that this test specifically validates source-only mode behavior with wildcard queries.
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/persistence/JdbcOnlyService.java (1)
19-30: Consider try-with-resources for more robust resource management.While explicit
close()calls are present, an exception occurring between resource creation and the close statements would cause a leak. Try-with-resources ensures cleanup even when exceptions occur.Apply this diff to use try-with-resources:
public void executeQuery(Connection conn) throws SQLException { // Uses only JDBC PreparedStatement - PreparedStatement stmt = conn.prepareStatement("SELECT * FROM products"); - ResultSet rs = stmt.executeQuery(); - - while (rs.next()) { - System.out.println("Product: " + rs.getString("name")); + try (PreparedStatement stmt = conn.prepareStatement("SELECT * FROM products"); + ResultSet rs = stmt.executeQuery()) { + + while (rs.next()) { + System.out.println("Product: " + rs.getString("name")); + } } - - rs.close(); - stmt.close(); }java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/EnumConstantSymbolProvider.java (1)
28-42: Consider logging unexpected element types for debugging.When the element is not a
FIELDtype (line 29), the method silently returns an empty list. While this might be intentional, adding a debug log statement would help troubleshoot cases where enum constant searches unexpectedly return no results.Apply this diff to add optional debug logging:
// Enum constants are fields, so we need to check if the element is a field if (element.getElementType() == IJavaElement.FIELD) { IField field = (IField) element; // Only include if it's actually an enum constant if (field.isEnumConstant()) { SymbolInformation symbol = new SymbolInformation(); symbol.setName(field.getElementName()); symbol.setKind(convertSymbolKind(field)); symbol.setContainerName(field.getParent().getElementName()); symbol.setLocation(getLocation(field, match)); symbols.add(symbol); } + } else { + logInfo("EnumConstantSymbolProvider: element is not a FIELD, type=" + element.getElementType()); }java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/PackageDeclarationSymbolProvider.java (1)
72-74: Use structured logging instead of printStackTrace().The exception handler uses
printStackTrace()which outputs to stderr. For consistency with the rest of the codebase, use structured logging.Apply this diff:
} catch (Exception e) { logInfo("Error processing package declaration: " + e.toString()); - e.printStackTrace(); + logInfo("Stack trace: " + java.util.Arrays.toString(e.getStackTrace())); }java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/service/CustomerService.java (1)
13-34: Service behavior is fine; a couple of optional style tweaks.The implementation correctly delegates to
CustomerRepositoryand adds useful debug logging. If you want to align more with typical Spring style later:
- Prefer constructor injection over field
@Autowiredto ease testing.- Make the logger a
private static finalwith an upper‑case name:private static final Logger LOGGER = Logger.getLogger(CustomerService.class);These are non-blocking; the current code is acceptable for this test project.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
java-analyzer-bundle.test/integration/go.sumis excluded by!**/*.sum
📒 Files selected for processing (71)
.github/workflows/integration-tests.yml(1 hunks).gitignore(1 hunks)Dockerfile(2 hunks)Dockerfile.test(1 hunks)Makefile(1 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java(6 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SymbolInformationTypeRequestor.java(2 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/ConstructorCallSymbolProvider.java(3 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java(4 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/EnumConstantSymbolProvider.java(1 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/PackageDeclarationSymbolProvider.java(1 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/ReturnTypeSymbolProvider.java(2 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/SymbolProviderResolver.java(1 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/VariableDeclarationSymbolProvider.java(1 hunks)java-analyzer-bundle.test/README.md(1 hunks)java-analyzer-bundle.test/docs/README.md(1 hunks)java-analyzer-bundle.test/docs/integration-tests.md(1 hunks)java-analyzer-bundle.test/docs/query-reference.md(1 hunks)java-analyzer-bundle.test/docs/quick-reference.md(1 hunks)java-analyzer-bundle.test/docs/test-projects.md(1 hunks)java-analyzer-bundle.test/integration/client/jdtls_client.go(1 hunks)java-analyzer-bundle.test/integration/go.mod(1 hunks)java-analyzer-bundle.test/integration/integration_test.go(1 hunks)java-analyzer-bundle.test/integration/run_local.sh(1 hunks)java-analyzer-bundle.test/integration/test_helpers.go(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/.gitignore(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/Dockerfile(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/Makefile(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/pom.xml(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/OrderManagementAppInitializer.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/config/PersistenceConfig.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/config/WebConfig.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/controller/CustomerController.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/exception/ResourceNotFoundException.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/exception/handler/ExceptionHandlingController.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/model/Customer.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/repository/CustomerRepository.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/service/CustomerService.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/service/ICustomerService.java(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/resources/import.sql(1 hunks)java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/resources/persistence.properties(1 hunks)java-analyzer-bundle.test/projects/maven/springboot-todo/.project(1 hunks)java-analyzer-bundle.test/projects/test-project/.classpath(1 hunks)java-analyzer-bundle.test/projects/test-project/.project(1 hunks)java-analyzer-bundle.test/projects/test-project/.settings/org.eclipse.core.resources.prefs(1 hunks)java-analyzer-bundle.test/projects/test-project/.settings/org.eclipse.jdt.apt.core.prefs(1 hunks)java-analyzer-bundle.test/projects/test-project/.settings/org.eclipse.jdt.core.prefs(1 hunks)java-analyzer-bundle.test/projects/test-project/.settings/org.eclipse.m2e.core.prefs(1 hunks)java-analyzer-bundle.test/projects/test-project/pom.xml(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/Calculator.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/EnumExample.java(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/projects/test-project/src/main/java/io/konveyor/demo/annotations/CustomAnnotation.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/annotations/DeprecatedApi.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/config/DataSourceConfig.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/config/MySQLDataSourceConfig.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/entity/Product.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/inheritance/BaseService.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/inheritance/CustomException.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/inheritance/DataService.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/jms/MessageProcessor.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/jms/TopicMessageProcessor.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/persistence/AnotherMixedService.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/persistence/JdbcOnlyService.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/persistence/PureJpaService.java(1 hunks)java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/persistence/ServiceWithEntityManager.java(1 hunks)java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/RuleEntryParamsTest.java(1 hunks)java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandlerTest.java(1 hunks)java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/query/AnnotationQueryTest.java(1 hunks)pom.xml(4 hunks)
✅ Files skipped from review due to trivial changes (2)
- java-analyzer-bundle.test/docs/query-reference.md
- java-analyzer-bundle.test/README.md
🚧 Files skipped from review as they are similar to previous changes (30)
- java-analyzer-bundle.test/projects/test-project/.settings/org.eclipse.jdt.apt.core.prefs
- java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/OrderManagementAppInitializer.java
- java-analyzer-bundle.test/integration/go.mod
- java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/config/WebConfig.java
- java-analyzer-bundle.test/projects/test-project/.settings/org.eclipse.m2e.core.prefs
- java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/service/ICustomerService.java
- java-analyzer-bundle.test/projects/test-project/pom.xml
- java-analyzer-bundle.test/integration/test_helpers.go
- java-analyzer-bundle.test/projects/customers-tomcat-legacy/Makefile
- java-analyzer-bundle.test/projects/test-project/.settings/org.eclipse.core.resources.prefs
- java-analyzer-bundle.test/projects/customers-tomcat-legacy/.gitignore
- java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/resources/import.sql
- java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/jms/MessageProcessor.java
- pom.xml
- java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java
- java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/config/DataSourceConfig.java
- java-analyzer-bundle.test/integration/run_local.sh
- java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/entity/Product.java
- .gitignore
- java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/config/MySQLDataSourceConfig.java
- java-analyzer-bundle.test/docs/test-projects.md
- java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SymbolInformationTypeRequestor.java
- java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/exception/handler/ExceptionHandlingController.java
- java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/inheritance/DataService.java
- java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/EnumExample.java
- java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/ServletExample.java
- java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/inheritance/BaseService.java
- java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/annotations/DeprecatedApi.java
- .github/workflows/integration-tests.yml
- java-analyzer-bundle.test/projects/test-project/.classpath
🔇 Additional comments (63)
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/inheritance/CustomException.java (1)
1-21: Custom exception implementation looks correct and idiomaticThe class cleanly follows the standard pattern for a checked custom exception (package,
serialVersionUID, and three common constructors delegating tosuper). This is appropriate for its role as a test fixture for inheritance behavior. No changes needed.java-analyzer-bundle.test/projects/maven/springboot-todo/.project (1)
23-33: LGTM! Standard Eclipse resource filtering configuration.The filteredResources section appropriately excludes common directories and files from the Eclipse project explorer (node_modules, .git, and JDT.LS marker files). This is a standard configuration for Java development environments.
java-analyzer-bundle.test/projects/test-project/.project (1)
1-34: LGTM! Well-structured Eclipse project configuration.The project descriptor is correctly configured with appropriate build specifications, natures, and resource filtering. The filteredResources section uses the same pattern as the springboot-todo project, maintaining consistency across the test suite while using a unique filter ID.
java-analyzer-bundle.test/docs/README.md (2)
7-7: Verify that all cross-referenced documentation files and relative paths are present and correctly positioned.The README references four related documentation files (quick-reference.md, query-reference.md, integration-tests.md, test-projects.md) and two external resources via relative paths (../../README.md, ../../CLAUDE.md). Confirm these files exist in the PR and that the relative paths resolve correctly from the docs folder context.
Also applies to: 22-22, 37-37, 53-53, 305-306
128-138: Verify test counts and Phase 1/Phase 2 test structure match actual test implementations.The documentation specifies 48 tests in Phase 1 (Maven unit tests) and 16 functions in Phase 2 (Go integration tests). Confirm these counts are accurate by cross-referencing with the actual test files and ensure the test names listed (RuleEntryParamsTest, AnnotationQueryTest, SampleDelegateCommandHandlerTest for Phase 1; and the 16 TestXxx functions for Phase 2) correspond to the actual test implementations.
Also applies to: 142-166
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/jms/TopicMessageProcessor.java (4)
1-6: LGTM!The package declaration and imports are appropriate for a JMS message-driven bean test class. All necessary JEE and JMS types are correctly imported.
8-11: LGTM!The Javadoc clearly documents the test purpose, making it easy to understand that this class is designed to test JMS Topic configuration detection.
12-15: LGTM!The
@MessageDrivenannotation is correctly configured for JMS Topic message processing. The activation config properties properly specify a Topic destination type and lookup name.
16-16: LGTM!The class declaration correctly implements
MessageListener, which is the required interface for message-driven beans.java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/Calculator.java (7)
1-8: LGTM! Clear test fixture structure.The package structure, import, and class documentation are well-organized. The Javadoc clearly states the testing purpose (location type 7 coverage and method declarations), which helps maintainability of the integration test suite.
10-13: LGTM! Simple and effective test method.The
addmethod correctly demonstrates integer return types for the analyzer test coverage.
15-21: LGTM! Appropriate error handling for test fixture.The
dividemethod correctly demonstrates double return types and includes proper division-by-zero checking. Theb == 0comparison is acceptable for checking against the literal zero value.
23-26: LGTM! Clean string formatting method.The
formatmethod appropriately tests String return types for the integration test coverage.
28-31: LGTM! Tests custom return types effectively.The
getStatusmethod demonstrates the analyzer's capability to handle custom enum return types.
33-37: LGTM! Intentional deprecation for annotation testing.The
summethod is correctly annotated with the custom@DeprecatedApiannotation, allowing the integration tests to verify the analyzer's handling of deprecated methods and custom annotations.
39-42: LGTM! Appropriate void method for testing.The
resetmethod correctly demonstrates void return types. The console output is acceptable for a test fixture and may serve test verification purposes.java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/RuleEntryParamsTest.java (5)
19-51: LGTM! Basic parameter parsing tests are well-structured.The tests correctly validate parsing of project name, query, location, and analysis modes (both "full" and "source-only"). The assertions are appropriate and the test data is representative.
53-154: LGTM! Configuration parameter tests provide excellent coverage.The tests comprehensively validate included paths, open-source libraries flag (including default behavior), and Maven repository paths. The explicit test for the default value of
includeOpenSourceLibraries(lines 108-122) is particularly good practice.
156-173: LGTM! Comprehensive location type coverage.Excellent use of a parameterized approach to validate all 15 location types (0-14). This ensures complete coverage and makes it easy to maintain if location types are added or modified.
175-216: LGTM! Annotation query tests handle both present and absent scenarios.The tests properly validate complex nested parameter structures (lines 184-194) and correctly assert null when the optional
annotationQueryparameter is omitted. Good coverage of optional parameter handling.
218-236: LGTM! Error condition tests correctly validate exception handling.The test for missing arguments (lines 218-223) appropriately expects
UnsupportedOperationException, and the invalid location format test (lines 225-236) correctly expectsNumberFormatExceptionwhen parsing a non-numeric string.java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/query/AnnotationQueryTest.java (3)
18-35: LGTM! Basic constructor and getter tests are well-structured.The tests correctly verify the constructor parameters and getter methods with clear assertions.
179-191: Verify that mutable shared state is the intended design.The test explicitly verifies that the
AnnotationQuerydoes not perform a defensive copy of the elements map, meaning external modifications affect the query's internal state. This violates encapsulation and can lead to unexpected behavior.Please confirm this is the intended design. If not, consider implementing defensive copying:
// In AnnotationQuery constructor: this.elements = new HashMap<>(elements); // And update the test expectation: assertEquals("original", query.getElements().get("value"));If this behavior is intentional (e.g., for performance reasons), consider documenting this in the production class and making the elements map unmodifiable via
Collections.unmodifiableMap()to prevent accidental modifications while still avoiding the copy overhead.
193-231: LGTM! Additional test coverage is thorough.The tests effectively cover complex regex patterns, empty collections, and behavior across multiple location types. The
testFromMapMultipleLocationsis particularly valuable for verifying that only location 4 triggers annotation-specific behavior.java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/VariableDeclarationSymbolProvider.java (3)
32-34: LGTM! Type guard is appropriate.The early type check prevents processing incompatible match types and ensures type safety for the cast on line 36.
39-48: LGTM! Fast path handles standard cases correctly.When
getLocalElement()succeeds, this path correctly constructs the symbol from theILocalVariablewith proper name, kind, container, and location.
64-67: AST parsing approach is appropriate for the fallback case.Disabling binding resolution (line 66) is a good optimization since only structural information is needed. Parsing the entire compilation unit does have performance implications, but this is acceptable given it's a fallback path for interface types where
getLocalElement()fails.java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandlerTest.java (6)
1-14: LGTM!The imports are appropriate and cover all necessary testing utilities, assertions, and collection types used throughout the test file.
16-27: LGTM!The class setup and initialization are clean. The "Integration test" label in the javadoc aligns with the PR's objective of adding comprehensive integration test coverage.
29-32: LGTM!The basic command test correctly verifies the simple "Hello World" response from the base command handler.
122-174: LGTM!These tests appropriately cover implements_type, import, includedPaths parameter handling, and exception behavior for unsupported commands. The use of HashMap in testRuleEntryWithIncludedPaths correctly accommodates the ArrayList parameter type.
191-234: LGTM!These tests appropriately cover package, field, and method declaration location types with well-documented location codes.
93-105: Verify javax.ejb.Stateless annotation exists in test-project.This test searches for
javax.ejb.Stateless(line 98). Ensure this annotation exists in the test-project classes, otherwise the test will always return empty results and won't effectively validate annotation location handling.Run the following script to verify the annotation exists:
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/persistence/ServiceWithEntityManager.java (1)
1-29: Test fixture serves its purpose for analyzer detection.This test class appropriately demonstrates mixed JPA/JDBC usage patterns for exercising the analyzer's symbol search and file filtering capabilities, as documented in the javadoc.
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/persistence/PureJpaService.java (1)
1-28: Test fixture appropriately demonstrates pure JPA usage pattern.This class correctly serves its documented purpose of providing test data containing only EntityManager usage without PreparedStatement, enabling verification of the analyzer's filtering capabilities.
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/persistence/AnotherMixedService.java (1)
1-27: Test fixture provides additional mixed usage scenario for analyzer testing.This class complements ServiceWithEntityManager by providing a second example of mixed JPA/JDBC usage, enhancing test coverage for the analyzer's file filtering and symbol search capabilities.
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/annotations/CustomAnnotation.java (1)
1-17: LGTM! Well-structured test annotation.The annotation definition is correct and appropriate for testing. Runtime retention enables reflection-based searches, and the target types (TYPE, METHOD, FIELD) provide comprehensive coverage for annotation search pattern testing.
java-analyzer-bundle.test/projects/test-project/src/main/java/io/konveyor/demo/SampleApplication.java (6)
1-23: LGTM! Clear test fixture structure.The class declaration, imports, and annotation usage are correct. The JavaDoc effectively documents the testing purpose and coverage of different JDT.LS location types.
25-31: Field declarations are correct.The field declarations appropriately demonstrate various types for testing field declaration searches.
33-38: Constructor patterns appear intentional for testing.The default constructor correctly initializes all fields. The use of
new String("Test Application")(line 35) and explicit type parameters (line 36) appear to be intentional test patterns to verify constructor call detection, as indicated by the comments.
46-63: LGTM! Methods demonstrate test patterns correctly.The
processData()andgetName()methods correctly demonstrate various patterns for testing (variable declarations, method calls, constructor calls, type references).
73-76: LGTM! Static method is correct.The static method correctly demonstrates a simple static method pattern for testing.
93-96: LGTM! Override implementation is correct.The
initialize()method correctly overrides the base class method with an appropriate test implementation.Dockerfile (2)
25-26: LGTM! Clean environment variable handling.Using
ENV JAVA_HOMEinstead of inlineexportis a Docker best practice that makes the environment variable available to all subsequent layers and improves maintainability.
54-54: LGTM! Plugin path aligns with JDT.LS conventions.The updated destination
/jdtls/plugins/matches the expected plugin loading path for JDT.LS, ensuring the Java Analyzer Bundle is correctly discovered at runtime.java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/SymbolProviderResolver.java (2)
18-18: LGTM! Enum constant provider registered correctly.The mapping for location type 6 correctly routes to
EnumConstantSymbolProvider, which handles enum constant references as documented in the integration test suite.
23-23: LGTM! Package declaration provider registered correctly.The mapping for location type 11 correctly routes to
PackageDeclarationSymbolProvider, completing the 100% location type coverage mentioned in the PR objectives.java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/ReturnTypeSymbolProvider.java (2)
10-10: LGTM! Enhanced matching with readable type conversion.The addition of
Signature.toString()to convert JVM type signatures (e.g., "I", "Ljava/lang/String;") to readable type names (e.g., "int", "java.lang.String") significantly improves query usability. Users can now search using human-readable type names instead of JVM internal signatures.Also applies to: 23-27
34-35: LGTM! Dual matching expands query coverage.The OR condition
signature.contains(s) || readableType.contains(s)ensures that searches succeed whether the query uses JVM signatures or readable type names, improving the user experience.java-analyzer-bundle.test/projects/test-project/.settings/org.eclipse.jdt.core.prefs (1)
1-9: LGTM! Eclipse JDT configuration aligns with test environment.The preferences file correctly configures the test project for Java 17 compilation, matching the Java 17 environment used throughout the integration test infrastructure.
java-analyzer-bundle.test/docs/integration-tests.md (1)
1-1028: Excellent comprehensive documentation!This documentation provides outstanding coverage of the Phase 2 integration testing framework:
✅ Clear architecture diagrams and component descriptions
✅ Detailed test scenarios with code examples for all 15 location types
✅ Troubleshooting guidance and debugging tips
✅ Coverage matrix showing 100% location type testing
✅ Migration use case examples (javax→jakarta, Spring patterns)
✅ Multiple execution methods (local, container, CI/CD)The documentation will be extremely valuable for developers working with the integration test suite.
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/EnumConstantSymbolProvider.java (1)
23-47: LGTM! Enum constant provider follows established patterns.The implementation correctly:
- Validates the element is a
FIELD(line 29)- Filters for enum constants using
isEnumConstant()(line 33)- Constructs
SymbolInformationfollowing the same pattern as other providers- Handles exceptions with appropriate logging
The provider integrates well with the symbol resolution framework and enables location type 6 (enum constant) searches as documented in the integration tests.
java-analyzer-bundle.test/projects/customers-tomcat-legacy/Dockerfile (1)
5-5: Verify Java 11 compatibility of the test project.The base image has been upgraded from Java 8 (maven:3.6-jdk-8-slim) to Java 11 (maven:3.8-openjdk-11). While this aligns with the broader test infrastructure using Java 17, ensure that the customers-tomcat-legacy project code is compatible with Java 11, particularly:
- Any deprecated APIs removed in Java 11
- Libraries that may not support Java 11
- Build configuration in pom.xml targeting the correct Java version
Run the following script to verify the build succeeds:
java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/exception/ResourceNotFoundException.java (1)
19-27: LGTM!Standard exception implementation following best practices with proper serialVersionUID and constructor delegation.
Dockerfile.test (1)
1-54: LGTM!Well-structured multi-stage build that efficiently creates a test environment with JDT.LS, Fernflower, Maven tooling, multiple JDK versions, and Go support. The stage separation minimizes final image size and build cache efficiency.
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java (5)
167-168: LGTM!Helpful comment clarifying that package searches always use PATTERN_MATCH due to Eclipse JDT's flexible package matching semantics.
183-186: Good enhancement for parameterized type matching.Adding R_ERASURE_MATCH for return types and variable declarations enables matching parameterized types (e.g.,
List<String>when searching forjava.util.List), improving search coverage for generic types.
194-195: Good enhancement for parameterized constructor matching.Adding R_ERASURE_MATCH for constructor calls enables matching parameterized constructors (e.g.,
ArrayList<String>when searching forjava.util.ArrayList).
199-200: Good enhancement for parameterized field type matching.Adding R_ERASURE_MATCH for field declarations enables matching parameterized field types.
205-207: LGTM!Correctly handles enum constants as FIELD references, improving search completeness.
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/PackageDeclarationSymbolProvider.java (1)
20-71: LGTM!Well-structured logic that correctly handles different element types (IPackageDeclaration, ICompilationUnit, IPackageFragment) with appropriate defensive checks and logging.
java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/model/Customer.java (1)
11-98: LGTM!Standard JPA entity implementation with appropriate column mappings, sequence generation, and accessor methods. The entity follows best practices for a simple data model.
java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/java/io/konveyor/demo/ordermanagement/repository/CustomerRepository.java (1)
1-8: Straightforward Spring Data repository – looks good.
CustomerRepositorycleanly extendsPagingAndSortingRepository<Customer, Long>without extra complexity. No issues from a test-fixture perspective.
| private static class VariableInfo { | ||
| String name; | ||
| int offset; | ||
| int length; | ||
|
|
||
| VariableInfo(String name, int offset, int length) { | ||
| this.name = name; | ||
| this.offset = offset; | ||
| this.length = length; | ||
| } | ||
| } |
There was a problem hiding this comment.
Remove unused fields or utilize them properly.
The VariableInfo class captures offset and length fields (lines 125-126), but these are never used in the symbol creation loop (lines 100-109). This indicates an incomplete fix—these fields were likely added to build accurate locations for each variable, but that implementation was never completed (see issue at lines 107-108).
Either:
- Use these fields to construct proper locations (as suggested in the comment above), or
- Remove the unused fields if you decide they're unnecessary
| jdbc.driverClassName=oracle.jdbc.driver.OracleDriver | ||
| jdbc.url=jdbc:oracle:thin:@169.60.225.216:1521/XEPDB1 | ||
| jdbc.user=customers | ||
| jdbc.password=customers | ||
| hibernate.hbm2ddl.auto=create-drop | ||
| hibernate.dialect=org.hibernate.dialect.OracleDialect No newline at end of file |
There was a problem hiding this comment.
Avoid committing real DB credentials and non-local endpoints, even in test fixtures.
This file hardcodes a real-looking IP and cleartext DB credentials. If these point at anything other than a throwaway local container, this is a security/compliance risk, and hbm2ddl.auto=create-drop could be destructive.
Consider:
- Switching to
localhost/container networking instead of a fixed external IP. - Replacing credentials with obviously fake sample values, or injecting them via env/overrides.
- Rotating any real credentials that may have been exposed by this commit.
🤖 Prompt for AI Agents
In
java-analyzer-bundle.test/projects/customers-tomcat-legacy/src/main/resources/persistence.properties
lines 1-6, the file contains hardcoded DB host, IP and cleartext credentials
plus hbm2ddl.auto=create-drop; replace the external IP with localhost or a
container network alias, remove or replace real credentials with obviously fake
placeholders or reference env vars/system properties (e.g. ${DB_USER},
${DB_PASS}, ${JDBC_URL}) so test fixtures don’t contain secrets, and change
hbm2ddl.auto away from create-drop for non-local runs (or guard it behind a
test-only profile); if these credentials were ever real, rotate them immediately
and use overrides for CI/local config instead of committing secrets.
Signed-off-by: Shawn Hurley <[email protected]>
Signed-off-by: Shawn Hurley <[email protected]>
1 participant
Summary by CodeRabbit
New Features
Tests
Documentation
Chores