Feature/travel game tests#32
Conversation
…ootstrapService tests
📝 WalkthroughWalkthroughThis PR adds comprehensive unit test coverage across five test suites totaling 663 new lines: BootstrapServiceTest, JourneyServiceTest, PlayerEventServiceTest, TransportTest, and TravelerTest. Tests employ JUnit 5 and Mockito to verify service initialization logic, journey management, event application, transport mechanics, and traveler state transitions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 1
🤖 Fix all issues with AI agents
In `@src/test/java/org/example/TransportTest.java`:
- Around line 17-40: The test rollDistance_returnsValidRange is tautological
because it mocks Transport.rollDistance() to fixed values and then asserts those
values fall within ranges; replace the mocks with real Transport instances (or
inject a mock Random source if Transport uses one) so rollDistance()
implementation is exercised: instantiate Transport objects for "BUSS", "TRAIN",
"AIRPLANE" in the test method rollDistance_returnsValidRange (or refactor
Transport to accept a Random and supply a controllable/mock Random) and run the
loop asserting returned values fall within the expected bounds for each type
instead of stubbing rollDistance().
🧹 Nitpick comments (3)
src/test/java/org/example/BootstrapServiceTest.java (2)
68-77: Redundant null assertion and loose message check.The
assertNotNull(ex)on line 74 is redundant sinceassertThrowsalready guarantees a non-null exception is returned (it would fail otherwise). Additionally, the message assertion checks for either "failed to bootstrap database" or "File not found", but the mock throws "table missing"—this works only ifBootstrapServicewraps the exception with a new message.♻️ Suggested simplification
RuntimeException ex = assertThrows(RuntimeException.class, service::initialize); verify(tx).begin(); verify(tx).isActive(); verify(tx).rollback(); - assertNotNull(ex); assertTrue(ex.getMessage().contains("failed to bootstrap database") || - ex.getMessage().contains("File not found")); + ex.getMessage().contains("File not found") || + ex.getMessage().contains("table missing"));
93-102: Same redundant null assertion.Similar to the previous test,
assertNotNull(ex)is unnecessary afterassertThrows.♻️ Suggested simplification
RuntimeException ex = assertThrows(RuntimeException.class, service::initialize); verify(tx).begin(); verify(tx).isActive(); verify(tx).rollback(); - assertNotNull(ex); assertTrue(ex.getMessage().contains("File not found") || ex.getMessage().contains("failed to bootstrap database"));src/test/java/org/example/PlayerEventServiceTest.java (1)
40-48: Tautological size check and flaky test concern.The condition
events.size() >= 0is always true for anyList—list sizes cannot be negative. Additionally, since event generation appears to be probabilistic, a single test run might not exercise all branches reliably.♻️ Suggested improvement
`@Test` -void eventCountWithinBounds() { +@RepeatedTest(10) +void eventCountWithinBounds() { Traveler traveler = dummyTraveler(); PlayerEventService service = new PlayerEventService(); List<PlayerEventService.EventResult> events = service.applyEndOfTurnEvents(traveler); - assertTrue(events.size() >= 0 && events.size() <= 2); + assertTrue(events.size() <= 2, "Expected at most 2 events, got: " + events.size()); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/test/java/org/example/BootstrapServiceTest.javasrc/test/java/org/example/JourneyServiceTest.javasrc/test/java/org/example/PlayerEventServiceTest.javasrc/test/java/org/example/TransportTest.javasrc/test/java/org/example/TravelerTest.java
🔇 Additional comments (21)
src/test/java/org/example/TravelerTest.java (7)
1-13: LGTM!The helper method
dummyLocationprovides good test isolation by creating minimal Location objects without unnecessary dependencies.
20-27: LGTM!Constructor position test correctly validates that
playerPosXandplayerPosYare initialized from the location coordinates.
34-38: LGTM!Initial state test properly verifies that a new Traveler is not travelling by default.
45-55: LGTM!Journey start test validates both the travelling state and remaining distance are set correctly.
62-72: LGTM!The advance test correctly verifies distance reduction, turn count increment, and continued travelling state.
79-90: LGTM!Journey completion test validates the edge case where advance distance exceeds remaining distance, properly checking that travelling stops and location updates.
97-120: LGTM!Payment tests properly validate both successful payment and the exception case for insufficient funds. Good use of
assertThrowsto verify both the exception type and message.src/test/java/org/example/BootstrapServiceTest.java (2)
27-36: LGTM!Well-structured test setup with properly mocked JPA components. The
@BeforeEachensures clean state for each test.
43-53: LGTM!This test correctly verifies that when the database is already seeded (count >= 1), no transaction is started and no SQL is executed.
src/test/java/org/example/JourneyServiceTest.java (7)
37-42: LGTM!Clean setup with appropriate dependency mocking for the service under test.
49-82: LGTM!Comprehensive test that validates the happy path for starting a journey, including proper setup of mocked route, transport, and transport links. Good verification of journey state and associations.
90-119: LGTM!Good boundary test that verifies the service prevents starting a new journey when already travelling. The exception message assertion ensures the error is user-friendly.
126-162: LGTM!Well-designed test that verifies transport restrictions are enforced. Creates both allowed and disallowed transports to ensure the validation logic works correctly.
169-202: LGTM!Good test for the insufficient funds scenario. Properly sets up a traveler with less money than the transport cost.
210-251: LGTM!Thorough test for journey continuation that properly mocks the database query to return an existing journey. Good verification of state consistency.
258-299: LGTM!Good test coverage for
findPossibleMoves. Verifies that multiple transports on a single route correctly expand to multiplePossibleMovesentries.src/test/java/org/example/PlayerEventServiceTest.java (5)
14-18: LGTM!Good helper method that creates a traveler with sufficient funds for testing event scenarios.
25-33: LGTM!Basic smoke test ensures the service can be called without throwing exceptions.
55-62: LGTM!Good invariant test that ensures money never goes negative after events are applied.
69-80: LGTM!Properly validates that all generated events have valid types and positive amounts.
87-97: LGTM!Good validation that all events have messages. This ensures UI/logging can always display event information.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| @Test | ||
| void rollDistance_returnsValidRange() { | ||
| Transport buss = mock(Transport.class); | ||
| when(buss.getType()).thenReturn("BUSS"); | ||
| when(buss.rollDistance()).thenAnswer(invocation -> 4); | ||
|
|
||
| Transport train = mock(Transport.class); | ||
| when(train.getType()).thenReturn("TRAIN"); | ||
| when(train.rollDistance()).thenAnswer(invocation -> 6); | ||
|
|
||
| Transport airplane = mock(Transport.class); | ||
| when(airplane.getType()).thenReturn("AIRPLANE"); | ||
| when(airplane.rollDistance()).thenAnswer(invocation -> 9); | ||
|
|
||
| for (int i = 0; i < 50; i++) { | ||
| int bussRoll = buss.rollDistance(); | ||
| int trainRoll = train.rollDistance(); | ||
| int airplaneRoll = airplane.rollDistance(); | ||
|
|
||
| assertTrue(bussRoll >= 1 && bussRoll <= 6, "BUSS roll utanför spann: " + bussRoll); | ||
| assertTrue(trainRoll >= 2 && trainRoll <= 12, "TRAIN roll utanför spann: " + trainRoll); | ||
| assertTrue(airplaneRoll >= 3 && airplaneRoll <= 18, "AIRPLANE roll utanför spann: " + airplaneRoll); | ||
| } | ||
| } |
There was a problem hiding this comment.
Test is tautological and provides no real coverage.
This test mocks Transport.rollDistance() to return fixed values (4, 6, 9), then asserts those same fixed values fall within expected ranges. The test only verifies that Mockito returns what you told it to return—it never exercises the actual Transport.rollDistance() implementation.
To properly test the randomness bounds, you should either:
- Use real
Transportinstances and run multiple iterations to statistically verify the range, or - If
Transportdelegates to a random source, inject a mock random generator and verify the calculation logic.
🧪 Example approach using real instances
`@Test`
void rollDistance_returnsValidRange() {
Transport buss = new Transport("BUSS", /* other params */);
Transport train = new Transport("TRAIN", /* other params */);
Transport airplane = new Transport("AIRPLANE", /* other params */);
for (int i = 0; i < 50; i++) {
int bussRoll = buss.rollDistance();
int trainRoll = train.rollDistance();
int airplaneRoll = airplane.rollDistance();
assertTrue(bussRoll >= 1 && bussRoll <= 6, "BUSS roll utanför spann: " + bussRoll);
assertTrue(trainRoll >= 2 && trainRoll <= 12, "TRAIN roll utanför spann: " + trainRoll);
assertTrue(airplaneRoll >= 3 && airplaneRoll <= 18, "AIRPLANE roll utanför spann: " + airplaneRoll);
}
}🤖 Prompt for AI Agents
In `@src/test/java/org/example/TransportTest.java` around lines 17 - 40, The test
rollDistance_returnsValidRange is tautological because it mocks
Transport.rollDistance() to fixed values and then asserts those values fall
within ranges; replace the mocks with real Transport instances (or inject a mock
Random source if Transport uses one) so rollDistance() implementation is
exercised: instantiate Transport objects for "BUSS", "TRAIN", "AIRPLANE" in the
test method rollDistance_returnsValidRange (or refactor Transport to accept a
Random and supply a controllable/mock Random) and run the loop asserting
returned values fall within the expected bounds for each type instead of
stubbing rollDistance().
1 participant
Uppdaterat alla tester efter feedback från olika håll.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.