Feat/testing the full image before publishing - #84
Conversation
📝 WalkthroughWalkthroughAdds Testcontainers-based integration tests, two test-scoped Testcontainers dependencies, and updates Dockerfile to copy a fixed artifact name ( Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Runner
participant TC as Testcontainers (Docker)
participant App as App (inside Container)
participant HTTP as HttpClient
Test->>TC: start container from local image (mount project)
TC->>App: initialize server, expose 8080, wait for HTTP 200 on "/"
Note right of TC: readiness check
HTTP->>App: GET "/"
App-->>HTTP: 200 + index HTML
HTTP->>Test: return response
HTTP->>App: GET "/not-found.html"
App-->>HTTP: 404 + body "404"
HTTP->>Test: return response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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 (2)
src/test/java/org/juv25d/AppIT.java (2)
27-27: MakeHttpClientstatic to share it across test instances.JUnit 5 creates a new
AppITinstance per test method, soHttpClient.newHttpClient()is called twice. SinceHttpClientis designed to be reused, making itstaticeliminates the redundant allocation and any internal thread-pool overhead.♻️ Proposed refactor
- private final HttpClient client = HttpClient.newHttpClient(); + private static final HttpClient client = HttpClient.newHttpClient();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/juv25d/AppIT.java` at line 27, Make the HttpClient instance shared across test instances by changing the non-static field client in the AppIT class to a static field: locate the declaration "private final HttpClient client = HttpClient.newHttpClient();" in class AppIT and modify it to be static so the HttpClient is created once and reused across JUnit 5 test instances; ensure other uses of client in the class remain unchanged.
24-24: Consider adding a.dockerignoreto reduce the build context size.
withFileFromPath(".", Paths.get("."))transfers the entire project directory — includingtarget/,.git/, and any other generated artifacts — into the Docker daemon as the build context. This can significantly slow down the image build step, especially in CI.A
.dockerignoreat the project root (e.g., excludingtarget/,.git/,*.md) keeps the context lean without affecting theCOPY pom.xmlandCOPY srcsteps inside the Dockerfile.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/juv25d/AppIT.java` at line 24, The test uses withFileFromPath(".", Paths.get(".")) which sends the entire repo as Docker build context; add a .dockerignore at project root (exclude target/, .git/, .idea/, *.class, *.log, etc.) so the build context is small and CI builds are faster, then keep using withFileFromPath(".", Paths.get(".")) unchanged; ensure .dockerignore still allows necessary files (pom.xml and src/) by not listing them.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pom.xml`:
- Around line 43-54: The pom references invalid Testcontainers versions; update
the dependency versions for org.testcontainers:testcontainers and
org.testcontainers:junit-jupiter to the existing release 1.21.3 by changing the
<version> elements for the artifactId testcontainers and junit-jupiter to 1.21.3
so Maven can resolve the artifacts.
In `@src/test/java/org/juv25d/AppIT.java`:
- Around line 22-25: The Testcontainers GenericContainer "server" is missing a
readiness wait and can return connection-refused; update the container
definition that builds with ImageFromDockerfile and withExposedPorts(8080) to
include a waitingFor strategy (e.g., Wait.forHttp("/") or
Wait.forListeningPort() with an appropriate forStatusCode and a startup timeout
via Duration) so Testcontainers waits until the Java HTTP server is actually
accepting connections on port 8080 before tests proceed; apply this to the
static "server" variable initialization.
---
Nitpick comments:
In `@src/test/java/org/juv25d/AppIT.java`:
- Line 27: Make the HttpClient instance shared across test instances by changing
the non-static field client in the AppIT class to a static field: locate the
declaration "private final HttpClient client = HttpClient.newHttpClient();" in
class AppIT and modify it to be static so the HttpClient is created once and
reused across JUnit 5 test instances; ensure other uses of client in the class
remain unchanged.
- Line 24: The test uses withFileFromPath(".", Paths.get(".")) which sends the
entire repo as Docker build context; add a .dockerignore at project root
(exclude target/, .git/, .idea/, *.class, *.log, etc.) so the build context is
small and CI builds are faster, then keep using withFileFromPath(".",
Paths.get(".")) unchanged; ensure .dockerignore still allows necessary files
(pom.xml and src/) by not listing them.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/test/java/org/juv25d/AppIT.java (2)
21-27: LGTM — wait strategy is now in place.The
.waitingFor(Wait.forHttp("/").forStatusCode(200))on line 27 correctly addresses the previously raised concern about race conditions between container RUNNING state and the server being ready to accept connections.💡 Optional: add an explicit startup timeout for slow CI environments
The default Testcontainers startup timeout is 60 seconds. In resource-constrained CI runners (especially on first run, when the base image needs to be pulled), that can be tight. An explicit ceiling avoids a hard-to-diagnose timeout failure:
).withExposedPorts(8080) - .waitingFor(Wait.forHttp("/").forStatusCode(200)); + .waitingFor(Wait.forHttp("/").forStatusCode(200) + .withStartupTimeout(java.time.Duration.ofMinutes(2)));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/juv25d/AppIT.java` around lines 21 - 27, Add an explicit startup timeout to the Testcontainers GenericContainer "server" to avoid flaky failures in slow CI; update the container configuration for the static field server (GenericContainer<?> server) to chain a withStartupTimeout(Duration.ofSeconds(...)) call with an appropriate ceiling (e.g., 120s) and ensure java.time.Duration is imported so the wait strategy (Wait.forHttp("/").forStatusCode(200)) has a longer overall startup window.
29-29: Consider astaticHttpClientto avoid per-test-instance allocation.Containers declared as static fields are started only once before any test method is executed and stopped after the last test method has executed. The
HttpClientis stateless and safe to share across tests, so creating a new instance per test-instance (JUnit Jupiter's defaultPER_METHODlifecycle) allocates unnecessary objects. In Java 21+,HttpClientalso implementsAutoCloseable— none of the instances here are ever closed.♻️ Proposed refactor
- private final HttpClient client = HttpClient.newHttpClient(); + private static final HttpClient client = HttpClient.newHttpClient();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/juv25d/AppIT.java` at line 29, The HttpClient in AppIT is created per test-instance which is unnecessary and never closed; change the instance field "private final HttpClient client" to a shared static client (e.g., "private static HttpClient client") and initialize it once for the test class, and if you want to honor Java 21's AutoCloseable behavior, create it in a `@BeforeAll` (or static initializer) and close it in an `@AfterAll` method so the single HttpClient is reused across tests and properly cleaned up.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/test/java/org/juv25d/AppIT.java`:
- Around line 4-8: Update the JUnit Jupiter Testcontainers dependency in pom.xml
to match Testcontainers 2.0.3: replace the old artifact declaration using
groupId/org.testcontainers and artifactId/junit-jupiter (version 1.21.4) with
the new artifactId testcontainers-junit-jupiter and version 2.0.3, keeping scope
test; ensure the groupId remains org.testcontainers so the project uses the
correct Testcontainers JUnit 5 module compatible with the core 2.0.3 APIs.
---
Nitpick comments:
In `@src/test/java/org/juv25d/AppIT.java`:
- Around line 21-27: Add an explicit startup timeout to the Testcontainers
GenericContainer "server" to avoid flaky failures in slow CI; update the
container configuration for the static field server (GenericContainer<?> server)
to chain a withStartupTimeout(Duration.ofSeconds(...)) call with an appropriate
ceiling (e.g., 120s) and ensure java.time.Duration is imported so the wait
strategy (Wait.forHttp("/").forStatusCode(200)) has a longer overall startup
window.
- Line 29: The HttpClient in AppIT is created per test-instance which is
unnecessary and never closed; change the instance field "private final
HttpClient client" to a shared static client (e.g., "private static HttpClient
client") and initialize it once for the test class, and if you want to honor
Java 21's AutoCloseable behavior, create it in a `@BeforeAll` (or static
initializer) and close it in an `@AfterAll` method so the single HttpClient is
reused across tests and properly cleaned up.
simonforsberg
left a comment
simonforsberg
left a comment
There was a problem hiding this comment.
Nice addition! It’s great that we’re testing the actual server end-to-end instead of only using unit tests.
3 participants
Summary
This PR introduces integration testing framework using Testcontainers. It allows the project to automatically build the Docker image and run real HTTP requests against the server in a containerized environment before publishing.
A Key benefit using Testcontainers is taht all Docker containers and resoruces are automatically cleaned up after the tests finish, ensuring no stale containers are left running on the developer machine.
Changes
Test plan
Run the following command to verify the full build and integrations tests:
./mvnw verify -Dfailsafe.useFile=falseVerify that the AppIT starts the Docker container, all tests pass with a BUILD SUCCESS, and the container is automatically removed.
Keep in mind that docker needs to be running on computer when running the above command.
Summary by CodeRabbit
Tests
Chores