Adds Unit tests for HelloModel #4
Conversation
WalkthroughThe changes introduce error handling for message reception with a fallback mechanism, convert file upload from asynchronous to synchronous HTTP communication, update test endpoints, add comprehensive test coverage for file operations and message handling, and enhance test spies with file tracking and message simulation capabilities. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant NtfyConnectionImpl
participant HTTP
rect rgba(200, 150, 100, 0.2)
Note over NtfyConnectionImpl,HTTP: Before: Async File Upload
Client->>NtfyConnectionImpl: sendFile(file)
NtfyConnectionImpl->>HTTP: http.sendAsync(request)
HTTP-->>NtfyConnectionImpl: CompletableFuture returned
NtfyConnectionImpl-->>Client: true (returns immediately)
HTTP->>HTTP: Upload continues asynchronously
end
rect rgba(100, 150, 200, 0.2)
Note over NtfyConnectionImpl,HTTP: After: Sync File Upload
Client->>NtfyConnectionImpl: sendFile(file)
NtfyConnectionImpl->>HTTP: http.send(request)
HTTP->>HTTP: Upload completes
HTTP-->>NtfyConnectionImpl: Response received
NtfyConnectionImpl-->>Client: true (returns after completion)
end
sequenceDiagram
autonumber
participant Model as HelloModel
participant JavaFX as Platform
participant MessageList as messages
rect rgba(150, 200, 100, 0.2)
Note over Model,MessageList: receiveMessage() with Error Handling
Model->>Model: receiveMessage(msg) called
Model->>JavaFX: try { Platform.runLater(...) }
alt Platform.runLater succeeds
JavaFX->>MessageList: append message
MessageList-->>JavaFX: success
else IllegalStateException caught
rect rgba(255, 150, 100, 0.2)
Note over Model,MessageList: Fallback path
Model->>MessageList: directly append message
MessageList-->>Model: fallback executed
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example:
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
🧹 Nitpick comments (1)
src/test/java/com/example/HelloModelTest.java (1)
10-10: Remove unused import.The
IOExceptionimport is not used in the test code.-import java.io.IOException;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/com/example/HelloModel.java(1 hunks)src/main/java/com/example/NtfyConnectionImpl.java(1 hunks)src/test/java/com/example/HelloModelTest.java(2 hunks)src/test/java/com/example/NtfyConnectionSpy.java(1 hunks)
🔇 Additional comments (4)
src/test/java/com/example/NtfyConnectionSpy.java (1)
3-32: LGTM! Test spy enhancements are well-implemented.The additions to the spy properly support the new test scenarios:
sendFilecaptures the file reference for verificationreceivestores the handler enabling message simulationsimulateIncomingsafely invokes the handler with null-checkThe implementation is clean and follows good testing practices.
src/test/java/com/example/HelloModelTest.java (3)
39-39: Endpoint update looks correct.The change from
/mytopicto/adamproperly aligns with the updated endpoint in the production code.Also applies to: 44-44
48-74: File upload tests are well-structured.Both tests properly verify the file sending behavior:
sendFileCallConnectionSendFilevalidates the model-to-connection interaction via spysendFileToFakeServervalidates the HTTP PUT request using WireMockThe use of temporary files and appropriate assertions make these robust tests.
97-126: Excellent test coverage for model properties and initialization.The three tests comprehensively verify:
- Handler registration during construction
- Property binding and updates for
messageToSend- Greeting text accuracy
These tests follow good practices with clear naming and appropriate assertions.
| connection.receive(m-> { | ||
| try { | ||
| Platform.runLater(()->messages.add(m)); | ||
| } catch (IllegalStateException e) { | ||
| messages.add(m); | ||
| } | ||
|
|
||
| }); |
There was a problem hiding this comment.
Fallback bypasses JavaFX thread safety and can cause concurrency issues.
The catch block that directly adds messages to the ObservableList violates JavaFX threading requirements. ObservableList modifications must occur on the JavaFX Application Thread, but this fallback executes on whatever thread invokes the handler (likely a background HTTP thread from NtfyConnectionImpl.receive).
If Platform.runLater throws IllegalStateException (JavaFX toolkit not initialized), the correct approach is to either:
- Ensure JavaFX is initialized before creating
HelloModel, or - Queue the message for later processing once JavaFX is ready
The current fallback can cause ConcurrentModificationException or corrupt the observable list if UI components are bound to it.
Consider removing the catch block entirely and ensuring JavaFX is properly initialized:
- connection.receive(m-> {
- try {
- Platform.runLater(()->messages.add(m));
- } catch (IllegalStateException e) {
- messages.add(m);
- }
-
- });
+ connection.receive(m -> Platform.runLater(() -> messages.add(m)));🤖 Prompt for AI Agents
In src/main/java/com/example/HelloModel.java around lines 55 to 62, the current
catch block directly adds to the ObservableList off the JavaFX thread which
breaks thread-safety; remove the fallback that calls messages.add(m) inside the
catch and instead either (A) ensure the JavaFX toolkit is initialized before
constructing HelloModel so Platform.runLater never throws, or (B) enqueue
incoming messages into a thread-safe queue when Platform.runLater fails and
drain that queue on the JavaFX Application Thread once initialized (e.g.,
schedule a Platform.runLater to flush queued messages). Ensure all
ObservableList modifications happen only via Platform.runLater on the FX thread.
| .build(); | ||
|
|
||
| http.sendAsync(request, HttpResponse.BodyHandlers.discarding()); | ||
| http.send(request, HttpResponse.BodyHandlers.discarding()); |
There was a problem hiding this comment.
Blocking HTTP call can freeze the JavaFX UI thread.
Changing from sendAsync to send makes this a synchronous blocking operation. If sendFile is invoked from the JavaFX Application Thread (e.g., triggered by a UI button click), the UI will freeze until the HTTP request completes. File uploads can take seconds or longer, resulting in poor user experience.
The TODO comment at lines 36-38 acknowledges the same issue for the send() method. Consider applying the same fix here—either use sendAsync or execute on a background thread.
Revert to the asynchronous approach:
- http.send(request, HttpResponse.BodyHandlers.discarding());
+ http.sendAsync(request, HttpResponse.BodyHandlers.discarding());Committable suggestion skipped: line range outside the PR's diff.
| @Test | ||
| @DisplayName("Model should receive messages when connection invokes handler") | ||
| void receiveMessageShouldAddToModelViewHander(){ | ||
| var spy = new NtfyConnectionSpy(); | ||
| var model = new HelloModel(spy); | ||
|
|
||
| NtfyMessageDto incoming = new NtfyMessageDto( | ||
| "123", | ||
| 1000, | ||
| 0, | ||
| "message", | ||
| "adam", | ||
| "this is a test hehehe", | ||
| null | ||
| ); | ||
|
|
||
| spy.simulateIncoming(incoming); | ||
|
|
||
| assertThat(model.getMessages()).containsExactly(incoming); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Test may not properly verify JavaFX threading behavior.
The test calls simulateIncoming which triggers Platform.runLater in the model, but the test immediately asserts without waiting for the JavaFX thread to execute. This test likely only passes because of the IllegalStateException catch block in HelloModel.receiveMessage that bypasses JavaFX threading (which itself is problematic).
In a proper JavaFX test environment, you should either:
- Use
WaitForAsyncUtils.waitForFxEvents()from TestFX library to ensure JavaFX thread processes the event, or - Initialize the JavaFX toolkit properly for testing
Consider verifying whether JavaFX is properly initialized in the test environment:
Additionally, search for TestFX or similar JavaFX testing utilities:
🏁 Script executed:
#!/bin/bash
# Check if JavaFX test dependencies or initialization are present in the test setup
rg -n "Platform\.startup|FxToolkit|ApplicationTest|@BeforeAll.*Platform" --type=javaLength of output: 0
🏁 Script executed:
#!/bin/bash
# Look for JavaFX testing frameworks in dependencies
rg -n "testfx|WaitForAsyncUtils" Length of output: 0
I need to gather more information about the actual implementation to verify the review concern. Let me search for the relevant source code:
🏁 Script executed:
#!/bin/bash
# Find HelloModel class and check receiveMessage implementation
rg -n "class HelloModel" --type=java -A 20Length of output: 3266
🏁 Script executed:
#!/bin/bash
# Find the receiveMessage method in HelloModel
rg -n "receiveMessage" --type=java -B 2 -A 10Length of output: 2816
🏁 Script executed:
#!/bin/bash
# Find NtfyConnectionSpy and simulateIncoming implementation
rg -n "class NtfyConnectionSpy|simulateIncoming" --type=java -B 2 -A 10Length of output: 2522
🏁 Script executed:
#!/bin/bash
# Check for pom.xml to see dependencies
fd -n "pom.xml|build.gradle" -x cat {}Length of output: 230
🏁 Script executed:
#!/bin/bash
# Look for test setup or configuration methods in the test file
rg -n "@BeforeEach|@Before|@BeforeAll|static.*{|setUp" src/test/java/com/example/HelloModelTest.java -B 1 -A 5Length of output: 168
🏁 Script executed:
#!/bin/bash
# Check imports and class declaration at top of HelloModelTest
head -20 src/test/java/com/example/HelloModelTest.javaLength of output: 688
🏁 Script executed:
#!/bin/bash
# Check HelloModel imports for JavaFX
head -15 src/main/java/com/example/HelloModel.javaLength of output: 360
🏁 Script executed:
#!/bin/bash
# Search for any Platform or javafx usage in the codebase
rg -n "import.*Platform|import.*javafx" --type=javaLength of output: 1882
🏁 Script executed:
#!/bin/bash
# Fix the fd command to list pom.xml and build.gradle files
fd "pom.xml|build.gradle"Length of output: 7
🏁 Script executed:
#!/bin/bash
# Check pom.xml for dependencies
cat pom.xmlLength of output: 3194
Test relies on IllegalStateException fallback instead of verifying actual JavaFX threading.
The test passes because:
- JavaFX toolkit is never initialized (no imports, no @beforeeach setup, no TestFX)
Platform.runLater()inHelloModel.receiveMessage()throwsIllegalStateException- The catch block at line 58-60 executes
messages.add(m)directly, bypassing threading entirely - Immediate assertion succeeds on this fallback path, not on proper JavaFX thread execution
The production code's try-catch (lines 56-60 in src/main/java/com/example/HelloModel.java) is a code smell—it couples production logic to test environment state.
Fix options:
- Add TestFX dependency and use
WaitForAsyncUtils.waitForFxEvents()to properly wait for JavaFX thread execution - Initialize JavaFX toolkit with
@BeforeEachusingPlatform.startup()orcom.sun.javafx.application.PlatformImpl.startup() - Remove the try-catch workaround once proper JavaFX testing is in place
🤖 Prompt for AI Agents
In src/test/java/com/example/HelloModelTest.java around lines 76-95 the test is
passing due to HelloModel.receiveMessage() falling back to catching
IllegalStateException from Platform.runLater(), so update the test to run
against a real JavaFX toolkit rather than relying on that fallback: initialize
the JavaFX platform before tests (e.g., with a @BeforeEach that calls
Platform.startup(...) or use TestFX/TestApplication setup), then use a proper
sync/wait mechanism (e.g., WaitForAsyncUtils.waitForFxEvents() or an explicit
wait) after spy.simulateIncoming(...) before asserting model.getMessages(); once
tests are updated, remove the try-catch fallback in
src/main/java/com/example/HelloModel.java (lines ~56-60) so production code no
longer depends on IllegalStateException from missing JavaFX initialization.
This PR adds eight unit tests for the HelloModel class.
The tests cover the following behaviors:
Summary by CodeRabbit
Bug Fixes
Tests