Add Test and utility classes to create a jdt-ls workspace and test the commands #174
Conversation
WalkthroughAdds a Spring Boot To‑Do Maven test project (sources, POM, wrapper scripts, templates, JS), updates bundle/classpath and VSCode settings, makes SampleDelegateCommandHandler.search protected with enhanced logging, and introduces test utilities and new JUnit tests plus job-synchronization helpers for Eclipse/JDT integration. Changes
Sequence Diagram(s)mermaid mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–75 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (3)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
java-analyzer-bundle.test/META-INF/MANIFEST.MF(1 hunks)java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/test/JavaUtils.java(1 hunks)java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/test/JobHelpers.java(1 hunks)java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/test/PomDependencyTest.java(1 hunks)java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/test/ProjectUtilsTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/test/PomDependencyTest.java (1)
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/test/ProjectUtilsTest.java (1)
MavenProjectName(56-59)
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/test/ProjectUtilsTest.java (1)
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/test/JavaUtils.java (1)
JavaUtils(32-136)
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/JavaUtils.java
Show resolved
Hide resolved
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/test/PomDependencyTest.java
Show resolved
Hide resolved
|
Would the changes in #165 cover the same use case or is this trying to do something else? |
Maybe. It depends what you want to cover part of the Unit Tests. This PR will allow to load a project (mave, gradle), set the parameters needed to mock the |
cmoulliard
changed the title
…adle and create some test classes to test the command able to find an annotation. konveyor#171 Signed-off-by: Charles Moulliard <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 23
♻️ Duplicate comments (2)
java-analyzer-bundle.core/.classpath (1)
3-4: Same Java version ambiguity as in test bundle.This file has the same issue as
java-analyzer-bundle.test/.classpath: moving from pinned JavaSE-17 to generic JRE_CONTAINER while leaving commented code. Please apply the same resolution across both bundles to maintain consistency.java-analyzer-bundle.test/projects/maven/springboot-todo/src/main/java/com/todo/app/service/TaskServiceImpl.java (1)
24-27: Remove duplicate delete method implementations.Both
deleteTaskByIdanddeleteTaskdo the same thing. This duplication matches the duplicate declarations in the TaskService interface. Once the interface is fixed, remove one of these implementations as well.Also applies to: 39-42
🧹 Nitpick comments (16)
java-analyzer-bundle.test/projects/maven/springboot-todo/src/README.md (1)
55-60: Clarify clone instructions for this test project context.The clone instructions (lines 58–59) reference the original external repository. Since this project is now part of the java-analyzer-bundle test suite, consider clarifying how developers should access/build this test project within the bundle context.
java-analyzer-bundle.test/projects/maven/springboot-todo/src/main/resources/templates/error.html (1)
17-50: Consider extracting inline styles to a separate stylesheet.For a test project, keeping inline CSS in the template file works, but extracting to a separate stylesheet (e.g.,
error.css) would improve maintainability and follow standard web project structure.java-analyzer-bundle.test/META-INF/MANIFEST.MF (1)
9-13: Consider adding version constraints for new bundle dependencies.The new bundle dependencies (
org.apache.commons.io,org.apache.commons.lang3,org.eclipse.m2e.core,org.eclipse.buildship.core) lack version constraints. While this provides flexibility, it may lead to runtime compatibility issues if incompatible versions are present.Consider adding minimum version constraints to ensure compatibility:
org.apache.commons.io, org.apache.commons.lang3, org.junit;bundle-version="4.12", - org.eclipse.m2e.core, - org.eclipse.buildship.core + org.eclipse.m2e.core;bundle-version="1.0.0", + org.eclipse.buildship.core;bundle-version="1.0.0"Note: Adjust the minimum versions based on your target Eclipse/OSGi platform.
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/CommandHandlerTest.java (1)
33-33: Use constant for location type.The location parameter is hardcoded as
"0", but the parent class definesLOCATION_TYPE_ANNOTATION = "4". Using magic strings makes the test harder to understand.If testing annotation location, use the constant:
- "location","0", + "location", LOCATION_TYPE_ANNOTATION,Or define a constant for location type
"0"if that's the intended value.java-analyzer-bundle.test/projects/maven/springboot-todo/src/main/java/com/todo/app/service/TaskService.java (1)
11-26: Remove redundantpublicmodifiers.Interface methods are implicitly public in Java. The explicit
publicmodifiers are redundant.Apply this diff:
- public void addTask(Task task); + void addTask(Task task); - public void deleteTaskById(Long id); + void deleteTaskById(Long id); - public void updateTaskById(Long id, Task task); + void updateTaskById(Long id, Task task); - public List<Task> getAllTasks(); + List<Task> getAllTasks(); - public void deleteTask(Long taskId); + void deleteTask(Long taskId); - Page<Task> getAllTasksPage(int pageNo, int pageSize); + Page<Task> getAllTasksPage(int pageNo, int pageSize);java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/test/JavaAnnotationTest.java (2)
62-62: UseassertTrueinstead ofassertEquals(true, ...).This is more idiomatic and provides better assertion failure messages.
- Assert.assertEquals(true, loc.getUri().contains("TaskController.java")); + Assert.assertTrue("Location URI should contain TaskController.java", loc.getUri().contains("TaskController.java"));
70-74: Hardcoded position values make test brittle.If the TaskController.java file is modified (e.g., imports added/removed), these exact line and character positions will break the test.
Consider verifying only that:
- The range is valid (start line ≤ end line)
- The annotation is found within a reasonable range
Or extract the expected values into named constants with comments explaining what they represent.
java-analyzer-bundle.test/projects/maven/springboot-todo/src/main/java/com/todo/app/entity/Task.java (1)
13-13: Consider using IDENTITY strategy for better compatibility.
GenerationType.AUTOcan behave differently across database providers. For a MySQL-backed application (as indicated in the broader project context),IDENTITYis more predictable.- @GeneratedValue(strategy = GenerationType.AUTO) + @GeneratedValue(strategy = GenerationType.IDENTITY)java-analyzer-bundle.test/projects/maven/springboot-todo/src/main/java/com/todo/app/service/TaskServiceImpl.java (1)
45-48: Document or reconsider 1-based page numbering.The code subtracts 1 from
pageNo, suggesting the API uses 1-based page indexing while Spring Data uses 0-based indexing. This is non-standard and may confuse users.Either:
- Document this behavior clearly in the interface with a JavaDoc comment, or
- Use 0-based indexing consistently throughout the application to match Spring Data conventions.
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java (1)
403-414: Consider guarding expensive logging operations.The detailed symbol logging uses
String.formatand stream operations that execute regardless of log level. For production performance, consider wrapping expensive logging in level checks.Example refactor:
- logInfo("KONVEYOR_LOG: Results size: " + requestor.getSymbols().size()); - - String result = requestor.getSymbols().stream() - .map(si -> - String.format("\n-------------------------\nSymbol name: %s\nkind: %s\nLocation: %s",si.getName(), si.getKind(), si.getLocation()) - ) - .collect(Collectors.joining()); - logInfo("KONVEYOR_LOG: " + result); + logInfo("KONVEYOR_LOG: Results size: " + requestor.getSymbols().size()); + + if (JavaLanguageServerPlugin.isLogLevelEnabled(Level.INFO)) { + String result = requestor.getSymbols().stream() + .map(si -> + String.format("\n-------------------------\nSymbol name: %s\nkind: %s\nLocation: %s",si.getName(), si.getKind(), si.getLocation()) + ) + .collect(Collectors.joining()); + logInfo("KONVEYOR_LOG: " + result); + }java-analyzer-bundle.test/projects/maven/springboot-todo/src/main/resources/templates/home.html (1)
121-142: Consider moving inline script to external file.The
deleteTaskfunction is defined inline while also loading an external/js/home.js. For better maintainability and separation of concerns, consider moving this function to the external JavaScript file.java-analyzer-bundle.test/projects/maven/springboot-todo/src/main/java/com/todo/app/controller/TaskController.java (2)
24-25: Prefer constructor injection over field injection.Spring best practices recommend constructor injection for better testability and immutability.
Apply this diff:
+ private final TaskService taskService; + + public TaskController(TaskService taskService) { + this.taskService = taskService; + } - @Autowired - private TaskService taskService;
85-89: Use HTTP 204 No Content for successful DELETE and handle missing tasks.For a DELETE operation with no response body, HTTP 204 (No Content) is more semantically correct than 200 (OK). Also, consider handling the case where the task doesn't exist.
@DeleteMapping("/home/{taskId}") public ResponseEntity<Void> deleteTask(@PathVariable Long taskId) { taskService.deleteTask(taskId); - return ResponseEntity.ok().build(); + return ResponseEntity.noContent().build(); }If
TaskService.deleteTaskcan indicate whether the task existed, also handle 404:@DeleteMapping("/home/{taskId}") public ResponseEntity<Void> deleteTask(@PathVariable Long taskId) { boolean deleted = taskService.deleteTask(taskId); if (!deleted) { return ResponseEntity.notFound().build(); } return ResponseEntity.noContent().build(); }java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/test/JavaUtils.java (1)
54-56: Document whywaitForBackgroundJobsis a no-op.The empty
waitForBackgroundJobsmethod is called after project creation (line 50) but performs no synchronization. This could lead to race conditions in tests if background jobs haven't completed.Consider either:
- Implementing the wait logic using
JobHelpers.waitForJobsToComplete(similar toProjectUtilsTest)- Adding a comment explaining why the wait is unnecessary in this context
- Removing the method call if it's truly not needed
private static void waitForBackgroundJobs() { - + // No-op: Background job synchronization is handled by ProjectUtilsTest.waitForBackgroundJobs + // when this utility is used within the test framework }java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/test/ProjectUtilsTest.java (2)
80-90: Improve error handling in test cleanup.The
cleanWorkspacemethod silently swallows exceptions, which could mask cleanup failures and cause subsequent test failures due to leftover state.@After public void cleanWorkspace() { try { IProject[] projects = ResourcesPlugin.getWorkspace().getRoot().getProjects(); for (IProject project : projects) { project.delete(true, null); } } catch (Exception e) { - e.printStackTrace(); + LOGGER.log(Level.SEVERE, "Failed to clean workspace", e); + // Consider: throw new RuntimeException("Workspace cleanup failed", e); } }
106-146: Propagate exceptions instead of returning empty array.Lines 138-140 catch and print exceptions but return an empty array, which could cause confusing test failures. Tests expecting projects would receive an empty result without understanding why.
public static IJavaProject[] loadJavaProjects(String[] parentSlashName) { List<IPath> paths = new ArrayList<>(); List<IJavaProject> javaProjects = new ArrayList<>(); try { // ... project loading logic ... } catch (Exception e) { - e.printStackTrace(); + LOGGER.log(Level.SEVERE, "Failed to load Java projects", e); + throw new RuntimeException("Failed to load Java projects", e); } // Set the rootPaths manually. This is is needed when running the tests JavaLanguageServerPlugin.getPreferencesManager().getPreferences().setRootPaths(paths); return javaProjects.toArray(new IJavaProject[0]); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
java-analyzer-bundle.test/projects/maven/springboot-todo/src/main/resources/static/favicon.icois excluded by!**/*.ico
📒 Files selected for processing (32)
.vscode/settings.json(1 hunks)java-analyzer-bundle.core/.classpath(1 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java(4 hunks)java-analyzer-bundle.test/.classpath(1 hunks)java-analyzer-bundle.test/META-INF/MANIFEST.MF(1 hunks)java-analyzer-bundle.test/projects/maven/springboot-todo/.project(1 hunks)java-analyzer-bundle.test/projects/maven/springboot-todo/README.md(1 hunks)java-analyzer-bundle.test/projects/maven/springboot-todo/mvnw(1 hunks)java-analyzer-bundle.test/projects/maven/springboot-todo/mvnw.cmd(1 hunks)java-analyzer-bundle.test/projects/maven/springboot-todo/pom.xml(1 hunks)java-analyzer-bundle.test/projects/maven/springboot-todo/src/README.md(1 hunks)java-analyzer-bundle.test/projects/maven/springboot-todo/src/main/java/com/todo/app/AppApplication.java(1 hunks)java-analyzer-bundle.test/projects/maven/springboot-todo/src/main/java/com/todo/app/controller/TaskController.java(1 hunks)java-analyzer-bundle.test/projects/maven/springboot-todo/src/main/java/com/todo/app/entity/Task.java(1 hunks)java-analyzer-bundle.test/projects/maven/springboot-todo/src/main/java/com/todo/app/repository/TaskRepository.java(1 hunks)java-analyzer-bundle.test/projects/maven/springboot-todo/src/main/java/com/todo/app/service/TaskService.java(1 hunks)java-analyzer-bundle.test/projects/maven/springboot-todo/src/main/java/com/todo/app/service/TaskServiceImpl.java(1 hunks)java-analyzer-bundle.test/projects/maven/springboot-todo/src/main/resources/application.properties(1 hunks)java-analyzer-bundle.test/projects/maven/springboot-todo/src/main/resources/static/js/home.js(1 hunks)java-analyzer-bundle.test/projects/maven/springboot-todo/src/main/resources/templates/error.html(1 hunks)java-analyzer-bundle.test/projects/maven/springboot-todo/src/main/resources/templates/home.html(1 hunks)java-analyzer-bundle.test/projects/maven/springboot-todo/src/mvnw(1 hunks)java-analyzer-bundle.test/projects/maven/springboot-todo/src/mvnw.cmd(1 hunks)java-analyzer-bundle.test/projects/maven/springboot-todo/src/pom.xml(1 hunks)java-analyzer-bundle.test/projects/maven/springboot-todo/src/test/java/com/todo/app/AppApplicationTests.java(1 hunks)java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/CommandHandlerTest.java(1 hunks)java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandlerTest.java(0 hunks)java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/test/JavaAnnotationTest.java(1 hunks)java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/test/JavaUtils.java(1 hunks)java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/test/JobHelpers.java(1 hunks)java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/test/PomDependencyTest.java(1 hunks)java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/test/ProjectUtilsTest.java(1 hunks)
💤 Files with no reviewable changes (1)
- java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandlerTest.java
✅ Files skipped from review due to trivial changes (1)
- java-analyzer-bundle.test/projects/maven/springboot-todo/.project
🚧 Files skipped from review as they are similar to previous changes (1)
- java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/test/JobHelpers.java
🧰 Additional context used
🧬 Code graph analysis (3)
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/test/JavaAnnotationTest.java (2)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/RuleEntryParams.java (1)
RuleEntryParams(11-77)java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/test/ProjectUtilsTest.java (1)
MavenProjectName(54-57)
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/CommandHandlerTest.java (1)
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/test/ProjectUtilsTest.java (1)
ProjectUtilsTest(45-248)
java-analyzer-bundle.test/projects/maven/springboot-todo/src/main/java/com/todo/app/entity/Task.java (1)
java-analyzer-bundle.test/projects/maven/springboot-todo/src/main/resources/static/js/home.js (3)
title(7-7)description(8-8)dueDate(9-9)
🪛 LanguageTool
java-analyzer-bundle.test/projects/maven/springboot-todo/README.md
[grammar] ~65-~65: Ensure spelling is correct
Context: ...ion.properties` file with your database jdtLSConfiguration such as username, password, database na...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~112-~112: Consider using a less common alternative to make your writing sound more unique and professional.
Context: ...ject is licensed under the MIT License. Feel free to use it, modify it, and share it as you ...
(FEEL_FREE_TO_STYLE_ME)
java-analyzer-bundle.test/projects/maven/springboot-todo/src/README.md
[grammar] ~65-~65: Ensure spelling is correct
Context: ...ion.properties` file with your database jdtLSConfiguration such as username, password, database na...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~112-~112: Consider using a less common alternative to make your writing sound more unique and professional.
Context: ...ject is licensed under the MIT License. Feel free to use it, modify it, and share it as you ...
(FEEL_FREE_TO_STYLE_ME)
🪛 OSV Scanner (2.2.4)
java-analyzer-bundle.test/projects/maven/springboot-todo/pom.xml
[CRITICAL] 1-1: org.apache.tomcat.embed:tomcat-embed-core 10.1.42: Apache Tomcat Improper Resource Shutdown or Release vulnerability
[CRITICAL] 1-1: org.apache.tomcat.embed:tomcat-embed-core 10.1.42: Apache Tomcat Vulnerable to Improper Resource Shutdown or Release
[CRITICAL] 1-1: org.apache.tomcat.embed:tomcat-embed-core 10.1.42: Apache Tomcat Vulnerable to Improper Neutralization of Escape, Meta, or Control Sequences
[CRITICAL] 1-1: org.apache.tomcat.embed:tomcat-embed-core 10.1.42: Apache Tomcat Vulnerable to Relative Path Traversal
[HIGH] 1-1: org.springframework:spring-core 6.2.8: Spring Framework annotation detection mechanism may result in improper authorization
java-analyzer-bundle.test/projects/maven/springboot-todo/src/pom.xml
[CRITICAL] 1-1: org.apache.tomcat.embed:tomcat-embed-core 10.1.42: Apache Tomcat Improper Resource Shutdown or Release vulnerability
[CRITICAL] 1-1: org.apache.tomcat.embed:tomcat-embed-core 10.1.42: Apache Tomcat Vulnerable to Improper Resource Shutdown or Release
[CRITICAL] 1-1: org.apache.tomcat.embed:tomcat-embed-core 10.1.42: Apache Tomcat Vulnerable to Improper Neutralization of Escape, Meta, or Control Sequences
[CRITICAL] 1-1: org.apache.tomcat.embed:tomcat-embed-core 10.1.42: Apache Tomcat Vulnerable to Relative Path Traversal
[HIGH] 1-1: org.springframework:spring-core 6.2.8: Spring Framework annotation detection mechanism may result in improper authorization
🔇 Additional comments (14)
.vscode/settings.json (1)
2-3: Configuration looks good for testing workflows.The addition of
"java.debug.settings.onBuildFailureProceed": trueallows the debugger to proceed when build failures occur, which is sensible for test scenarios. The JSON syntax is valid, and the setting aligns well with the PR's test infrastructure objectives.java-analyzer-bundle.test/projects/maven/springboot-todo/src/README.md (1)
70-72: Verify jar artifact name matches pom.xml output.The build instructions assume a jar named
awesome-todo-app-0.1.jar. Ensure this matches the actual artifact name configured in the pom.xml for this project.java-analyzer-bundle.test/projects/maven/springboot-todo/src/pom.xml (1)
50-53: Clarify whether Spring Security should be enabled.Spring Security is commented out, but jjwt (JWT library) is included. If this sample app is meant to demonstrate JWT-based authentication, Spring Security should likely be enabled. Please confirm whether this is intentional or if the dependency should be uncommented.
java-analyzer-bundle.test/projects/maven/springboot-todo/src/test/java/com/todo/app/AppApplicationTests.java (1)
1-13: LGTM! Standard Spring Boot smoke test.This is a standard Spring Boot test class with a context-loading smoke test, which verifies that the application context starts without errors. This pattern is appropriate for baseline application health checks.
java-analyzer-bundle.test/projects/maven/springboot-todo/pom.xml (1)
50-53: Clarify security implementation strategy.Spring Security is commented out while JWT authentication dependencies are included. This is an unusual pattern—JWT is typically used with Spring Security for authentication/authorization. Please clarify whether:
- This is intentional for the test project scope, or
- Security implementation is incomplete
If security is out of scope for this test project, consider removing the JWT dependency as well.
java-analyzer-bundle.test/projects/maven/springboot-todo/src/main/java/com/todo/app/repository/TaskRepository.java (1)
1-8: LGTM! Standard Spring Data JPA repository.This is a clean, standard Spring Data JPA repository interface. It correctly extends
JpaRepository<Task, Long>, which provides automatic CRUD operations without requiring custom implementation. This follows Spring Data best practices.java-analyzer-bundle.test/projects/maven/springboot-todo/src/main/java/com/todo/app/AppApplication.java (1)
1-13: LGTM!Standard Spring Boot application entry point. The implementation is correct and follows Spring Boot conventions.
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/test/PomDependencyTest.java (1)
11-17: LGTM!The test now includes a proper assertion that verifies the pom.xml path, as recommended in the previous review. The fix has been correctly applied.
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/test/JavaAnnotationTest.java (1)
26-29: Document workaround for known issue.The test works around issue #175 by using a shortened query. Consider adding a TODO or tracking this as a known limitation.
This workaround may cause the test to pass even if the full qualified name matching is broken. Once issue #175 is resolved, update this test to use the fully qualified annotation name and verify it returns only one symbol.
java-analyzer-bundle.test/projects/maven/springboot-todo/src/main/java/com/todo/app/entity/Task.java (1)
8-64: LGTM!Standard JPA entity implementation with appropriate annotations and accessor methods. Suitable for a test/sample project.
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java (1)
203-204: LGTM! Visibility change supports the test infrastructure.Making the
searchmethod public enables tests to call it directly without JSON-RPC overhead, which aligns with the PR objectives to simplify test setup.java-analyzer-bundle.test/projects/maven/springboot-todo/src/main/java/com/todo/app/controller/TaskController.java (1)
44-73: LGTM! Pagination logic handles edge cases appropriately.The pagination implementation correctly handles empty results, out-of-bounds page numbers, and redirects appropriately. The redirect logic when
tasks.isEmpty()andpageNo > 1(lines 53-57) appropriately redirects to the previous page.java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/test/JavaUtils.java (1)
94-94: LGTM! NPE issues resolved.The classpath entry construction now correctly uses
source.getFullPath()andnew Path(jar)directly, avoiding the NPE that would occur withgetPackageFragmentRoot()on entries not yet in the classpath.Based on learnings (past review comments).
Also applies to: 101-101
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/test/ProjectUtilsTest.java (1)
197-208: LGTM! File timestamp handling is appropriate.The 1-second sleep is necessary to ensure file modification timestamps are distinct on macOS and Linux file systems, which have 1-second granularity for
getLastModifiedTime. TheInterruptedExceptionis properly handled by restoring the interrupt flag.
| <!--<classpathentry kind="con" path="org.eclipse.jdt.launching.JRE_CONTAINER/org.eclipse.jdt.internal.debug.ui.launcher.StandardVMType/JavaSE-17"/>--> | ||
| <classpathentry kind="con" path="org.eclipse.jdt.launching.JRE_CONTAINER"/> |
There was a problem hiding this comment.
🧩 Analysis chain
Clarify Java version strategy and remove commented code.
The change from a pinned JavaSE-17 container to a generic JRE_CONTAINER introduces version ambiguity. Additionally:
- The commented-out line should be removed rather than left in place.
- The MANIFEST.MF still declares
Bundle-RequiredExecutionEnvironment: JavaSE-17, creating an inconsistency.
Please clarify the intended Java version strategy: if JavaSE-17 is required (as the manifest suggests), restore the pinned container; otherwise, update the manifest to match the generic approach.
- <!--<classpathentry kind="con" path="org.eclipse.jdt.launching.JRE_CONTAINER/org.eclipse.jdt.internal.debug.ui.launcher.StandardVMType/JavaSE-17"/>-->
- <classpathentry kind="con" path="org.eclipse.jdt.launching.JRE_CONTAINER"/>
+ <classpathentry kind="con" path="org.eclipse.jdt.launching.JRE_CONTAINER/org.eclipse.jdt.internal.debug.ui.launcher.StandardVMType/JavaSE-17"/>I'll verify the review comment by examining the relevant configuration files.
🏁 Script executed:
#!/bin/bash
# Search for MANIFEST.MF files in the repository, particularly in the test bundle
fd -t f "MANIFEST.MF" | head -20Length of output: 93
🏁 Script executed:
#!/bin/bash
# Find and examine the .classpath file being reviewed
find . -path "*/java-analyzer-bundle.test/.classpath" -type fLength of output: 38
🏁 Script executed:
#!/bin/bash
# Look at the .classpath file content to confirm current state
if [ -f "java-analyzer-bundle.test/.classpath" ]; then
cat "java-analyzer-bundle.test/.classpath"
fiLength of output: 564
🏁 Script executed:
#!/bin/bash
# Search for MANIFEST.MF in or near the java-analyzer-bundle.test directory
fd -t f "MANIFEST.MF" | xargs -I {} sh -c 'echo "=== {} ===" && head -20 "{}"'Length of output: 1107
Remove commented code and align .classpath with manifest Java version requirement.
Verification confirms the review concerns are legitimate:
- Inconsistency verified:
java-analyzer-bundle.test/META-INF/MANIFEST.MFdeclaresBundle-RequiredExecutionEnvironment: JavaSE-17, but.classpathline 4 uses genericJRE_CONTAINERwithout version pinning. - Commented code confirmed: Line 3 contains the commented JavaSE-17 pinned entry and should be removed.
- Version mismatch confirmed: The manifest requirement (JavaSE-17) conflicts with the classpath configuration.
Either restore the pinned JavaSE-17 container on line 4 to match the manifest, or update the manifest to reflect the generic approach. Remove the commented line regardless.
🤖 Prompt for AI Agents
In java-analyzer-bundle.test/.classpath around lines 3 to 4, remove the
commented out JavaSE-17 entry and make the active classpath entry explicitly pin
to JavaSE-17 to match META-INF/MANIFEST.MF's
Bundle-RequiredExecutionEnvironment: JavaSE-17; specifically delete the
commented line and replace the generic <classpathentry kind="con"
path="org.eclipse.jdt.launching.JRE_CONTAINER"/> with the pinned container path
for JavaSE-17
(org.eclipse.jdt.launching.JRE_CONTAINER/org.eclipse.jdt.internal.debug.ui.launcher.StandardVMType/JavaSE-17),
or alternately update the MANIFEST.MF to reflect the generic container if you
prefer that approach.
| <parent> | ||
| <groupId>org.springframework.boot</groupId> | ||
| <artifactId>spring-boot-starter-parent</artifactId> | ||
| <version>3.5.3</version> |
There was a problem hiding this comment.
🧩 Analysis chain
Update Spring Boot version to address critical security vulnerabilities.
Static analysis detected multiple CRITICAL vulnerabilities in the transitive Tomcat dependency (version 10.1.42) from Spring Boot 3.5.3:
- GHSA-gqp3-2cvr-x8m3: Improper Resource Shutdown
- GHSA-hgrr-935x-pq79: Improper Resource Shutdown
- GHSA-vfww-5hm6-hx2j: Improper Neutralization of Escape Sequences
- GHSA-wmwf-9ccg-fff5: Relative Path Traversal
Additionally, a HIGH severity vulnerability in Spring Core (GHSA-jmp9-x22r-554x) affects authorization.
Even though this is a test project, it should follow security best practices. Please verify if a newer Spring Boot version addresses these vulnerabilities.
🌐 Web query:
What is the latest stable version of Spring Boot 3.x and does it fix Tomcat vulnerabilities GHSA-gqp3-2cvr-x8m3, GHSA-hgrr-935x-pq79, GHSA-vfww-5hm6-hx2j, GHSA-wmwf-9ccg-fff5?
💡 Result:
Short answer
- Latest stable Spring Boot 3.x (GA) is 3.5.3 (release June 19–20, 2025). [1]
- That release does not universally address the Tomcat GHSA issues you listed — the Tomcat "Made You Reset" vulnerability (GHSA-gqp3-2cvr-x8m3 / CVE-2025-48989) was fixed in Tomcat 9.0.108, 10.1.44 and 11.0.10, but Spring Boot 3.5.x releases around June 2025 still pull older embedded Tomcat versions and several 3.5.* starters are flagged as containing CVE-2025-48989. You will need either a Spring Boot release that updates the managed Tomcat to 10.1.44+/9.0.108+/11.0.10+ or explicitly override the tomcat.version in your build. [2][3][4]
Sources
- Spring Boot 3.5.3 release announcement. [1]
- GHSA / CVE details and fixed Tomcat versions (GHSA-gqp3-2cvr-x8m3 / CVE-2025-48989). [2]
- Evidence that Spring Boot 3.5.x starters were still pulling vulnerable tomcat-embed versions (scanner reports / advisories; example Snyk/Maven repo findings). [3][4]
If you want, I can:
- check the exact embedded Tomcat version used by your specific Spring Boot 3.5.* patch (3.5.0 → 3.5.3, 3.5.4, 3.5.5 etc.), or
- show how to override the tomcat.version in Maven/Gradle to force a patched Tomcat immediately.
Spring Boot 3.5.3 is the latest stable version but still contains vulnerable Tomcat—explicit override required.
Spring Boot 3.5.3 (released June 19–20, 2025) is the latest stable 3.x GA release, so no newer version is available to upgrade to. However, the embedded Tomcat versions in Spring Boot 3.5.x are still outdated; the Tomcat vulnerability GHSA-gqp3-2cvr-x8m3 (CVE-2025-48989) was fixed in Tomcat 10.1.44+, but Spring Boot 3.5.x starters around June 2025 still pull vulnerable versions.
To address the identified vulnerabilities, either:
- Explicitly override
tomcat.versionproperty in pom.xml to 10.1.44+ (or 9.0.108+/11.0.10+ depending on your Tomcat major version), or - Wait for a patched Spring Boot 3.5.x release that updates the managed Tomcat version.
Even for a test project, following this practice demonstrates secure dependency management.
🤖 Prompt for AI Agents
In java-analyzer-bundle.test/projects/maven/springboot-todo/pom.xml around line
8, the project uses Spring Boot 3.5.3 which still pulls a vulnerable Tomcat; add
an explicit tomcat.version property to override the managed Tomcat to a fixed
release (e.g., set tomcat.version to 10.1.44 or later for Tomcat 10.x, or to the
appropriate 9.0.108+/11.0.10+ value if using those majors), placing the property
in the <properties> section so Maven uses this version instead of the transitive
one.
| <dependency> | ||
| <groupId>io.jsonwebtoken</groupId> | ||
| <artifactId>jjwt</artifactId> | ||
| <version>0.9.1</version> | ||
| </dependency> |
There was a problem hiding this comment.
Update outdated JWT library.
The jjwt version 0.9.1 is severely outdated (released in 2017) and likely contains security vulnerabilities. The library has since moved to a modular structure with separate artifacts.
Replace with the latest version using the modular dependencies:
- <dependency>
- <groupId>io.jsonwebtoken</groupId>
- <artifactId>jjwt</artifactId>
- <version>0.9.1</version>
- </dependency>
+ <dependency>
+ <groupId>io.jsonwebtoken</groupId>
+ <artifactId>jjwt-api</artifactId>
+ <version>0.12.6</version>
+ </dependency>
+ <dependency>
+ <groupId>io.jsonwebtoken</groupId>
+ <artifactId>jjwt-impl</artifactId>
+ <version>0.12.6</version>
+ <scope>runtime</scope>
+ </dependency>
+ <dependency>
+ <groupId>io.jsonwebtoken</groupId>
+ <artifactId>jjwt-jackson</artifactId>
+ <version>0.12.6</version>
+ <scope>runtime</scope>
+ </dependency>📝 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.
| <dependency> | |
| <groupId>io.jsonwebtoken</groupId> | |
| <artifactId>jjwt</artifactId> | |
| <version>0.9.1</version> | |
| </dependency> | |
| <dependency> | |
| <groupId>io.jsonwebtoken</groupId> | |
| <artifactId>jjwt-api</artifactId> | |
| <version>0.12.6</version> | |
| </dependency> | |
| <dependency> | |
| <groupId>io.jsonwebtoken</groupId> | |
| <artifactId>jjwt-impl</artifactId> | |
| <version>0.12.6</version> | |
| <scope>runtime</scope> | |
| </dependency> | |
| <dependency> | |
| <groupId>io.jsonwebtoken</groupId> | |
| <artifactId>jjwt-jackson</artifactId> | |
| <version>0.12.6</version> | |
| <scope>runtime</scope> | |
| </dependency> |
🤖 Prompt for AI Agents
In java-analyzer-bundle.test/projects/maven/springboot-todo/pom.xml around lines
54-58, the project depends on the old monolithic io.jsonwebtoken:jjwt:0.9.1;
replace it with the modern modular artifacts (jjwt-api, jjwt-impl, jjwt-jackson)
and a current stable version (e.g. 0.11.5): add jjwt-api as the compile
dependency, add jjwt-impl and jjwt-jackson as runtime (or runtimeOnly)
dependencies so the API stays compile-time but the implementation and Jackson
serializer are provided at runtime, and ensure any necessary exclusions/versions
align (or use the jjwt BOM if preferred) so the modular artifacts resolve
correctly.
| 2. **Database Configuration:** | ||
|
|
||
| - Install MySQL and create a database. | ||
| - Update the `application.properties` file with your database jdtLSConfiguration such as username, password, database name and port number. |
There was a problem hiding this comment.
Fix typo in documentation.
The word "jdtLSConfiguration" should be "configuration". This appears to be a variable name accidentally left in the documentation.
Apply this diff:
- - Update the `application.properties` file with your database jdtLSConfiguration such as username, password, database name and port number.
+ - Update the `application.properties` file with your database configuration such as username, password, database name and port number.📝 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.
| - Update the `application.properties` file with your database jdtLSConfiguration such as username, password, database name and port number. | |
| - Update the `application.properties` file with your database configuration such as username, password, database name and port number. |
🧰 Tools
🪛 LanguageTool
[grammar] ~65-~65: Ensure spelling is correct
Context: ...ion.properties` file with your database jdtLSConfiguration such as username, password, database na...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
In java-analyzer-bundle.test/projects/maven/springboot-todo/README.md around
line 65, the documentation contains a typo: replace the word
"jdtLSConfiguration" with "configuration" in the sentence so it reads "Update
the `application.properties` file with your database configuration such as
username, password, database name and port number." Ensure only that token is
changed and punctuation/formatting remains intact.
| @PostMapping("/home") | ||
| @ResponseBody | ||
| public ResponseEntity<List<Task>> createTask(@RequestBody Task task) { | ||
| taskService.addTask(task); | ||
| List<Task> tasks = taskService.getAllTasks(); | ||
| return ResponseEntity.ok(tasks); | ||
| } |
There was a problem hiding this comment.
Reconsider returning the full task list.
Returning all tasks after creating a single task is inefficient, especially as the dataset grows. The client already has the existing tasks.
Consider these alternatives:
- Return only the created task with HTTP 201 Created
- Return a paginated response if pagination is required
- Let the client append the new task to its local list
@PostMapping("/home")
@ResponseBody
- public ResponseEntity<List<Task>> createTask(@RequestBody Task task) {
+ public ResponseEntity<Task> createTask(@RequestBody @Valid Task task) {
- taskService.addTask(task);
- List<Task> tasks = taskService.getAllTasks();
- return ResponseEntity.ok(tasks);
+ Task savedTask = taskService.addTask(task);
+ return ResponseEntity.status(HttpStatus.CREATED).body(savedTask);
}Note: Also added @Valid for input validation.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
java-analyzer-bundle.test/projects/maven/springboot-todo/src/main/java/com/todo/app/controller/TaskController.java
around lines 76-82, do not return the full task list after creating a task;
instead annotate the request body with @Valid, have the service return/persist
and return only the created Task, and respond with HTTP 201 Created (preferably
include a Location header pointing to the new resource). Change the method
signature to return ResponseEntity<Task>, use @Valid on @RequestBody, call the
service to save and obtain the saved Task (with id), then return
ResponseEntity.created(new URI("/tasks/" + id)).body(savedTask) or
ResponseEntity.status(201).body(savedTask); if pagination is required elsewhere,
handle it in the list endpoint rather than here.
| <dependency> | ||
| <groupId>io.jsonwebtoken</groupId> | ||
| <artifactId>jjwt</artifactId> | ||
| <version>0.9.1</version> | ||
| </dependency> |
There was a problem hiding this comment.
Upgrade jjwt to a supported version; fix indentation.
jjwt 0.9.1 is severely outdated (~2017, unmaintained for years) and likely harbors known CVEs. Current stable versions (0.12.3 or 1.x for JWT 4.0 compliance) include critical security patches.
Apply this diff to upgrade jjwt and fix the inconsistent indentation:
- <!-- <dependency>
- <groupId>org.springframework.boot</groupId>
- <artifactId>spring-boot-starter-security</artifactId>
- </dependency> -->
- <dependency>
- <groupId>io.jsonwebtoken</groupId>
- <artifactId>jjwt</artifactId>
- <version>0.9.1</version>
+ <!-- <dependency>
+ <groupId>org.springframework.boot</groupId>
+ <artifactId>spring-boot-starter-security</artifactId>
+ </dependency> -->
+ <dependency>
+ <groupId>io.jsonwebtoken</groupId>
+ <artifactId>jjwt-api</artifactId>
+ <version>0.12.3</version>
+ </dependency>
+ <dependency>
+ <groupId>io.jsonwebtoken</groupId>
+ <artifactId>jjwt-impl</artifactId>
+ <version>0.12.3</version>
+ <scope>runtime</scope>
+ </dependency>
+ <dependency>
+ <groupId>io.jsonwebtoken</groupId>
+ <artifactId>jjwt-jackson</artifactId>
+ <version>0.12.3</version>
+ <scope>runtime</scope>Note: Modern jjwt (0.12.x+) is split into API, implementation, and Jackson modules. Adjust version as needed based on your JWT specification requirements.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In java-analyzer-bundle.test/projects/maven/springboot-todo/src/pom.xml around
lines 54-58, the pom uses the old io.jsonwebtoken:jjwt 0.9.1 and has
inconsistent indentation; replace the single legacy jjwt dependency with current
modular artifacts (e.g., io.jsonwebtoken:jjwt-api, io.jsonwebtoken:jjwt-impl,
io.jsonwebtoken:jjwt-jackson) pinned to a supported version such as 0.12.3 (or a
chosen 1.x line), mark jjwt-impl and jjwt-jackson with the appropriate scope
(runtime) per jjwt docs, and fix indentation to match surrounding XML
formatting.
| 2. **Database Configuration:** | ||
|
|
||
| - Install MySQL and create a database. | ||
| - Update the `application.properties` file with your database jdtLSConfiguration such as username, password, database name and port number. |
There was a problem hiding this comment.
Fix typo in database configuration instructions.
Line 65 contains "jdtLSConfiguration" which appears to be a typo. Based on context, this should read "configuration" or "credentials."
Apply this diff:
- - Update the `application.properties` file with your database jdtLSConfiguration such as username, password, database name and port number.
+ - Update the `application.properties` file with your database configuration such as username, password, database name and port number.📝 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.
| - Update the `application.properties` file with your database jdtLSConfiguration such as username, password, database name and port number. | |
| - Update the `application.properties` file with your database configuration such as username, password, database name and port number. |
🧰 Tools
🪛 LanguageTool
[grammar] ~65-~65: Ensure spelling is correct
Context: ...ion.properties` file with your database jdtLSConfiguration such as username, password, database na...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
In java-analyzer-bundle.test/projects/maven/springboot-todo/src/README.md around
line 65, the word "jdtLSConfiguration" is a typo; replace it with
"configuration" (or "credentials" if you prefer) so the sentence reads e.g.
"Update the `application.properties` file with your database configuration such
as username, password, database name and port number." Keep the rest of the
sentence intact and ensure grammar and punctuation remain correct.
| public void ruleEntryCmdShouldReturnEmptyListAsProjectIsNull() throws Exception { | ||
| List<Object> params = new ArrayList<Object>(); | ||
| Map<String, String> param = Map.of( | ||
| "project", "project", | ||
| "query", "customresourcedefinition", | ||
| "location","0", | ||
| "analysisMode", ANALYSIS_MODE_SOURCE_ONLY); | ||
| params.add(param); | ||
|
|
||
| var result = cmdHandler.executeCommand(SampleDelegateCommandHandler.RULE_ENTRY_COMMAND_ID, params, new NullProgressMonitor()); | ||
| assertEquals(Collections.emptyList(), result); | ||
| } |
There was a problem hiding this comment.
Test name and implementation are inconsistent.
The test is named ruleEntryCmdShouldReturnEmptyListAsProjectIsNull, but the project parameter is set to the string "project" rather than null. The test likely returns an empty list because the project string doesn't match any actual loaded project, not because it's null.
Consider either:
- Renaming the test to
ruleEntryCmdShouldReturnEmptyListWhenProjectNotFound, or - Changing the parameter to actually test null handling:
"project", null
🤖 Prompt for AI Agents
In
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/CommandHandlerTest.java
around lines 28 to 39, the test name says project is null but the param sets
"project" to the string "project"; fix by making the test behavior match its
name: either rename the test to
ruleEntryCmdShouldReturnEmptyListWhenProjectNotFound if you intend to pass a
non-null missing project, or change the param to use a null project value (i.e.
set "project" to null) so the test actually exercises the null-project branch;
update the assertion message or test name accordingly to keep intent clear.
| params.getMavenIndexPath(), new NullProgressMonitor()); | ||
| Assert.assertNotNull(results); | ||
|
|
||
| // Search within the results the symbol matching thge annotation to search |
There was a problem hiding this comment.
Fix typo in comment.
"thge" should be "the".
- // Search within the results the symbol matching thge annotation to search
+ // Search within the results the symbol matching the annotation to search📝 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.
| // Search within the results the symbol matching thge annotation to search | |
| // Search within the results the symbol matching the annotation to search |
🤖 Prompt for AI Agents
In
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/test/JavaAnnotationTest.java
around line 48, fix the typo in the inline comment by replacing "thge" with
"the" so the comment reads "Search within the results the symbol matching the
annotation to search".
| .filter(symbol -> symbol.getName().equals(targetAnnotation)) | ||
| .findFirst(); | ||
|
|
||
| Assert.assertNotNull(foundSymbol.get()); |
There was a problem hiding this comment.
Check Optional before calling get().
Calling get() on an Optional without checking isPresent() first will throw NoSuchElementException if the symbol isn't found. Use assertTrue with isPresent() first to provide a better error message.
Apply this diff:
- Assert.assertNotNull(foundSymbol.get());
+ Assert.assertTrue("ResponseBody annotation not found in results", foundSymbol.isPresent());
+
+ SymbolInformation symbol = foundSymbol.get();
+ Assert.assertNotNull(symbol);
- Location loc = foundSymbol.get().getLocation();
+ Location loc = symbol.getLocation();Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/test/JavaAnnotationTest.java
around line 54, the test calls foundSymbol.get() without checking the Optional,
which can throw NoSuchElementException; replace the direct get assertion with an
assertion that the Optional is present first (e.g.,
assertTrue(foundSymbol.isPresent(), "Expected symbol to be present")), and then
if needed assert on foundSymbol.get() after that presence check.
jmle
left a comment
jmle
left a comment
There was a problem hiding this comment.
Thanks @cmoulliard, it looks good. The only thing I would like to ask you to do is to move the new files to follow a more typical maven organization:
src
|- main
|- java
|- resources
|- test
|- java <- let's move the test code here
|- resources <- let's move the test data here
According to eclipse jdt-ls engineers, as the project The alternative is perhaps to change the surefire plugin as documented here: https://tycho.eclipseprojects.io/doc/main/TestingBundles.html OR to change build.properties et .classpath. To be investigated ! |
|
I am plus one to merging this and #165 because I think it won't be a bad thing to have both types of coverage. We should come back once they are both merged and consider having a shared set of test projects, having a shared set of "expected" output. I don't want to to have to update expected output twice in two different ways forever if we add or change something IMO. |
shawn-hurley
left a comment
shawn-hurley
left a comment
There was a problem hiding this comment.
the comments are minor and can be follow-ups if we need, I will defer to the @jmle to merge when he is ready based on his review.
...bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java
Outdated
Show resolved
Hide resolved
...bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java
Outdated
Show resolved
Hide resolved
|
@cmoulliard I am looking into the e2e failure now, it seems to be pervasive for the repo, I will re-run the CI once it is good. |
… package as SampleDelegateCommandHandler Signed-off-by: Charles Moulliard <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/JavaAnnotationTest.java (2)
46-46: Fix typo in comment.The comment contains "thge" which should be "the".
This issue was already identified in a previous review.
52-54: Unsafe Optional access without presence check.Calling
foundSymbol.get()without first checkingisPresent()will throwNoSuchElementExceptionif the symbol isn't found.This issue was already identified in a previous review. Apply this fix:
- Assert.assertNotNull(foundSymbol.get()); + Assert.assertTrue("ResponseBody annotation not found in results", foundSymbol.isPresent()); + SymbolInformation symbol = foundSymbol.get(); + Assert.assertNotNull(symbol); - Location loc = foundSymbol.get().getLocation(); + Location loc = symbol.getLocation();java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java (1)
403-414: Consider conditional logging to prevent log bloat.The new logging outputs detailed information for every symbol in the results. For queries returning many results, this could bloat log files significantly.
A previous review suggested: "if size > then say 50(??) just output the size and if it is less then output using the stream"
Consider implementing conditional logging:
-logInfo("KONVEYOR_LOG: Got: " + requestor.getAllSearchMatches() + - " Number of search matching the query: \"" + query + "\"" + - " and location type: " + location); - -logInfo("KONVEYOR_LOG: Results size: " + requestor.getSymbols().size()); - -String result = requestor.getSymbols().stream() - .map(si -> - String.format("\n-------------------------\nSymbol name: %s\nkind: %s\nLocation: %s",si.getName(), si.getKind(), si.getLocation()) - ) - .collect(Collectors.joining()); -logInfo("KONVEYOR_LOG: " + result); +int resultSize = requestor.getSymbols().size(); +logInfo("KONVEYOR_LOG: Got: " + requestor.getAllSearchMatches() + + " Number of search matching the query: \"" + query + "\"" + + " and location type: " + location + + " Results size: " + resultSize); + +if (resultSize > 0 && resultSize <= 50) { + String result = requestor.getSymbols().stream() + .map(si -> + String.format("\n-------------------------\nSymbol name: %s\nkind: %s\nLocation: %s", + si.getName(), si.getKind(), si.getLocation()) + ) + .collect(Collectors.joining()); + logInfo("KONVEYOR_LOG: Detailed results: " + result); +}java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/CommandHandlerTest.java (1)
25-36: Test name doesn't match implementation.The test is named
ruleEntryCmdShouldReturnEmptyListAsProjectIsNull, but the project parameter is set to the string"project"(line 28) rather thannull. The test returns an empty list because the project doesn't exist in the workspace, not because it's null.This issue was already identified in a previous review. Consider either:
- Renaming the test to
ruleEntryCmdShouldReturnEmptyListWhenProjectNotFound, or- Changing line 28 to
"project", nullto actually test null handling
🧹 Nitpick comments (6)
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/ProjectUtilsTest.java (4)
52-60: Make project name constantsfinal.The static fields in
MavenProjectNameandGradleProjectNameshould befinalto prevent accidental reassignment and clearly indicate they are constants.Apply this diff:
public static class MavenProjectName { - public static String empty_maven_project = "empty-maven"; - public static String springboot_todo_project = "springboot-todo"; + public static final String empty_maven_project = "empty-maven"; + public static final String springboot_todo_project = "springboot-todo"; } public static class GradleProjectName { - public static String empty_gradle_project = "empty-gradle-project"; - public static String quarkus_gradle_project = "quarkus-gradle-project"; + public static final String empty_gradle_project = "empty-gradle-project"; + public static final String quarkus_gradle_project = "quarkus-gradle-project"; }
62-62: Consider test isolation concerns with static mutable field.The
public static SampleDelegateCommandHandler cmdHandleris mutable shared state that could affect test isolation if tests modify its state. While this may be acceptable for the current test setup, consider whether instance-level initialization would be more appropriate.
79-88: Error handling in cleanup may hide test failures.The
cleanWorkspace()method catches all exceptions and only prints the stack trace. If workspace cleanup fails, subsequent tests might be affected. Consider either propagating the exception or logging it more prominently.@After public void cleanWorkspace() { try { IProject[] projects = ResourcesPlugin.getWorkspace().getRoot().getProjects(); for (IProject project : projects) { project.delete(true, null); } } catch (Exception e) { - e.printStackTrace(); + LOGGER.log(Level.SEVERE, "Failed to clean workspace", e); + throw new RuntimeException("Workspace cleanup failed", e); } }
195-206: Document the magic number for file timestamp resolution.The 1050ms sleep duration (line 201) addresses file timestamp resolution on Mac OS and Linux (1-second granularity), but the hardcoded value should be a named constant for clarity.
+private static final int FILE_TIMESTAMP_RESOLUTION_MILLIS = 1050; + private static void updateFile(IFile file, String content) throws CoreException { // For Mac OS, Linux OS, the call of Files.getLastModifiedTime is working for 1 // second. // Here we wait for > 1s to be sure that call of Files.getLastModifiedTime will // work. try { - Thread.sleep(1050); + Thread.sleep(FILE_TIMESTAMP_RESOLUTION_MILLIS); } catch(InterruptedException e) { Thread.currentThread().interrupt(); } createFile(file, content); }java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/JavaAnnotationTest.java (1)
60-60: UseassertTruefor boolean assertions.Use
assertTrueinstead ofassertEquals(true, ...)for better readability and more descriptive failure messages.- Assert.assertEquals(true, loc.getUri().contains("TaskController.java")); + Assert.assertTrue("Expected URI to contain TaskController.java", loc.getUri().contains("TaskController.java"));java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/JavaUtils.java (1)
129-132: LGTM with optional suggestion.The method correctly constructs absolute JAR paths for classpath entries.
Optionally, validate that the JAR exists to provide earlier failure feedback:
public static String getJarPath(String jar) { java.nio.file.Path jarPath = Paths.get("jars", jar); + if (!java.nio.file.Files.exists(jarPath)) { + throw new IllegalArgumentException("JAR not found: " + jarPath.toAbsolutePath()); + } return jarPath.toAbsolutePath().toString(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java(5 hunks)java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/CommandHandlerTest.java(1 hunks)java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/JavaAnnotationTest.java(1 hunks)java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/JavaUtils.java(1 hunks)java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/JobHelpers.java(1 hunks)java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/PomDependencyTest.java(1 hunks)java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/ProjectUtilsTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/PomDependencyTest.java (1)
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/ProjectUtilsTest.java (1)
MavenProjectName(52-55)
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/JavaAnnotationTest.java (1)
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/ProjectUtilsTest.java (1)
MavenProjectName(52-55)
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/CommandHandlerTest.java (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java (1)
SampleDelegateCommandHandler(44-428)
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/ProjectUtilsTest.java (1)
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/JavaUtils.java (1)
JavaUtils(31-133)
🔇 Additional comments (6)
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/PomDependencyTest.java (1)
10-17: LGTM!The test correctly loads a Maven project and verifies the pom.xml file location. The assertion logic is sound and the test structure is appropriate.
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/JobHelpers.java (1)
22-214: LGTM!This utility class provides essential job synchronization functionality for tests, properly adapted from m2e's test infrastructure. The implementation correctly handles Eclipse workspace jobs, M2E background processing queues, and various job types (build, initialize, download sources). The timeout mechanisms and logging are appropriate for test diagnostics.
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/CommandHandlerTest.java (1)
19-22: LGTM!The test correctly verifies that the sample command returns "Hello World".
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java (1)
203-203: LGTM!Changing the
searchmethod fromprivatetoprotectedis appropriate to support direct testing without JSON-RPC, as described in the PR objectives.java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/JavaUtils.java (2)
116-120: LGTM!The method correctly creates and returns the working directory for test projects. Using
target/workingProjectsfollows Maven conventions and ensures test artifacts are in the standard build directory.
105-110: Do not make the suggested change — the reviewed method appears to be unused dead code.The public method
createJavaProject()at line 45 has no callers anywhere in the codebase. Tests useJavaLanguageServerPlugin.getProjectsManager().initializeProjects()instead (as shown in ProjectUtilsTest.java), not theJavaUtils.createJavaProject()utility. Since this method is not invoked, the commented-out JRE container has no impact on test compilation or functionality.Additionally, if the JRE line were uncommented, the code would fail to compile because
org.eclipse.jdt.launching.JavaRuntimeis not imported in the file.Likely an incorrect or invalid review comment.
| * "variable_declaration": 9, | ||
| * "type": 10, | ||
| * "package": 11, | ||
| * "package": 1 |
There was a problem hiding this comment.
Fix incorrect location code in comment.
The comment shows "package": 1 but based on the switch statement (line 191), package location should be 11, not 1. Location 1 is used for "inheritance" (line 178).
- * "package": 1
+ * "package": 11📝 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.
| * "package": 1 | |
| * "package": 11 |
🤖 Prompt for AI Agents
In
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java
around line 154, the inline comment incorrectly lists the location code as
"package": 1; update it to "package": 11 so it matches the switch cases
(inheritance uses 1 at ~line 178 and package uses 11 at ~line 191). Ensure the
comment value is changed from 1 to 11 and that any nearby documentation/comments
remain consistent with the switch mapping.
| private static void waitForBackgroundJobs() { | ||
|
|
||
| } |
There was a problem hiding this comment.
Implement job synchronization or verify if intentional.
The empty waitForBackgroundJobs() method is called after project creation but does nothing. Eclipse workspace operations (building, indexing) typically spawn background jobs that must complete before the project is ready for use. Without proper synchronization, tests may experience race conditions or access incomplete project state.
Consider implementing job synchronization:
private static void waitForBackgroundJobs() {
-
+ try {
+ org.eclipse.core.runtime.jobs.Job.getJobManager().join(ResourcesPlugin.FAMILY_AUTO_BUILD, null);
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ }
}If this is intentionally left empty for incremental implementation in this WIP PR, please add a TODO comment explaining the plan.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/JavaUtils.java
around lines 53-55, the waitForBackgroundJobs() method is empty; replace it with
logic that waits for Eclipse workspace/background jobs to finish by joining the
relevant Job families (e.g., ResourcesPlugin.FAMILY_AUTO_BUILD, PlatformUI/UI
jobs if used) via Job.getJobManager().join(...) (and any manual/build families
your tests rely on), handle InterruptedException by re-interrupting the thread
and failing or throwing a runtime assertion so tests don't proceed on incomplete
state, and apply a sensible timeout to avoid hanging; if this is intentionally
left for later, add a clear TODO comment stating the planned implementation and
why it's deferred.
| if (project.exists()) { | ||
| return project; | ||
| } |
There was a problem hiding this comment.
Validate existing project configuration before returning.
When a project already exists, the method returns it immediately without verifying that it has the correct Java nature, classpath entries, or output location. If a test reuses a project name or the workspace is not clean between test runs, the returned project may be misconfigured.
Consider validating or reconfiguring the existing project:
if (project.exists()) {
- return project;
+ // Validate and reconfigure if needed, or delete and recreate
+ project.delete(true, true, monitor);
}Alternatively, document that callers must ensure unique project names or clean workspace state.
📝 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.
| if (project.exists()) { | |
| return project; | |
| } | |
| if (project.exists()) { | |
| // Validate and reconfigure if needed, or delete and recreate | |
| project.delete(true, true, monitor); | |
| } |
🤖 Prompt for AI Agents
In
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/JavaUtils.java
around lines 59 to 61, the method returns an existing IProject without
validating its Java nature, classpath entries or output location which can lead
to misconfigured projects being reused between tests; update the logic so that
when project.exists() is true you validate and (re)configure it: check and add
the Java nature if missing, verify and set expected classpath entries on the
IJavaProject, ensure the output folder is set to the expected IPath (create it
if absent), and open the project if it is closed; if verification fails
optionally recreate or clean the project before returning, or alternatively
document that callers must provide unique project names or a clean workspace.
| try { | ||
| for (String parentSlashNameEntry : parentSlashName) { | ||
| String parentDirName = parentSlashNameEntry.substring(0, parentSlashNameEntry.lastIndexOf('/')); | ||
| String projectName = parentSlashNameEntry.substring(parentSlashNameEntry.lastIndexOf('/') + 1); | ||
|
|
||
| // Move project to working directory | ||
| File projectFolder = copyProjectToWorkingDirectory(projectName, parentDirName); | ||
| IPath path = new Path(projectFolder.getAbsolutePath()); | ||
| paths.add(path); | ||
| } | ||
|
|
||
| JavaLanguageServerPlugin.getPreferencesManager().initialize(); | ||
| JavaLanguageServerPlugin.getPreferencesManager().updateClientPrefences(new ClientCapabilities(), | ||
| new HashMap<>()); | ||
| JavaLanguageServerPlugin.getProjectsManager().initializeProjects(paths, new NullProgressMonitor()); | ||
|
|
||
| IProgressMonitor monitor = new NullProgressMonitor(); | ||
| waitForBackgroundJobs(monitor); | ||
| org.eclipse.jdt.ls.core.internal.JobHelpers.waitUntilIndexesReady(); | ||
|
|
||
| for (IPath path : paths) { | ||
| IProjectDescription description = ResourcesPlugin.getWorkspace() | ||
| .loadProjectDescription(path.append(".project")); | ||
| IProject project = ResourcesPlugin.getWorkspace().getRoot().getProject(description.getName()); | ||
| javaProjects.add(JavaCore.create(project)); | ||
| } | ||
|
|
||
| } catch (Exception e) { | ||
| e.printStackTrace(); | ||
| } |
There was a problem hiding this comment.
Silent exception handling may hide project loading failures.
The loadJavaProjects method catches all exceptions and only prints stack traces (line 137), returning an empty array on failure. This could cause tests to silently pass when project loading actually fails.
Consider propagating exceptions or at minimum logging them with appropriate severity:
} catch (Exception e) {
- e.printStackTrace();
+ LOGGER.log(Level.SEVERE, "Failed to load Java projects", e);
+ throw new RuntimeException("Project loading failed", e);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/ProjectUtilsTest.java
around lines 109 to 138: the catch-all block silently prints the stack trace and
allows the method to continue, hiding project load failures; replace the broad
try/catch behavior by rethrowing the caught exception (or wrapping it in a
RuntimeException) or call a test failure (e.g., Assertions.fail) with the
exception message so test execution stops on load errors, and remove the silent
e.printStackTrace() to ensure failures surface in test reports.
… debugTrace(). Add the property <showEclipseLog> to true to see the runtime messages Signed-off-by: Charles Moulliard <[email protected]>
|
@cmoulliard will merge this once this goes green |
|
Many thanks folks @shawn-hurley @jmle |
3 participants
WIP
Summary by CodeRabbit
New Features
Tests
Chores