✨ Add comprehensive integration test suite with CI/CD pipeline #165
Conversation
WalkthroughAdds phased CI and local Makefile, a Go-based Phase‑2 integration test harness with a JDT.LS client, two test projects with many Java sources, expanded unit and integration tests, multiple new/updated symbol providers and AST logic, container build artifacts, and extensive documentation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev
participant GHA as GitHub Actions
participant Make as Local Makefile
participant Builder as Image Builder
participant Container as Test Container
participant GoTest as Go test harness
participant JDTLS as jdt-language-server
Dev->>GHA: push / PR triggers workflow
GHA->>GHA: run Unit Tests (mvn clean integration-test)
GHA-->>Builder: on success → build image (Dockerfile.test)
Builder->>Container: run container (mount workspace, set envs)
Container->>GoTest: execute `go test -v`
GoTest->>JDTLS: launch server via CmdDialer, open JSON‑RPC
GoTest->>JDTLS: Initialize then ExecuteCommand / SearchSymbols
JDTLS-->>GoTest: return results
GoTest-->>Container: tests finish (exit code)
GHA->>Dev: upload artifacts, report results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas to focus on:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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
* Phase 1 Adds unit tests, for parsing, validation of the inpput to the handler. * Phase 2 adds a golang jsonrpc client to run a set of integration tests against test projects to verify the accuracy of results. * The testing infrastructure was also added to use the phases to validate any changes on PR or merge. * README.md in java-analyzer-bundle.test/integration/ to document the testing. 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
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.
| IMethodBinding binding = node.resolveMethodBinding(); | ||
| if (binding != null) { | ||
| // get fqn of the method being called | ||
| ITypeBinding declaringClass = binding.getDeclaringClass(); | ||
| if (declaringClass != null) { | ||
| // Handle Erasure results | ||
| if (declaringClass.getErasure() != null) { | ||
| declaringClass = declaringClass.getErasure(); | ||
| } | ||
| String fullyQualifiedName = declaringClass.getQualifiedName() + "." + binding.getName(); | ||
| // match fqn with query pattern | ||
| if (fullyQualifiedName.matches(this.query)) { | ||
| this.symbolMatches = true; | ||
| return false; | ||
| } else { | ||
| logInfo("method fqn " + fullyQualifiedName + " did not match with " + query); | ||
| return true; | ||
| } | ||
| } | ||
| if (binding == null) { | ||
| return false; | ||
| } | ||
| logInfo("getTypeParameters: " + binding.getTypeParameters()); | ||
| // get fqn of the method being called | ||
| ITypeBinding declaringClass = binding.getDeclaringClass(); | ||
| if (declaringClass == null) { | ||
| return false; | ||
| } | ||
| // Handle Erasure results | ||
| if (declaringClass.getErasure() != null) { | ||
| declaringClass = declaringClass.getErasure(); | ||
| } | ||
| String fullyQualifiedName = declaringClass.getQualifiedName() + "." + binding.getName(); | ||
| // match fqn with query pattern | ||
| // Note: We keep using regex matching for now as SearchPattern doesn't expose | ||
| // a simple string matching API. The pattern is passed for potential future use. | ||
| boolean matches = fullyQualifiedName.matches(this.query); | ||
| if (matches) { | ||
| this.symbolMatches = true; | ||
| return false; | ||
| } else { | ||
| logInfo("method fqn " + fullyQualifiedName + " did not match with " + query); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Restore fallback when method bindings cannot be resolved
If resolveMethodBinding() (or getDeclaringClass()) returns null we now exit without ever setting symbolMatches, so the caller drops the hit. The old logic accepted the match in this scenario to avoid false negatives on incomplete sources. Please keep that fallback.
try {
IMethodBinding binding = node.resolveMethodBinding();
- if (binding == null) {
- return false;
- }
+ 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;
- }
+ if (declaringClass == null) {
+ logInfo("failed to resolve declaring class, falling back");
+ this.symbolMatches = true;
+ return false;
+ }📝 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.
| IMethodBinding binding = node.resolveMethodBinding(); | |
| if (binding != null) { | |
| // get fqn of the method being called | |
| ITypeBinding declaringClass = binding.getDeclaringClass(); | |
| if (declaringClass != null) { | |
| // Handle Erasure results | |
| if (declaringClass.getErasure() != null) { | |
| declaringClass = declaringClass.getErasure(); | |
| } | |
| String fullyQualifiedName = declaringClass.getQualifiedName() + "." + binding.getName(); | |
| // match fqn with query pattern | |
| if (fullyQualifiedName.matches(this.query)) { | |
| this.symbolMatches = true; | |
| return false; | |
| } else { | |
| logInfo("method fqn " + fullyQualifiedName + " did not match with " + query); | |
| return true; | |
| } | |
| } | |
| if (binding == null) { | |
| return false; | |
| } | |
| logInfo("getTypeParameters: " + binding.getTypeParameters()); | |
| // get fqn of the method being called | |
| ITypeBinding declaringClass = binding.getDeclaringClass(); | |
| if (declaringClass == null) { | |
| return false; | |
| } | |
| // Handle Erasure results | |
| if (declaringClass.getErasure() != null) { | |
| declaringClass = declaringClass.getErasure(); | |
| } | |
| String fullyQualifiedName = declaringClass.getQualifiedName() + "." + binding.getName(); | |
| // match fqn with query pattern | |
| // Note: We keep using regex matching for now as SearchPattern doesn't expose | |
| // a simple string matching API. The pattern is passed for potential future use. | |
| boolean matches = fullyQualifiedName.matches(this.query); | |
| if (matches) { | |
| this.symbolMatches = true; | |
| return false; | |
| } else { | |
| logInfo("method fqn " + fullyQualifiedName + " did not match with " + query); | |
| return true; | |
| } | |
| 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; | |
| } | |
| // Handle Erasure results | |
| if (declaringClass.getErasure() != null) { | |
| declaringClass = declaringClass.getErasure(); | |
| } | |
| String fullyQualifiedName = declaringClass.getQualifiedName() + "." + binding.getName(); | |
| // match fqn with query pattern | |
| // Note: We keep using regex matching for now as SearchPattern doesn't expose | |
| // a simple string matching API. The pattern is passed for potential future use. | |
| boolean matches = fullyQualifiedName.matches(this.query); | |
| if (matches) { | |
| this.symbolMatches = true; | |
| return false; | |
| } else { | |
| logInfo("method fqn " + fullyQualifiedName + " did not match with " + query); | |
| return true; | |
| } |
🤖 Prompt for AI Agents
In
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java
around lines 146 to 171, the code returns early when resolveMethodBinding() or
getDeclaringClass() is null which drops potential matches; restore the previous
fallback by treating unresolved bindings or declaring classes as a match: when
binding == null or declaringClass == null set this.symbolMatches = true and
return false (instead of returning false/true as currently done), preserving the
existing regex matching logic for resolved bindings and keeping logging as-is.
| IMethodBinding binding = node.resolveConstructorBinding(); | ||
| 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 | ||
| if (declaringClass.getErasure() != null) { | ||
| declaringClass = declaringClass.getErasure(); | ||
| } | ||
| String fullyQualifiedName = declaringClass.getQualifiedName(); | ||
| // match fqn with query pattern | ||
| if (fullyQualifiedName.matches(this.query)) { | ||
| // Note: We keep using regex matching for now as SearchPattern doesn't expose | ||
| // a simple string matching API. The pattern is passed for potential future use. | ||
| boolean matches = fullyQualifiedName.matches(this.query); |
There was a problem hiding this comment.
Guard constructor bindings before logging
node.resolveConstructorBinding() can return null, yet we call binding.getTypeParameters() immediately. When that happens (e.g., broken code, missing deps), we’ll throw a NullPointerException and drop the match. Please bail out before logging so we keep the previous “fallback to accepting the match” behavior.
- 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());Apply the same guard in visit(ClassInstanceCreation) so both call sites stay safe.
Also applies to: 239-253
🤖 Prompt for AI Agents
In
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java
around lines 191-205 (and similarly at 239-253), the code calls
node.resolveConstructorBinding() and immediately uses
binding.getTypeParameters() which can NPE when resolveConstructorBinding()
returns null; guard the binding by checking if binding == null before any access
or logging and if null fall back to the previous behavior (bail out and accept
the match), and apply the same null-check guard to the
visit(ClassInstanceCreation) call site so both places avoid NullPointerException
and preserve the fallback matching behavior.
| String s = string.replaceAll("\\*", ""); | ||
| s = s.replaceAll("\\[", ""); | ||
| // check if the string found is apart of the signature. | ||
| // check if the string found is apart of the signature or readable type |
There was a problem hiding this comment.
Fix typo in comment.
The comment contains a typo: "apart of" should be "a part of". The word "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 type📝 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.
| // 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 type |
🤖 Prompt for AI Agents
In
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/ReturnTypeSymbolProvider.java
around line 32, fix the typo in the inline comment by changing "apart of" to "a
part of" so the comment reads that the string found is "a part of the signature
or readable type"; update only the comment text to correct the wording.
| symbol.setLocation(getLocation(element, match)); | ||
| symbols.add(symbol); |
There was a problem hiding this comment.
Fix variable name location in fallback path.
The fallback emits SymbolInformation per VariableInfo, but symbol.setLocation(getLocation(element, match)) still anchors every result to the type-reference match. That means “go to definition” (or any navigation) lands on the type token instead of the actual variable identifier, so the returned symbols are incorrect. Please build each location from the VariableInfo offset/length captured by the AST visitor (using the method’s compilation unit) instead of reusing the match location tied to the type node.
...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
| @Test(expected = NumberFormatException.class) | ||
| public void testMissingLocation() { | ||
| List<Object> arguments = new ArrayList<>(); | ||
| Map<String, Object> params = new HashMap<>(); | ||
| params.put("project", "test-project"); | ||
| params.put("query", "test.query"); | ||
| // Missing location | ||
| params.put("analysisMode", "full"); | ||
| arguments.add(params); | ||
|
|
||
| new RuleEntryParams("test-command", arguments); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify exception type for missing required parameter.
The test expects NumberFormatException when the location parameter is entirely missing from the map. This suggests RuleEntryParams attempts to parse a null value as a number. While this works, a more descriptive exception type (e.g., IllegalArgumentException with a message like "location parameter is required") would provide better diagnostics.
Verify this is the intended behavior in the RuleEntryParams implementation:
🏁 Script executed:
#!/bin/bash
# Description: Check RuleEntryParams constructor/parser implementation for location parameter handling
# Search for RuleEntryParams class definition and location parsing logic
ast-grep --pattern 'class RuleEntryParams {
$$$
}'
# Also look for location parameter parsing
rg -n -A 5 -B 2 'location.*parseInt\|getLocation\(' --type java -g '**/RuleEntryParams.java'Length of output: 0
---
🏁 Script executed:
```shell
#!/bin/bash
# Find all Java files containing RuleEntryParams class definition
fd --type f --extension java --exec grep -l "class RuleEntryParams" {} \;
# Also search more broadly for RuleEntryParams references
echo "---"
rg -n "class RuleEntryParams" --type java
Length of output: 463
🏁 Script executed:
#!/bin/bash
# Once we find the file, examine the RuleEntryParams implementation
# Looking for how it handles location parameter
# First, find the file
RULENTRY_FILE=$(find . -type f -name "*.java" -exec grep -l "class RuleEntryParams" {} \;)
if [ -n "$RULENTRY_FILE" ]; then
echo "Found RuleEntryParams in: $RULENTRY_FILE"
echo "---"
# Show the entire file or at least the constructor
cat "$RULENTRY_FILE" | head -100
else
echo "File not found, searching for location parameter parsing..."
rg -n "location" --type java -A 3 -B 3 | head -50
fiLength of output: 461
---
🏁 Script executed:
```shell
#!/bin/bash
# Read the RuleEntryParams implementation directly
cat ./java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/RuleEntryParams.java
Length of output: 2531
🏁 Script executed:
#!/bin/bash
# Search for location parameter handling and exception logic
rg -n "location" ./java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/RuleEntryParams.java -A 5 -B 2Length of output: 1288
Test expects incorrect exception type—will fail at runtime.
The testMissingLocation() test is broken. When obj.get("location") returns null on line 31 of RuleEntryParams.java, Integer.parseInt((String) null) throws NullPointerException, not NumberFormatException. The test must expect NullPointerException instead.
Additionally, handle the null case explicitly with a descriptive error message (e.g., via Optional or an explicit null check) rather than relying on an NPE for error reporting.
🤖 Prompt for AI Agents
In
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/RuleEntryParamsTest.java
around lines 238 to 249, the test currently expects NumberFormatException when
"location" is missing but the real behavior is a NullPointerException; instead
of relying on NPE, change RuleEntryParams to explicitly check for a null
"location" value and throw a clear IllegalArgumentException (with a descriptive
message like "missing or invalid location"), and update this test to expect
IllegalArgumentException; ensure the null check is performed before any
Integer.parseInt cast so the error message is clear and deterministic.
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]>
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.
| // CmdDialer wraps a command's stdin/stdout as an io.ReadWriteCloser | ||
| // This is based on github.com/konveyor/analyzer-lsp/lsp/base_service_client/cmd_dialer.go | ||
| 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) Close() error { | ||
| // Just close the pipes - don't kill or wait for the process | ||
| // The Shutdown() method handles process termination | ||
| if rwc.Stdin != nil { | ||
| rwc.Stdin.Close() | ||
| } | ||
| if rwc.Stdout != nil { | ||
| rwc.Stdout.Close() | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| 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 | ||
| } |
There was a problem hiding this comment.
CmdDialer.err field is never assigned.
The err field is checked in Read, Write, and Dial methods but is never set anywhere in the code. These guards are currently ineffective.
Either remove the unused err field and its checks, or set it appropriately (e.g., when the command fails to start, or when pipes fail).
-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)
-}
+type CmdDialer struct {
+ Cmd *exec.Cmd
+ Stdin io.WriteCloser
+ Stdout io.ReadCloser
+}
+
+func (rwc *CmdDialer) Read(p []byte) (int, error) {
+ return rwc.Stdout.Read(p)
+}
+
+func (rwc *CmdDialer) Write(p []byte) (int, error) {
+ return rwc.Stdin.Write(p)
+}Committable suggestion skipped: line range outside the PR's diff.
| InitializationOptions: map[string]any{ | ||
| "bundles": []string{ | ||
| "/jdtls/plugins/java-analyzer-bundle.core-1.0.0-SNAPSHOT.jar", | ||
| }, | ||
| "workspaceFolders": []string{string(workspaceURI)}, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Hardcoded bundle path reduces flexibility.
The bundle path is hardcoded to an absolute container path. This should be configurable to support different environments (local development, CI, different container layouts).
Consider accepting bundle paths as a constructor parameter or environment variable:
type JDTLSClient struct {
jdtlsPath string
workspaceDir string
+ bundlePaths []string
conn *jsonrpc2.Connection
...
}
-func NewJDTLSClient(jdtlsPath, workspaceDir string) *JDTLSClient {
+func NewJDTLSClient(jdtlsPath, workspaceDir string, bundlePaths []string) *JDTLSClient {
...
return &JDTLSClient{
jdtlsPath: jdtlsPath,
workspaceDir: workspaceDir,
+ bundlePaths: bundlePaths,
logger: logger,
}
}Then use c.bundlePaths in the InitializationOptions instead of the hardcoded slice.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In java-analyzer-bundle.test/integration/client/jdtls_client.go around lines 191
to 195, the InitializationOptions currently hardcodes an absolute bundle path
which reduces flexibility; change the client to accept bundle paths (e.g., via
constructor parameter or environment variable) and use that slice
(c.bundlePaths) in InitializationOptions instead of the hardcoded []string,
providing a sensible default/fallback when none provided and validating
non-empty entries before inserting them into the InitializationOptions so tests
and different environments can supply their own paths.
Signed-off-by: Shawn Hurley <[email protected]>
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.
1 participant
Summary by CodeRabbit
New Features
Improvements
Documentation