Fix JavaFX threading issues and improve tests #5
Conversation
thread handling
file uploads
and adding async delay for file upload
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (3)
Tip CodeRabbit can scan for known vulnerabilities in your dependencies using OSV Scanner.OSV Scanner will automatically detect and report security vulnerabilities in your project's dependencies. No additional configuration is required. ✨ 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 |
|
Warning Docstrings generation - IN PROGRESS Generating docstrings for this pull request |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/test/java/com/example/HelloModelTest.java (1)
75-76: Consider more robust async coordination.Fixed delays with
Thread.sleep(500)can cause flaky tests if operations take longer than expected or waste time if they complete sooner. Consider using more deterministic approaches:Option 1: Use CountDownLatch (Java built-in)
For
sendFileToFakeServer:void sendFileToFakeServer(WireMockRuntimeInfo wmRuntimeInfo) throws Exception { var con = new NtfyConnectionImpl("http://localhost:" + wmRuntimeInfo.getHttpPort()); var model = new HelloModel(con); File temp = File.createTempFile("upload_test", ".txt"); stubFor(put("/adam").willReturn(ok())); CountDownLatch latch = new CountDownLatch(1); // Hook to detect completion - would need API change in sendFile model.sendFile(temp); assertThat(latch.await(2, TimeUnit.SECONDS)).isTrue(); verify(putRequestedFor(urlEqualTo("/adam"))); }Option 2: Use Awaitility library (recommended)
Add dependency:
<dependency> <groupId>org.awaitility</groupId> <artifactId>awaitility</artifactId> <version>4.2.0</version> <scope>test</scope> </dependency>Then:
import static org.awaitility.Awaitility.*; // In sendFileToFakeServer: model.sendFile(temp); await().atMost(2, TimeUnit.SECONDS) .untilAsserted(() -> verify(putRequestedFor(urlEqualTo("/adam")))); // In receiveMessageShouldAddToModelViewHander: spy.simulateIncoming(incoming); await().atMost(2, TimeUnit.SECONDS) .untilAsserted(() -> assertThat(model.getMessages()).containsExactly(incoming));Note: The underlying issue in
NtfyConnectionImpl.sendFile(ignored async result) makes proper test coordination harder. Fixing that would enable cleaner test code.Also applies to: 100-101
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/example/HelloModel.java(1 hunks)src/main/java/com/example/NtfyConnectionImpl.java(3 hunks)src/test/java/com/example/HelloModelTest.java(3 hunks)
🔇 Additional comments (3)
src/main/java/com/example/NtfyConnectionImpl.java (1)
58-58: LGTM! Modern Java method references.The refactoring to use method references (
System.out::printlnandObjects::nonNull) improves code readability and follows modern Java best practices.Also applies to: 67-67
src/main/java/com/example/HelloModel.java (1)
55-57: LGTM! Simplified callback improves readability.The streamlined callback directly schedules UI updates via
Platform.runLaterwithout nested exception handling, making the code cleaner and more maintainable. This assumes the JavaFX platform is properly initialized, which the test updates in this PR now ensure.src/test/java/com/example/HelloModelTest.java (1)
16-19: Excellent addition! Enables JavaFX operations in tests.The
@BeforeAllinitialization properly starts the JavaFX platform for headless testing, preventingIllegalStateExceptionwhenPlatform.runLateris invoked inHelloModel. This is essential for the simplified callback approach in this PR.
| http.sendAsync(request, HttpResponse.BodyHandlers.discarding()); | ||
| return true; |
There was a problem hiding this comment.
Critical: Ignored async result breaks the return contract.
The change from send to sendAsync makes the operation non-blocking, but the CompletableFuture result is discarded. The method immediately returns true on line 90 without waiting for the upload to complete or handling failures. This means:
- Callers cannot rely on the boolean return value to determine success/failure
- Async errors will be silently ignored
- Tests added in this PR work around this with
Thread.sleep(500), which is brittle
Consider one of these approaches:
Option 1: Keep synchronous behavior (minimal change)
- http.sendAsync(request, HttpResponse.BodyHandlers.discarding());
+ var response = http.send(request, HttpResponse.BodyHandlers.discarding());
return true;Option 2: Change to async API (requires broader refactoring)
- public boolean sendFile(File file){
+ public CompletableFuture<Boolean> sendFile(File file){
try {
String mime = java.nio.file.Files.probeContentType(file.toPath());
if(mime == null) mime = "application/octet-stream";
long size = file.length();
HttpRequest request = HttpRequest.newBuilder()
.uri(URI.create(hostName + "/adam"))
.header("Filename", file.getName())
.header("Content-Type", mime)
.PUT(HttpRequest.BodyPublishers.ofFile(file.toPath()))
.build();
- http.sendAsync(request, HttpResponse.BodyHandlers.discarding());
- return true;
+ return http.sendAsync(request, HttpResponse.BodyHandlers.discarding())
+ .thenApply(response -> true)
+ .exceptionally(e -> {
+ e.printStackTrace();
+ return false;
+ });
} catch (Exception e) {
e.printStackTrace();
- return false;
+ return CompletableFuture.completedFuture(false);
}
}If choosing Option 2, update NtfyConnection interface and all callers (including HelloModel.sendFile).
📝 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.
| http.sendAsync(request, HttpResponse.BodyHandlers.discarding()); | |
| return true; | |
| var response = http.send(request, HttpResponse.BodyHandlers.discarding()); | |
| return true; |
🤖 Prompt for AI Agents
In src/main/java/com/example/NtfyConnectionImpl.java around lines 89-90, the
code switched from synchronous http.send to http.sendAsync but discards the
returned CompletableFuture and immediately returns true, breaking the method's
success/failure contract; either restore synchronous behavior by using http.send
and propagate/return its success/failure, or (if you want true async) update the
NtfyConnection interface and all callers (including HelloModel.sendFile) to
return a CompletableFuture<Boolean> and propagate the future from sendAsync,
adding proper exception handling (handle/whenComplete) and mapping failures to
false (or exceptional completion) so callers can observe outcomes; choose Option
1 for minimal change (replace sendAsync with send and handle
IOException/InterruptedException, returning false or rethrow as appropriate) or
Option 2 for a broader refactor (change signatures, propagate futures, update
tests to await results instead of sleeping).
This PR fixes JavaFX threading in HelloModel, updates NtfyConnectionImpl sendFile, and adds proper JavaFX initialization in tests.
Summary by CodeRabbit
Improvements
Tests