Conversation
…eded to be able to run our TestConnectionProvider to prevent tests to insert data into production.
implemented function so that we can connect to correct DB depending if test or not. added necessary dependencies
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds TestContainers-based MySQL integration tests and test data; introduces a TEST-mode switch in ConnectionProvider to use containerized MySQL settings; adds test DB schema/seed SQL and MySQL config; updates pom.xml with test dependencies and surefire plugin; small Booking and BookingService tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant MySQLContainer
participant Env
participant ConnectionProvider
participant EntityManagerFactory
participant Database
TestRunner->>MySQLContainer: start container with init script
MySQLContainer-->>TestRunner: provide JDBC URL, port, credentials
TestRunner->>Env: set TEST=TRUE and JDBC env vars
TestRunner->>ConnectionProvider: trigger initialization (tests start)
ConnectionProvider->>Env: read TEST and JDBC vars
ConnectionProvider->>EntityManagerFactory: build EMF using test JDBC config
EntityManagerFactory->>Database: connect and apply schema/init (testData.sql)
TestRunner->>EntityManagerFactory: run repository/service tests
EntityManagerFactory->>Database: perform CRUD operations validated by tests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 6
🤖 Fix all issues with AI agents
In `@pom.xml`:
- Around line 43-47: The org.testcontainers:mysql dependency entry is missing a
test scope and is being packaged into non-test artifacts; update the dependency
for groupId "org.testcontainers" and artifactId "mysql" (version "1.21.3") to
include a <scope>test</scope> element so it’s only used for integration tests,
matching the other test dependencies.
In `@src/main/java/backend/ConnectionProvider.java`:
- Around line 21-34: The null-check bug is that env.get("TEST").equals("TRUE")
can NPE if TEST is unset; in ConnectionProvider (where cfg is set using
HibernatePersistenceConfiguration) change the comparison to be null-safe by
calling "TRUE".equals(env.get("TEST")) or use Objects.equals(env.get("TEST"),
"TRUE") before entering the test-config block so the code no longer throws a
NullPointerException when the TEST environment variable is missing.
In `@src/main/java/backend/entities/Booking.java`:
- Around line 122-136: Replace the incorrect overload public boolean
equals(Booking booking) with a proper override public boolean equals(Object o)
annotated with `@Override`; perform an instanceof (or getClass()) check, handle
nulls, and compare all relevant fields (id, tableId, bookingStart, bookingEnd
and date) using Objects.equals(...) to avoid NPEs; also add a matching `@Override`
public int hashCode() that uses Objects.hash(id, tableId, bookingStart,
bookingEnd, date) and add the import java.util.Objects if missing.
In `@src/main/resources/testData.sql`:
- Around line 19-26: The Customer table definition has a duplicate
AUTO_INCREMENT on the customerId column causing a syntax error; edit the CREATE
TABLE statement (table Customer, column customerId) to use AUTO_INCREMENT only
once (remove the second AUTO_INCREMENT token) so the column is defined as a
single auto-incrementing primary key.
In `@src/test/java/backend/services/BookingServiceTest.java`:
- Around line 29-43: Remove the hard-coded host port binding and the stray
double semicolon: delete the withCreateContainerCmdModifier(...) block that
calls Ports.Binding.bindPort(3307) (or modify it to let Testcontainers assign
ports dynamically) so the MySQLContainer<?> mysql uses dynamic port mapping, and
remove the extra trailing ";;" after the container declaration; when you need
the actual host port at runtime, call mysql.getMappedPort(3306).
In `@src/test/resources/persistence-test.xml`:
- Around line 1-11: The persistence.xml is missing the XML declaration and the
required root <persistence> element/namespace, so wrap the existing
<persistence-unit name="test"> block inside a <persistence> root element and add
the XML declaration; ensure the root includes the JPA namespace and version
attributes (e.g., xmlns="http://xmlns.jcp.org/xml/ns/persistence" and
version="2.2") so the persistence-unit element is valid and the file parses.
🧹 Nitpick comments (5)
src/main/resources/testData.sql (2)
1-3: Test data file located insrc/main/resources.This file will be included in the production artifact. Consider moving it to
src/test/resources/to keep test data separate from production resources. The TestcontainerswithInitScript()can load from the test classpath.
9-18: Consider adding foreign key constraints.The
Bookingtable referencescustomer_id,restaurant_id, andtable_idbut lacks foreign key constraints. While this may be intentional for test flexibility, adding constraints would catch integrity issues earlier.src/test/java/backend/services/BookingServiceTest.java (3)
99-118: UnusedbookingRepovariable.
bookingRepois instantiated on line 103 but never used in this test method. The same issue appears inshouldNotAllowDoubleBookingSameTime(line 123) andshouldNotAllowOverlappingBookings(line 154).🧹 Proposed fix
`@Test` `@Order`(2) void shouldNotAllowBookingWhenNoTableCapacity() { RestaurantRepo restaurantRepo = new RestaurantRepo(); - BookingRepo bookingRepo = new BookingRepo(); Restaurant restaurant = restaurantRepo.fetchRestaurantByName("TOSO");Apply similar removal to lines 123 and 154.
54-71: Test state dependency makes tests fragile.
testConnectiontruncates theBookingtable, and subsequent tests rely on this execution order. If tests run in a different order or one fails mid-execution, subsequent tests may produce false failures. Consider using@BeforeEachto reset state, or at minimum, document that test order is intentional.
92-95: UseassertEqualsor AssertJ for better failure messages.
assertTrue(booking.equals(...))provides a generic "expected true but was false" message on failure. UsingassertEqualsor AssertJ'sassertThatgives more informative output showing the actual vs expected values.🛠️ Proposed fix
assertNotNull(booking); List<Booking> fetchedBookings = bookingRepo.fetchBookingsByRestaurant(restaurant); assertEquals(1, fetchedBookings.size()); - assertTrue(booking.equals(fetchedBookings.getFirst())); + assertEquals(booking, fetchedBookings.getFirst());Or using AssertJ (already imported):
- assertTrue(booking.equals(fetchedBookings.getFirst())); + assertThat(fetchedBookings.getFirst()).isEqualTo(booking);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
pom.xmlsrc/main/java/backend/ConnectionProvider.javasrc/main/java/backend/entities/Booking.javasrc/main/java/backend/services/BookingService.javasrc/main/resources/myconfig/my.cnfsrc/main/resources/testData.sqlsrc/test/java/backend/services/BookingServiceTest.javasrc/test/resources/persistence-test.xml
🧰 Additional context used
🧬 Code graph analysis (1)
src/test/java/backend/services/BookingServiceTest.java (2)
src/main/java/backend/repositories/BookingRepo.java (1)
BookingRepo(12-53)src/main/java/backend/repositories/RestaurantRepo.java (1)
RestaurantRepo(10-36)
🔇 Additional comments (6)
src/main/resources/myconfig/my.cnf (1)
1-4: LGTM!The MySQL configuration is appropriate for test containers. Using
STRICT_ALL_TABLESSQL mode ensures data integrity during tests, and theinnodb_redo_log_capacitysetting is suitable for a test environment.pom.xml (2)
95-100: LGTM!H2 database dependency is correctly scoped for tests.
127-131: LGTM!Adding maven-surefire-plugin enables proper test execution during the build.
src/main/java/backend/ConnectionProvider.java (1)
35-48: LGTM!Production database configuration correctly retrieves credentials from environment variables.
src/main/java/backend/services/BookingService.java (1)
57-57: LGTM!Minor whitespace change with no functional impact.
src/test/java/backend/services/BookingServiceTest.java (1)
119-180: Good test coverage for booking constraints.The tests effectively verify:
- Double booking prevention for the same time slot
- Overlapping booking prevention for adjacent time slots
- Using different restaurants to avoid test interference
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| <dependency> | ||
| <groupId>org.testcontainers</groupId> | ||
| <artifactId>mysql</artifactId> | ||
| <version>1.21.3</version> | ||
| </dependency> |
There was a problem hiding this comment.
Missing <scope>test</scope> on Testcontainers MySQL dependency.
The mysql Testcontainers dependency is missing the test scope, which means it will be included in production artifacts. Since this is only used for integration testing, it should be scoped to test like the other test dependencies.
Proposed fix
<dependency>
<groupId>org.testcontainers</groupId>
<artifactId>mysql</artifactId>
<version>1.21.3</version>
+ <scope>test</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>org.testcontainers</groupId> | |
| <artifactId>mysql</artifactId> | |
| <version>1.21.3</version> | |
| </dependency> | |
| <dependency> | |
| <groupId>org.testcontainers</groupId> | |
| <artifactId>mysql</artifactId> | |
| <version>1.21.3</version> | |
| <scope>test</scope> | |
| </dependency> |
🤖 Prompt for AI Agents
In `@pom.xml` around lines 43 - 47, The org.testcontainers:mysql dependency entry
is missing a test scope and is being packaged into non-test artifacts; update
the dependency for groupId "org.testcontainers" and artifactId "mysql" (version
"1.21.3") to include a <scope>test</scope> element so it’s only used for
integration tests, matching the other test dependencies.
| if(env.get("TEST").equals("TRUE")){ | ||
|
|
||
| cfg = new HibernatePersistenceConfiguration("persistence") | ||
| .jdbcUrl("jdbc:mysql://localhost:3307/testdb") | ||
| .jdbcUsername("user") | ||
| .jdbcPassword("password") | ||
| .property("hibernate.connection.provider_class", "org.hibernate.hikaricp.internal.HikariCPConnectionProvider") | ||
| .property("hibernate.hbm2ddl.auto", "update") | ||
| .property("hibernate.dialect", "org.hibernate.dialect.MySQLDialect") | ||
| .property("hibernate.show_sql", "true") | ||
| .property("hibernate.format_sql", "true") | ||
| .property("hibernate.highlight_sql", "true") | ||
| .managedClasses(Booking.class, Customer.class, DiningTable.class, OpeningHours.class, Restaurant.class); | ||
| } |
There was a problem hiding this comment.
Potential NullPointerException if TEST environment variable is not set.
If the TEST environment variable is not defined, env.get("TEST") returns null, and calling .equals("TRUE") on it will throw a NullPointerException. Reverse the comparison to avoid this.
Proposed fix
- if(env.get("TEST").equals("TRUE")){
+ if("TRUE".equals(env.get("TEST"))){📝 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(env.get("TEST").equals("TRUE")){ | |
| cfg = new HibernatePersistenceConfiguration("persistence") | |
| .jdbcUrl("jdbc:mysql://localhost:3307/testdb") | |
| .jdbcUsername("user") | |
| .jdbcPassword("password") | |
| .property("hibernate.connection.provider_class", "org.hibernate.hikaricp.internal.HikariCPConnectionProvider") | |
| .property("hibernate.hbm2ddl.auto", "update") | |
| .property("hibernate.dialect", "org.hibernate.dialect.MySQLDialect") | |
| .property("hibernate.show_sql", "true") | |
| .property("hibernate.format_sql", "true") | |
| .property("hibernate.highlight_sql", "true") | |
| .managedClasses(Booking.class, Customer.class, DiningTable.class, OpeningHours.class, Restaurant.class); | |
| } | |
| if("TRUE".equals(env.get("TEST"))){ | |
| cfg = new HibernatePersistenceConfiguration("persistence") | |
| .jdbcUrl("jdbc:mysql://localhost:3307/testdb") | |
| .jdbcUsername("user") | |
| .jdbcPassword("password") | |
| .property("hibernate.connection.provider_class", "org.hibernate.hikaricp.internal.HikariCPConnectionProvider") | |
| .property("hibernate.hbm2ddl.auto", "update") | |
| .property("hibernate.dialect", "org.hibernate.dialect.MySQLDialect") | |
| .property("hibernate.show_sql", "true") | |
| .property("hibernate.format_sql", "true") | |
| .property("hibernate.highlight_sql", "true") | |
| .managedClasses(Booking.class, Customer.class, DiningTable.class, OpeningHours.class, Restaurant.class); | |
| } |
🤖 Prompt for AI Agents
In `@src/main/java/backend/ConnectionProvider.java` around lines 21 - 34, The
null-check bug is that env.get("TEST").equals("TRUE") can NPE if TEST is unset;
in ConnectionProvider (where cfg is set using HibernatePersistenceConfiguration)
change the comparison to be null-safe by calling "TRUE".equals(env.get("TEST"))
or use Objects.equals(env.get("TEST"), "TRUE") before entering the test-config
block so the code no longer throws a NullPointerException when the TEST
environment variable is missing.
| public boolean equals(Booking booking) { | ||
| if(this.id.equals( booking.id) | ||
| && this.tableId.equals(booking.tableId) | ||
| && this.bookingStart.equals(booking.bookingStart) | ||
| && this.bookingEnd.equals( | ||
| booking.bookingEnd) | ||
| ){ | ||
| return true; | ||
| } | ||
|
|
||
|
|
||
|
|
||
| return false; | ||
|
|
||
| } |
There was a problem hiding this comment.
Incorrect equals implementation: does not override Object.equals(Object).
This method overloads rather than overrides equals. Collections and frameworks call equals(Object), so this custom equals(Booking) won't be used by standard APIs (e.g., List.contains(), Set, Map). Additionally:
- Missing null check for the
bookingparameter - Potential NPE if any compared field (
id,tableId,bookingStart,bookingEnd) is null - The
datefield is not compared, so bookings on different dates could be considered equal - Missing corresponding
hashCode()override
Proposed fix: proper equals/hashCode override
- public boolean equals(Booking booking) {
- if(this.id.equals( booking.id)
- && this.tableId.equals(booking.tableId)
- && this.bookingStart.equals(booking.bookingStart)
- && this.bookingEnd.equals(
- booking.bookingEnd)
- ){
- return true;
- }
-
-
-
- return false;
-
- }
+ `@Override`
+ public boolean equals(Object o) {
+ if (this == o) return true;
+ if (o == null || getClass() != o.getClass()) return false;
+ Booking booking = (Booking) o;
+ return Objects.equals(id, booking.id)
+ && Objects.equals(tableId, booking.tableId)
+ && Objects.equals(bookingStart, booking.bookingStart)
+ && Objects.equals(bookingEnd, booking.bookingEnd)
+ && Objects.equals(date, booking.date);
+ }
+
+ `@Override`
+ public int hashCode() {
+ return Objects.hash(id, tableId, bookingStart, bookingEnd, date);
+ }Add import at the top of the file:
import java.util.Objects;🤖 Prompt for AI Agents
In `@src/main/java/backend/entities/Booking.java` around lines 122 - 136, Replace
the incorrect overload public boolean equals(Booking booking) with a proper
override public boolean equals(Object o) annotated with `@Override`; perform an
instanceof (or getClass()) check, handle nulls, and compare all relevant fields
(id, tableId, bookingStart, bookingEnd and date) using Objects.equals(...) to
avoid NPEs; also add a matching `@Override` public int hashCode() that uses
Objects.hash(id, tableId, bookingStart, bookingEnd, date) and add the import
java.util.Objects if missing.
| @Container | ||
| private static final MySQLContainer<?> mysql = new MySQLContainer<>("mysql:9.5.0") | ||
| .withDatabaseName("testdb") | ||
| .withUsername("user") | ||
| .withPassword("password") | ||
| .withConfigurationOverride("myconfig") | ||
| .withInitScript("testData.sql") | ||
| .withCreateContainerCmdModifier(cmd -> | ||
| cmd.getHostConfig().withPortBindings( | ||
| new PortBinding( | ||
| Ports.Binding.bindPort(3307), // HOST PORT | ||
| new ExposedPort(3306) // CONTAINER PORT | ||
| ) | ||
| ) | ||
| );; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Docker Hub MySQL image available versions tags 9.5.0 8.0
💡 Result:
Yes — the official MySQL Docker image on Docker Hub includes both tag 9.5.0 and 8.0 (e.g., 8.0.44). [1][2]
Sources:
[1] Docker Hub — mysql (official) overview (shows 9.5.0 listed).
[2] Docker Hub — mysql tags page (shows 8.0 / 8.0.44 tags).
🏁 Script executed:
# Check the actual file content around the suspicious areas
if [ -f "src/test/java/backend/services/BookingServiceTest.java" ]; then
echo "=== Full Container setup (lines 29-43) ==="
sed -n '29,43p' src/test/java/backend/services/BookingServiceTest.java
echo ""
echo "=== Context around line 43 ==="
sed -n '40,50p' src/test/java/backend/services/BookingServiceTest.java
else
echo "File not found. Searching for BookingServiceTest.java..."
find . -name "BookingServiceTest.java" -type f
fiRepository: ithsjava25/project-jpa-grupp-2-g
Length of output: 1171
Remove fixed port binding and fix syntax error.
-
Fixed port binding (3307): Hardcoding the host port can cause "port already in use" failures, especially in CI environments or when running tests in parallel. Testcontainers dynamically assigns ports by default—let it do so and use
mysql.getMappedPort(3306)if needed. -
Line 43: Double semicolon (
;;) is a syntax error.
🛠️ Proposed fix
`@Container`
private static final MySQLContainer<?> mysql = new MySQLContainer<>("mysql:9.5.0")
.withDatabaseName("testdb")
.withUsername("user")
.withPassword("password")
.withConfigurationOverride("myconfig")
.withInitScript("testData.sql")
- .withCreateContainerCmdModifier(cmd ->
- cmd.getHostConfig().withPortBindings(
- new PortBinding(
- Ports.Binding.bindPort(3307), // HOST PORT
- new ExposedPort(3306) // CONTAINER PORT
- )
- )
- );;
+ .withInitScript("testData.sql");🤖 Prompt for AI Agents
In `@src/test/java/backend/services/BookingServiceTest.java` around lines 29 - 43,
Remove the hard-coded host port binding and the stray double semicolon: delete
the withCreateContainerCmdModifier(...) block that calls
Ports.Binding.bindPort(3307) (or modify it to let Testcontainers assign ports
dynamically) so the MySQLContainer<?> mysql uses dynamic port mapping, and
remove the extra trailing ";;" after the container declaration; when you need
the actual host port at runtime, call mysql.getMappedPort(3306).
| <persistence-unit name="test"> | ||
| <provider>org.hibernate.jpa.HibernatePersistenceProvider</provider> | ||
|
|
||
| <properties> | ||
| <property name="jakarta.persistence.jdbc.driver" value="org.h2.Driver"/> | ||
| <property name="jakarta.persistence.jdbc.url" value="jdbc:h2:mem:test;DB_CLOSE_DELAY=-1"/> | ||
| <property name="hibernate.dialect" value="org.hibernate.dialect.H2Dialect"/> | ||
| <property name="hibernate.hbm2ddl.auto" value="create-drop"/> | ||
| <property name="hibernate.show_sql" value="false"/> | ||
| </properties> | ||
| </persistence-unit> |
There was a problem hiding this comment.
Missing XML declaration and root <persistence> element.
This file is malformed and will fail to parse. A valid JPA persistence.xml requires the XML declaration and a <persistence> root element with proper namespace declarations.
🛠️ Proposed fix
+<?xml version="1.0" encoding="UTF-8"?>
+<persistence xmlns="https://jakarta.ee/xml/ns/persistence"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="https://jakarta.ee/xml/ns/persistence
+ https://jakarta.ee/xml/ns/persistence/persistence_3_0.xsd"
+ version="3.0">
<persistence-unit name="test">
<provider>org.hibernate.jpa.HibernatePersistenceProvider</provider>
<properties>
<property name="jakarta.persistence.jdbc.driver" value="org.h2.Driver"/>
<property name="jakarta.persistence.jdbc.url" value="jdbc:h2:mem:test;DB_CLOSE_DELAY=-1"/>
<property name="hibernate.dialect" value="org.hibernate.dialect.H2Dialect"/>
<property name="hibernate.hbm2ddl.auto" value="create-drop"/>
<property name="hibernate.show_sql" value="false"/>
</properties>
</persistence-unit>
+</persistence>📝 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.
| <persistence-unit name="test"> | |
| <provider>org.hibernate.jpa.HibernatePersistenceProvider</provider> | |
| <properties> | |
| <property name="jakarta.persistence.jdbc.driver" value="org.h2.Driver"/> | |
| <property name="jakarta.persistence.jdbc.url" value="jdbc:h2:mem:test;DB_CLOSE_DELAY=-1"/> | |
| <property name="hibernate.dialect" value="org.hibernate.dialect.H2Dialect"/> | |
| <property name="hibernate.hbm2ddl.auto" value="create-drop"/> | |
| <property name="hibernate.show_sql" value="false"/> | |
| </properties> | |
| </persistence-unit> | |
| <?xml version="1.0" encoding="UTF-8"?> | |
| <persistence xmlns="https://jakarta.ee/xml/ns/persistence" | |
| xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" | |
| xsi:schemaLocation="https://jakarta.ee/xml/ns/persistence | |
| https://jakarta.ee/xml/ns/persistence/persistence_3_0.xsd" | |
| version="3.0"> | |
| <persistence-unit name="test"> | |
| <provider>org.hibernate.jpa.HibernatePersistenceProvider</provider> | |
| <properties> | |
| <property name="jakarta.persistence.jdbc.driver" value="org.h2.Driver"/> | |
| <property name="jakarta.persistence.jdbc.url" value="jdbc:h2:mem:test;DB_CLOSE_DELAY=-1"/> | |
| <property name="hibernate.dialect" value="org.hibernate.dialect.H2Dialect"/> | |
| <property name="hibernate.hbm2ddl.auto" value="create-drop"/> | |
| <property name="hibernate.show_sql" value="false"/> | |
| </properties> | |
| </persistence-unit> | |
| </persistence> |
🤖 Prompt for AI Agents
In `@src/test/resources/persistence-test.xml` around lines 1 - 11, The
persistence.xml is missing the XML declaration and the required root
<persistence> element/namespace, so wrap the existing <persistence-unit
name="test"> block inside a <persistence> root element and add the XML
declaration; ensure the root includes the JPA namespace and version attributes
(e.g., xmlns="http://xmlns.jcp.org/xml/ns/persistence" and version="2.2") so the
persistence-unit element is valid and the file parses.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pom.xml (1)
59-64: MySQL connector scoped totestwill break production runtime.Moving
mysql-connector-jto test scope removes the JDBC driver from the runtime classpath. The application uses Hibernate with MySQL (as evidenced byhibernate-coreandHikariCPdependencies), so production deployments will fail to connect to the database.Proposed fix
<dependency> <groupId>com.mysql</groupId> <artifactId>mysql-connector-j</artifactId> <version>9.5.0</version> - <scope>test</scope> + <scope>runtime</scope> </dependency>
♻️ Duplicate comments (2)
pom.xml (1)
43-47: Missing<scope>test</scope>on Testcontainers MySQL dependency.This dependency should be scoped to
testlike the other test dependencies to avoid including it in production artifacts.Proposed fix
<dependency> <groupId>org.testcontainers</groupId> <artifactId>mysql</artifactId> <version>1.21.3</version> + <scope>test</scope> </dependency>src/test/java/backend/services/BookingServiceTest.java (1)
29-43: Remove fixed port binding to avoid port conflicts.Hardcoding host port 3307 can cause test failures in CI or parallel execution. Let Testcontainers assign ports dynamically.
Proposed fix
`@Container` private static final MySQLContainer<?> mysql = new MySQLContainer<>("mysql:9.5.0") .withDatabaseName("testdb") .withUsername("user") .withPassword("password") .withConfigurationOverride("myconfig") - .withInitScript("testData.sql") - .withCreateContainerCmdModifier(cmd -> - cmd.getHostConfig().withPortBindings( - new PortBinding( - Ports.Binding.bindPort(3307), // HOST PORT - new ExposedPort(3306) // CONTAINER PORT - ) - ) - ); + .withInitScript("testData.sql");Also remove the unused imports:
-import com.github.dockerjava.api.model.ExposedPort; -import com.github.dockerjava.api.model.PortBinding; -import com.github.dockerjava.api.model.Ports;
🧹 Nitpick comments (6)
src/test/java/backend/services/BookingServiceTest.java (6)
45-52: Remove debug print statement.The
System.out.printlnon line 48 clutters test output. Remove it or use a proper test logger if diagnostic output is needed.Proposed fix
`@BeforeAll` static void wireDbProperties() { System.setProperty("APP_JDBC_URL", mysql.getJdbcUrl()); - System.out.println("mysql url: " + mysql.getJdbcUrl()); System.setProperty("APP_DB_USER", mysql.getUsername()); System.setProperty("APP_DB_PASS", mysql.getPassword()); - }
54-71: Consider moving table truncation to@BeforeAllsetup.This test method primarily performs setup (truncating tables) rather than testing behavior. Moving the truncation logic to
wireDbProperties()or a separate@BeforeAllmethod would make the test intent clearer and ensure a clean slate regardless of test order.
92-96: PreferassertEqualsoverassertTruewithequals().Using
assertEqualsprovides clearer failure messages showing both expected and actual values.Proposed fix
assertNotNull(booking); List<Booking> fetchedBookings = bookingRepo.fetchBookingsByRestaurant(restaurant); assertEquals(1, fetchedBookings.size()); - assertTrue(booking.equals(fetchedBookings.getFirst())); + assertEquals(booking, fetchedBookings.getFirst());
99-118: Remove unusedbookingRepovariable.
bookingRepois declared on line 103 but never used in this test.Proposed fix
`@Test` `@Order`(2) void shouldNotAllowBookingWhenNoTableCapacity() { RestaurantRepo restaurantRepo = new RestaurantRepo(); - BookingRepo bookingRepo = new BookingRepo(); Restaurant restaurant = restaurantRepo.fetchRestaurantByName("TOSO");
119-149: Remove unusedbookingRepovariable.
bookingRepois declared on line 123 but never used in this test.Proposed fix
`@Test` `@Order`(3) void shouldNotAllowDoubleBookingSameTime() { RestaurantRepo restaurantRepo = new RestaurantRepo(); - BookingRepo bookingRepo = new BookingRepo(); Restaurant restaurant = restaurantRepo.fetchRestaurantByName("TOSO");
150-180: Remove unusedbookingRepovariable.
bookingRepois declared on line 154 but never used in this test.Proposed fix
`@Test` `@Order`(4) void shouldNotAllowOverlappingBookings() { RestaurantRepo restaurantRepo = new RestaurantRepo(); - BookingRepo bookingRepo = new BookingRepo(); Restaurant restaurant = restaurantRepo.fetchRestaurantByName("Lilla Taverna");
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pom.xmlsrc/main/resources/testData.sqlsrc/test/java/backend/services/BookingServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/resources/testData.sql
🧰 Additional context used
🧬 Code graph analysis (1)
src/test/java/backend/services/BookingServiceTest.java (2)
src/main/java/backend/repositories/BookingRepo.java (1)
BookingRepo(12-53)src/main/java/backend/repositories/RestaurantRepo.java (1)
RestaurantRepo(10-36)
🔇 Additional comments (1)
pom.xml (1)
37-42: LGTM!The test dependencies for Testcontainers JUnit integration, Spring JDBC, H2, and the Surefire plugin are correctly configured with appropriate test scopes.
Also applies to: 48-53, 95-100, 127-131
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
1 participant
Summary by CodeRabbit
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.