Add testing it testcontainer #162
Conversation
|
Warning Rate limit exceeded@drr00t has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 5 seconds before requesting another review. ⌛ 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 (16)
WalkthroughAdds Testcontainers test dependencies and a BaseTestContainerTestIT that starts a FalkorDB Docker container for tests. Several test classes now extend that base and obtain host/port from it; InstantiationTest adds an @AfterEach cleanup. Only test/build config and test sources changed; no production code edits. Changes
Sequence Diagram(s)sequenceDiagram
participant JUnit
participant BaseTest as BaseTestContainerTestIT
participant Container as FalkorDB Container
participant Test as TestClass
participant Driver as FalkorDB.driver
JUnit->>BaseTest: @BeforeAll -> setUpContainer()
BaseTest->>Container: start() (expose 6379, waitForListening)
JUnit->>Test: execute test method
Test->>BaseTest: getFalkordbHost()/getFalkordbPort()
Test->>Driver: driver(host, port).graph("...")
Driver-->>Test: Graph API client
Test->>Test: run assertions (@AfterEach cleanup if present)
JUnit->>BaseTest: @AfterAll -> tearDownContainer()
BaseTest->>Container: stop()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
pom.xml (1)
102-113: Keep Testcontainers artifacts in sync via BOMTo avoid accidental version drift across Testcontainers modules, consider importing the Testcontainers BOM and omitting explicit versions on Testcontainers artifacts.
Add this to dependencyManagement:
<dependencyManagement> <dependencies> <dependency> <groupId>org.testcontainers</groupId> <artifactId>testcontainers-bom</artifactId> <version>1.21.3</version> <type>pom</type> <scope>import</scope> </dependency> </dependencies> </dependencyManagement>Then remove the tags from org.testcontainers:testcontainers and org.testcontainers:junit-jupiter.
src/test/java/com/falkordb/test/BaseTestIT.java (4)
16-17: Fix typos and inconsistent naming for the container fieldThe field name “continerFalkordb” is misspelled and inconsistent. Rename it for clarity and update usages.
Apply this diff:
- @Container - protected static GenericContainer<?> continerFalkordb; + protected static GenericContainer<?> falkorDbContainer; @@ - continerFalkordb = new GenericContainer<>(FALOKRDB_IMAGE) + falkorDbContainer = new GenericContainer<>(FALKORDB_IMAGE) @@ - continerFalkordb.start(); + falkorDbContainer.start(); @@ - if (continerFalkordb != null) { - continerFalkordb.stop(); + if (falkorDbContainer != null) { + falkorDbContainer.stop(); @@ - return continerFalkordb.getFirstMappedPort(); + return falkorDbContainer.getFirstMappedPort(); @@ - return continerFalkordb.getHost(); + return falkorDbContainer.getHost();Note: This also pairs with the constant rename in the previous comment (FALKORDB_IMAGE).
Also applies to: 22-26, 30-31, 35-41
16-17: @container is ineffective without @testcontainers (and redundant with manual start/stop)Since you manually start/stop the container in @BeforeAll/@afterall and the class is not annotated with @testcontainers, the @container annotation is misleading. Remove it, or alternatively adopt the JUnit Jupiter extension and let it manage lifecycle (in which case, drop manual start/stop).
Minimal change (remove annotation only):
- @Container protected static GenericContainer<?> falkorDbContainer;Alternative (use extension-managed lifecycle):
- Add import: org.testcontainers.junit.jupiter.Testcontainers
- Annotate class with @testcontainers
- Make container static final and initialized inline
- Remove @BeforeAll/@afterall
22-24: Add readiness check to avoid race conditions on startupEven though Testcontainers usually waits for the port, being explicit improves robustness.
Add import:
+import org.testcontainers.containers.wait.strategy.Wait;And update initialization:
- falkorDbContainer = new GenericContainer<>(FALKORDB_IMAGE) - .withExposedPorts(falkordbPort) - .withLogConsumer(new Slf4jLogConsumer(log)); // Replace 6379 with Falkordb's default port if different + falkorDbContainer = new GenericContainer<>(FALKORDB_IMAGE) + .withExposedPorts(falkordbPort) + .waitingFor(Wait.forListeningPort()) + .withLogConsumer(new Slf4jLogConsumer(log));
18-18: Remove outdated inline commentThe comment implies uncertainty about the port even though falkordbPort is defined. It’s noise now.
Apply this change:
- private static final int falkordbPort = 6379; // Default port for Falkordb, adjust if necessary + private static final int falkordbPort = 6379;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pom.xml(1 hunks)src/test/java/com/falkordb/InstantiationTest.java(1 hunks)src/test/java/com/falkordb/test/BaseTestIT.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/test/java/com/falkordb/InstantiationTest.java (2)
src/test/java/com/falkordb/test/BaseTestIT.java (1)
BaseTestIT(12-42)src/main/java/com/falkordb/FalkorDB.java (1)
FalkorDB(10-57)
🔇 Additional comments (3)
pom.xml (2)
102-119: Testcontainers dependencies added — LGTMThe new test-scoped dependencies for Testcontainers and JUnit Jupiter are correctly added and scoped to tests. This aligns with the PR goal of enabling containerized integration tests.
114-119: Verify need for com.redis:testcontainers-redisThe test code uses GenericContainer and does not use the Redis-specific containers from com.redis:testcontainers-redis. If you don’t plan to use RedisContainer/RedisStackContainer, drop this dependency to keep the test classpath lean. Otherwise, consider switching BaseTestIT to use a RedisContainer if it meaningfully simplifies setup for FalkorDB.
src/test/java/com/falkordb/InstantiationTest.java (1)
21-21: Dynamic host/port — LGTMSwitching to BaseTestIT’s host/port for explicit and URI-based drivers is correct and aligns the tests with Testcontainers’ dynamic port mapping.
Also applies to: 28-28, 35-36
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (4)
src/test/java/com/falkordb/PipelineTest.java (1)
249-266: Bug: leftover default driver() will bypass the container and likely failLine 264 still uses FalkorDB.driver() which connects to localhost:6379. Under Testcontainers, use the container’s mapped host/port.
Replace:
GraphContextGenerator api2 = FalkorDB.driver().graph("social-copied");With:
GraphContextGenerator api2 = FalkorDB.driver(getFalkordbHost(), getFalkordbPort()).graph("social-copied");src/test/java/com/falkordb/GraphAPITest.java (2)
1041-1057: Bug: using FalkorDB.driver() bypasses the container and may break testsLine 1044 uses the no-arg driver(), which targets localhost:6379. Use container host/port.
Replace:
GraphContextGenerator client2 = FalkorDB.driver().graph("social-copied");With:
GraphContextGenerator client2 = FalkorDB.driver(getFalkordbHost(), getFalkordbPort()).graph("social-copied");
1071-1087: Bug: another occurrence of default driver()Same issue on Line 1073. Use the container mapping.
Replace:
GraphContextGenerator client2 = FalkorDB.driver().graph("social-copied");With:
GraphContextGenerator client2 = FalkorDB.driver(getFalkordbHost(), getFalkordbPort()).graph("social-copied");src/test/java/com/falkordb/IterableTest.java (1)
20-24: Make teardown resilient to setup failures (NPE-safe and best-effort cleanup)If createApi fails, api is null and @AfterEach will throw, obscuring the root cause. Make teardown best-effort and NPE-safe.
Apply this diff:
@AfterEach public void deleteGraph() { - api.deleteGraph(); - api.close(); + if (api != null) { + try { + api.deleteGraph(); + } catch (Exception ignored) { /* graph may not exist or already deleted */ } + try { + api.close(); + } catch (Exception ignored) { /* ignore close failures in teardown */ } + } }
🧹 Nitpick comments (10)
src/test/java/com/falkordb/test/BaseTestContainerTestIT.java (4)
14-19: Fix typos and naming for clarity and consistency
- FALOKRDB_IMAGE is misspelled (missing “R”).
- Field name continerFalkordb has a typo.
- falkordbPort is a constant; prefer ALL_CAPS.
Apply the following renames:
- public static final DockerImageName FALOKRDB_IMAGE = DockerImageName.parse("falkordb/falkordb:latest"); + public static final DockerImageName FALKORDB_IMAGE = DockerImageName.parse("falkordb/falkordb:latest"); - protected static GenericContainer<?> continerFalkordb; - private static final int falkordbPort = 6379; // Default port for Falkordb, adjust if necessary + protected static GenericContainer<?> containerFalkorDB; + private static final int FALKORDB_PORT = 6379; // Default port for FalkorDB
29-33: Update usages after renamesMake sure teardown references the renamed field.
- if (continerFalkordb != null) { - continerFalkordb.stop(); + if (containerFalkorDB != null) { + containerFalkorDB.stop();
35-41: Update accessors after renames- return continerFalkordb.getFirstMappedPort(); + return containerFalkorDB.getFirstMappedPort();- return continerFalkordb.getHost(); + return containerFalkorDB.getHost();
20-26: Container lifecycle scope: consider the singleton pattern to avoid per-class restartsWith @BeforeAll/@afterall in a base class, each subclass will start/stop a new container, increasing build time. If you want one container for the whole test suite, consider the “singleton container” pattern (start once in a static block, no tearDown), or use @testcontainers with a static @container field in a dedicated utility class.
I can provide a ready-to-use singleton variant if you want to switch.
src/test/java/com/falkordb/ListGraphsTest.java (2)
16-16: Nit: spacing after comma for readability- driver = FalkorDB.driver(getFalkordbHost(),getFalkordbPort()); + driver = FalkorDB.driver(getFalkordbHost(), getFalkordbPort());
19-66: Consider closing the driver if it holds resourcesIf Driver implements a close/cleanup, add an @AfterEach to close it. If not, ignore this.
Would you like me to check the Driver type in the repo and suggest an @AfterEach accordingly?
src/test/java/com/falkordb/PipelineTest.java (1)
21-21: Nit: spacing after comma- api = FalkorDB.driver(getFalkordbHost(),getFalkordbPort()).graph("social"); + api = FalkorDB.driver(getFalkordbHost(), getFalkordbPort()).graph("social");src/test/java/com/falkordb/TransactionTest.java (1)
28-29: Nit: spacing after comma- api = FalkorDB.driver(getFalkordbHost(),getFalkordbPort()).graph("social"); + api = FalkorDB.driver(getFalkordbHost(), getFalkordbPort()).graph("social");src/test/java/com/falkordb/GraphAPITest.java (2)
31-31: Nit: spacing after comma- client = FalkorDB.driver(getFalkordbHost(),getFalkordbPort()).graph("social"); + client = FalkorDB.driver(getFalkordbHost(), getFalkordbPort()).graph("social");
9-14: Duplicate import of AssertionsRemove the duplicate import to avoid warnings.
import com.falkordb.test.BaseTestContainerTestIT; import org.junit.jupiter.api.Assertions; -import org.junit.jupiter.api.AfterEach; -import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.AfterEach;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/test/java/com/falkordb/GraphAPITest.java(2 hunks)src/test/java/com/falkordb/InstantiationTest.java(1 hunks)src/test/java/com/falkordb/IterableTest.java(1 hunks)src/test/java/com/falkordb/ListGraphsTest.java(1 hunks)src/test/java/com/falkordb/PipelineTest.java(2 hunks)src/test/java/com/falkordb/TransactionTest.java(3 hunks)src/test/java/com/falkordb/test/BaseTestContainerTestIT.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/com/falkordb/InstantiationTest.java
🧰 Additional context used
🧬 Code Graph Analysis (5)
src/test/java/com/falkordb/ListGraphsTest.java (2)
src/test/java/com/falkordb/test/BaseTestContainerTestIT.java (1)
BaseTestContainerTestIT(12-42)src/main/java/com/falkordb/FalkorDB.java (1)
FalkorDB(10-57)
src/test/java/com/falkordb/PipelineTest.java (2)
src/test/java/com/falkordb/test/BaseTestContainerTestIT.java (1)
BaseTestContainerTestIT(12-42)src/main/java/com/falkordb/FalkorDB.java (1)
FalkorDB(10-57)
src/test/java/com/falkordb/TransactionTest.java (2)
src/test/java/com/falkordb/test/BaseTestContainerTestIT.java (1)
BaseTestContainerTestIT(12-42)src/main/java/com/falkordb/FalkorDB.java (1)
FalkorDB(10-57)
src/test/java/com/falkordb/IterableTest.java (2)
src/test/java/com/falkordb/test/BaseTestContainerTestIT.java (1)
BaseTestContainerTestIT(12-42)src/main/java/com/falkordb/FalkorDB.java (1)
FalkorDB(10-57)
src/test/java/com/falkordb/GraphAPITest.java (2)
src/test/java/com/falkordb/test/BaseTestContainerTestIT.java (1)
BaseTestContainerTestIT(12-42)src/main/java/com/falkordb/FalkorDB.java (1)
FalkorDB(10-57)
🔇 Additional comments (7)
src/test/java/com/falkordb/ListGraphsTest.java (1)
10-17: Switch to container host/port looks goodDriver initialization now uses dynamic host/port from the container. Nice.
src/test/java/com/falkordb/PipelineTest.java (1)
15-23: Good: tests now target the containerized FalkorDB instanceInitialization via getFalkordbHost()/getFalkordbPort() is correct.
src/test/java/com/falkordb/TransactionTest.java (1)
19-29: Good move to containerized host/portThe test now connects via the container. Looks correct.
src/test/java/com/falkordb/GraphAPITest.java (1)
25-33: Correctly migrated to container host/portClient initialization uses the container mapping. Good.
src/test/java/com/falkordb/IterableTest.java (3)
5-5: No-op whitespace changeNothing to review here.
6-6: Adopts container-based test infra correctlyImporting BaseTestContainerTestIT is the right move to run tests against the Testcontainers-managed FalkorDB instance.
15-18: Parallel execution isn’t enabled—no changes needed nowI searched the codebase and found:
- No
junit.jupiter.execution.parallel.enabledsetting in pom.xml or any junit-platform.properties- No
@Executionannotations in the testsBy default JUnit 5 runs tests sequentially, so using a fixed graph name (“social”) won’t cause cross-test interference today. If you do enable parallel execution in the future, you can switch to deriving a unique graph name per test (e.g. via
TestInfo) to avoid clashes.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/test/java/com/falkordb/GraphAPITest.java (2)
1044-1052: Use the same helper for client2 to keep tests consistent.Same rationale as above—use the base helper to avoid repeating host/port wiring.
- GraphContextGenerator client2 = FalkorDB.driver(getFalkordbHost(),getFalkordbPort()).graph("social-copied"); + GraphContextGenerator client2 = graph("social-copied");
1073-1086: Apply the helper here as well.This keeps the class uniform and reduces boilerplate.
- GraphContextGenerator client2 = FalkorDB.driver(getFalkordbHost(),getFalkordbPort()).graph("social-copied"); + GraphContextGenerator client2 = graph("social-copied");
🧹 Nitpick comments (3)
src/test/java/com/falkordb/GraphAPITest.java (2)
9-13: Remove duplicate import for Assertions.There are two identical
import org.junit.jupiter.api.Assertions;lines (Lines 10 and 12). Drop one to avoid compiler errors/warnings.-import org.junit.jupiter.api.Assertions;
31-31: Reduce duplication by adding a small helper in the base test.You’re repeating
FalkorDB.driver(getFalkordbHost(), getFalkordbPort()).graph(...)in multiple tests. Consider a thin helper in the base to improve readability and avoid duplication.Apply this diff here once the helper exists:
- client = FalkorDB.driver(getFalkordbHost(),getFalkordbPort()).graph("social"); + client = graph("social");Add this helper to BaseTestContainerTestIT (outside this file):
// In com.falkordb.test.BaseTestContainerTestIT protected GraphContextGenerator graph(String name) { return FalkorDB.driver(getFalkordbHost(), getFalkordbPort()).graph(name); }src/test/java/com/falkordb/exceptions/GraphErrorTest.java (1)
29-33: Harden teardown against partial setup failuresIf driver initialization fails (e.g., container not ready) and api remains null, @AfterEach will throw a NullPointerException. Guard cleanup to make teardown idempotent and safe.
Apply this diff:
- public void deleteGraph() throws Exception{ - - api.deleteGraph(); - api.close(); - } + public void deleteGraph() throws Exception { + if (api != null) { + try { + api.deleteGraph(); + } finally { + api.close(); + } + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/test/java/com/falkordb/GraphAPITest.java(4 hunks)src/test/java/com/falkordb/PipelineTest.java(3 hunks)src/test/java/com/falkordb/exceptions/GraphErrorTest.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/com/falkordb/PipelineTest.java
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/test/java/com/falkordb/exceptions/GraphErrorTest.java (1)
src/main/java/com/falkordb/FalkorDB.java (1)
FalkorDB(10-57)
src/test/java/com/falkordb/GraphAPITest.java (1)
src/main/java/com/falkordb/FalkorDB.java (1)
FalkorDB(10-57)
🔇 Additional comments (1)
src/test/java/com/falkordb/GraphAPITest.java (1)
25-25: Extending BaseTestContainerTestIT: LGTMGood move leveraging the Testcontainers base to provision host/port for the driver. This keeps environment concerns centralized.
| @BeforeEach | ||
| public void createApi() { | ||
| api = FalkorDB.driver().graph("social"); | ||
| api = FalkorDB.driver(getFalkordbHost(),getFalkordbPort()).graph("social"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid fixed graph name to prevent cross-test collisions when sharing a container
Multiple tests/classes commonly use "social". With concurrent test execution or shared containers, this can cause interference. Use a unique graph name per test to isolate state.
Apply this one-line change to generate a unique graph name per test:
- api = FalkorDB.driver(getFalkordbHost(),getFalkordbPort()).graph("social");
+ api = FalkorDB.driver(getFalkordbHost(), getFalkordbPort()).graph("social_" + System.nanoTime());📝 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.
| api = FalkorDB.driver(getFalkordbHost(),getFalkordbPort()).graph("social"); | |
| api = FalkorDB.driver(getFalkordbHost(), getFalkordbPort()).graph("social_" + System.nanoTime()); |
🤖 Prompt for AI Agents
In src/test/java/com/falkordb/exceptions/GraphErrorTest.java around line 24, the
test uses a fixed graph name "social" which can cause cross-test collisions;
change the graph creation to use a unique name per test run (for example append
a random UUID or the current timestamp or the test method/class name) so each
test uses its own isolated graph; update the line to build the graph name
dynamically (e.g., "social-" + UUID.randomUUID() or similar) when calling
FalkorDB.driver(...).graph(...).
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/test/java/com/falkordb/test/BaseTestContainerTestIT.java (1)
20-21: Don’t mix @container with manual start/stop; remove the annotation and its importYou manually manage lifecycle in @BeforeAll/@afterall. Keeping @container is misleading and may cause confusion. Also, the @container annotation has no effect without @testcontainers on the class.
Apply this diff:
-import org.testcontainers.junit.jupiter.Container; @@ - @Container private static GenericContainer<?> containerFalkorDB;Also applies to: 10-10
🧹 Nitpick comments (3)
src/test/java/com/falkordb/test/BaseTestContainerTestIT.java (3)
15-15: Mark base test as abstract to avoid accidental execution and clarify intentAs a base class, making it abstract prevents the runner from considering it as a concrete test class in some environments.
-public class BaseTestContainerTestIT { +public abstract class BaseTestContainerTestIT {
30-35: Optional: enable container reuse for faster local runsAllow opting into Testcontainers reuse via a system property; this speeds up local IT cycles when ~/.testcontainers.properties is configured.
containerFalkorDB = new GenericContainer<>(image) .withExposedPorts(FALKORDB_PORT) .withLogConsumer(new Slf4jLogConsumer(log)) .waitingFor(Wait.forListeningPort()) - .withStartupTimeout(Duration.ofSeconds(90)); + .withStartupTimeout(Duration.ofSeconds(90)) + .withReuse(Boolean.getBoolean("testcontainers.reuse"));
45-47: Prefer getMappedPort(FALKORDB_PORT) over getFirstMappedPort()This is clearer and resilient if multiple ports are exposed in the future.
- return containerFalkorDB.getFirstMappedPort(); + return containerFalkorDB.getMappedPort(FALKORDB_PORT);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pom.xml(2 hunks)src/test/java/com/falkordb/test/BaseTestContainerTestIT.java(1 hunks)
🔇 Additional comments (4)
pom.xml (2)
136-153: Failsafe IT pattern looks goodIncluding only **/*IT.java and binding to integration-test/verify phases is correct and matches your naming. No issues spotted here.
92-102: Dependencies on Maven Central and Java 8 compatibility confirmed
- org.junit.jupiter:junit-jupiter 5.13.4 and org.testcontainers:testcontainers 1.21.3 are present in Maven Central.
- junit-jupiter-api multi-release JAR ships classes compiled for Java 8 (major 52) and additional Java 9+ classes (major 53) that are ignored when running on Java 8.
- testcontainers 1.21.3 JAR contains classes for Java 7 (major 51) and Java 8 (major 52).
No further changes needed.
src/test/java/com/falkordb/test/BaseTestContainerTestIT.java (2)
30-35: Good: readiness wait and startup timeout reduce flakinessUsing Wait.forListeningPort() with a generous startup timeout is the right call for container stability.
17-18: Avoid defaulting to :latest to improve reproducibilityDefaulting to falkordb/falkordb:latest (Line 17) can introduce test flakiness when the upstream image changes. Consider pinning to a known-good tag and keeping the -Dfalkordb.image override as-is.
If you have a preferred FalkorDB image tag used in CI (e.g., the server version this client targets), we can update FALKORDB_IMAGE accordingly. Would you like me to propose that change?
| <groupId>org.testcontainers</groupId> | ||
| <artifactId>testcontainers</artifactId> | ||
| <version>1.21.3</version> | ||
| <scope>test</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.testcontainers</groupId> | ||
| <artifactId>junit-jupiter</artifactId> | ||
| <version>1.21.3</version> | ||
| <scope>test</scope> | ||
| </dependency> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Manage Testcontainers versions via a property/BOM for consistency
Define a single testcontainers.version and import the Testcontainers BOM so all modules stay aligned. This avoids accidental version drift across artifacts.
Apply these diffs:
Add a property (Lines 69-72):
<properties>
<maven.compiler.source>8</maven.compiler.source>
<maven.compiler.target>8</maven.compiler.target>
+ <testcontainers.version>1.21.3</testcontainers.version>
</properties>Import the BOM (add after properties; new block):
<dependencyManagement>
<dependencies>
<dependency>
<groupId>org.testcontainers</groupId>
<artifactId>testcontainers-bom</artifactId>
<version>${testcontainers.version}</version>
<type>pom</type>
<scope>import</scope>
</dependency>
</dependencies>
</dependencyManagement>Drop explicit versions for Testcontainers artifacts (Lines 104-114):
<dependency>
<groupId>org.testcontainers</groupId>
<artifactId>testcontainers</artifactId>
- <version>1.21.3</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.testcontainers</groupId>
<artifactId>junit-jupiter</artifactId>
- <version>1.21.3</version>
<scope>test</scope>
</dependency>Note: keep the version for com.redis:testcontainers-redis; it’s not governed by the Testcontainers BOM.
🤖 Prompt for AI Agents
In pom.xml around lines 69-72 and 104-114, you should centralize Testcontainers
versioning: add a new property testcontainers.version (around lines 69-72) and
import the Testcontainers BOM via a dependencyManagement block immediately after
properties, then remove the explicit <version> tags from Testcontainers
dependencies at lines 104-114 so they inherit the BOM-managed version; keep
explicit versions for artifacts not covered by the BOM (e.g.,
com.redis:testcontainers-redis).
| <groupId>com.redis</groupId> | ||
| <artifactId>testcontainers-redis</artifactId> | ||
| <version>2.2.4</version> | ||
| <scope>test</scope> | ||
| </dependency> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm whether com.redis:testcontainers-redis is actually used
I don’t see usages in the provided diff; if unused, drop it to keep the test classpath lean.
Run this to check for usage:
🏁 Script executed:
#!/bin/bash
# Look for classes from com.redis.testcontainers or typical container names
rg -n -A2 -i $'package com\\.redis\\.testcontainers|Redis(Container|StackContainer)|com\\.redis\\.testcontainers' src/test || trueLength of output: 126
We'll broaden the search for any Redis-related usage in tests and code:
🏁 Script executed:
#!/bin/bash
# Search for any mention of 'redis' in test sources
rg -n -i 'redis' -A2 src/test || true
# Search for RedisContainer instantiations anywhere in the repo
rg -n -i 'RedisContainer' -A2 src || trueLength of output: 6297
Unused testcontainers-redis dependency
I couldn’t find any references to com.redis.testcontainers.RedisContainer or other classes from com.redis:testcontainers-redis in the test code. It appears this dependency isn’t used and can be removed to slim down the test classpath.
• In pom.xml, remove lines 116–120 (the <dependency> block for com.redis:testcontainers-redis:2.2.4).
🤖 Prompt for AI Agents
In pom.xml around lines 116 to 120, the com.redis:testcontainers-redis
dependency block is unused; remove the entire <dependency>...</dependency> block
(lines 116–120) from the test dependencies section, then run mvn -q
dependency:tree or mvn -q test to validate build and ensure no tests reference
Redis testcontainers; if build fails, restore the block and search tests for
com.redis.* usages to resolve references.
| <plugin> | ||
| <artifactId>maven-surefire-plugin</artifactId> | ||
| <configuration> | ||
| <excludes> | ||
| <exclude>**/*IT.java</exclude> | ||
| </excludes> | ||
| </configuration> | ||
| </plugin> | ||
| <!-- Failsafe: run ITs --> |
There was a problem hiding this comment.
Consolidate duplicate surefire plugin declarations; excludes may be ignored
You declare maven-surefire-plugin twice. Maven merges configs in surprising ways; the second declaration (Lines 155-158) can override the first, causing your IT exclude (Lines 129-131) to be dropped. Keep a single declaration with version and configuration.
Apply this diff to merge into one surefire plugin and ensure ITs are excluded from unit phase:
- <!-- Surefire: skip ITs -->
- <plugin>
- <artifactId>maven-surefire-plugin</artifactId>
- <configuration>
- <excludes>
- <exclude>**/*IT.java</exclude>
- </excludes>
- </configuration>
- </plugin>
+ <!-- Surefire: skip ITs -->
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-surefire-plugin</artifactId>
+ <version>3.5.3</version>
+ <configuration>
+ <excludes>
+ <exclude>**/*IT.java</exclude>
+ </excludes>
+ </configuration>
+ </plugin>
@@
- <plugin>
- <groupId>org.apache.maven.plugins</groupId>
- <artifactId>maven-surefire-plugin</artifactId>
- <version>3.5.3</version>
- </plugin>
+ <!-- (removed duplicate surefire declaration) -->Also applies to: 155-158
🤖 Prompt for AI Agents
In pom.xml around lines 126-134 and 155-158 you have two maven-surefire-plugin
declarations which can cause Maven to merge/override configurations and drop the
exclude for **/*IT.java; consolidate into a single surefire plugin entry (keep a
single <plugin> element) that includes the plugin version and the
<configuration><excludes><exclude>**/*IT.java</exclude></excludes></configuration>,
remove the duplicate declaration, and ensure the failsafe plugin remains
configured separately to run ITs in the integration-test phase.
| private static final int falkordbPort = 6379; // Default port for Falkordb, adjust if necessary | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused duplicate port constant
The private static falkordbPort (Line 22) duplicates FALKORDB_PORT and is unused.
- private static final int falkordbPort = 6379; // Default port for Falkordb, adjust if necessary📝 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.
| private static final int falkordbPort = 6379; // Default port for Falkordb, adjust if necessary |
🤖 Prompt for AI Agents
In src/test/java/com/falkordb/test/BaseTestContainerTestIT.java around lines 22
to 23, there is an unused duplicate constant private static final int
falkordbPort = 6379 that duplicates FALKORDB_PORT; remove this unused field and
update any references to use the existing FALKORDB_PORT constant (or keep only
the single canonical constant) to eliminate redundancy and unused code.
|
@drr00t thank you very much for the PR! |
2 participants
This is a PR with test-container integration proposal: #161
Summary by CodeRabbit
Tests
Chores