QA: Implement Testcontainers #77#84
Conversation
|
Fix #77 |
omatheusmesmo
left a comment
There was a problem hiding this comment.
Pull Request #84 Review: Implement Testcontainers for Integration Testing
Hello @Gustavo-Felix! Thank you for the excellent work on this PR. Implementing Testcontainers is a major step forward for ShoppMate, ensuring our integration tests run against a real PostgreSQL environment.
I have validated your original implementation logic, and it works as expected. However, I identified a critical compatibility issue with modern Docker Engines (like the one in my environment) and would like to suggest some architectural improvements to align with our project standards.
1. Version Upgrade: Testcontainers 2.0.3
Your original PR used version 2.0.2 (which seemed to have some metadata issues) and triggered a Docker API version mismatch (client version 1.32 is too old. Minimum supported API version is 1.44).
- Validation: I updated the dependencies to Testcontainers 2.0.3. This version correctly negotiates the API with modern Docker Servers (API 1.44+).
- Recommendation: Use the
testcontainers-bomto manage versions consistently across all modules.
2. Package Structure Alignment (Architectural Suggestion)
Currently, the integration tests are in a global IntegrationTests package. To maintain our feature-based structure:
- Recommendation: Move the controller tests to their respective feature packages. For example,
CategoryControllerWithIntegrationTestshould be incom.omatheusmesmo.shoppmate.category.controller(undersrc/test/java). - Recommendation: Move
AbstractIntegrationTestto a shared package likecom.omatheusmesmo.shoppmate.shared.testcontainers.
3. JWE Integration and Constructor Injection
I noticed that TestUserFactory uses field injection (@Autowired) and manually persists users to generate tokens.
- JWE Alignment: We should use our real
JwtService(JWE implementation) directly in tests. SinceJwtServiceonly needs aUserDetailsobject to sign a token, we can generate tokens without always hitting the database, making tests faster and more decoupled. - Constructor Injection: To stay consistent with our clean code standards, please refactor test components (like
TestUserFactory) to use constructor-based injection.
4. Security Configuration (CSRF)
The addition of CsrfCookieFilter and the explicit activation of CSRF protection is great! It ensures our tests simulate a production-hardened environment for SPAs. No risks were identified in the security changes.
Final Validation Result
The original logic is sound and the tests pass perfectly once the library versions are adjusted. By applying the suggested refactorings, the PR will not only provide better QA but also maintain high architectural consistency.
Great job! 🚀
|
🤖 Hi @Gustavo-Felix, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
|
🤖 Hi @Gustavo-Felix, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
|
🤖 I'm sorry @Gustavo-Felix, but I was unable to process your request. Please see the logs for more details. |
|
🤖 I'm sorry @Gustavo-Felix, but I was unable to process your request. Please see the logs for more details. |
|
🤖 Hi @Gustavo-Felix, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
|
🤖 Hi @Gustavo-Felix, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
|
🤖 I'm sorry @Gustavo-Felix, but I was unable to process your request. Please see the logs for more details. |
omatheusmesmo
left a comment
There was a problem hiding this comment.
Hello @Gustavo-Felix! Thank you for the updates. I've reviewed the changes you made based on our previous feedback, and I can see that the infrastructure is much more aligned with our project standards now.
Great job on applying the following:
- Testcontainers BOM (2.0.3): Versioning is now handled correctly and centrally.
- Package Structure: The reorganization into feature packages is perfect.
- Constructor Injection: The
TestUserFactoryis much cleaner now. - CSRF Implementation: Security configuration is now reflecting our production needs.
⚠️ Remaining Issue: Integration Test Failure
Despite the improvements in the structure, we still have a failing test that is blocking the CI:
Error:
expected: <Book Putted> but was: <Food>intestGetAllCategories
Technical Root Cause:
The root cause of this failure is test interdependency and lack of isolation. Although the infrastructure is ready, the test logic itself is still fragile:
- Assertion by Index: Using
categories.get(0)assumes that the list will always be in a specific order and that no other data (like<Food>from a migration) exists. - Order Dependency (
@Order): Relying on one test to set the state for the next is a risky pattern. If any test in the chain fails, it causes a "domino effect" on the others.
🛠️ Final Recommendation: Test Independence (F.I.R.S.T.)
To finalize this PR, we should follow the "Independent" principle of F.I.R.S.T. tests:
- Remove
@Order: Tests should be able to run in any sequence (or in parallel) without failing. - Isolation: Use
@BeforeEachto clean the repository (e.g.,repository.deleteAll()) or annotate the class with@Transactionalso each test rolls back its changes. - Self-Contained Data: Each test should create exactly the data it needs to validate its logic, rather than relying on the result of the previous method.
Suggested Adjustment:
Update the assertion to check for the existence of the item in the list, rather than its position:
// Instead of categories.get(0)
assertTrue(categories.stream().anyMatch(c -> c.name().equals("Book Putted")));🚀 Conclusion
We are almost there! Once we decouple the test logic and ensure each method is independent, this PR will be 100% solid and ready for merging.
Thanks for the persistence and the great work so far! 👏
|
🤖 Hi @Gustavo-Felix, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
|
🤖 I'm sorry @Gustavo-Felix, but I was unable to process your request. Please see the logs for more details. |
|
🤖 Hi @Gustavo-Felix, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
|
🤖 I'm sorry @Gustavo-Felix, but I was unable to process your request. Please see the logs for more details. |
omatheusmesmo
left a comment
There was a problem hiding this comment.
Great job @Gustavo-Felix , thanks for your effort! Wonderful first PR 🎉
|
Hi @Gustavo-Felix! I’ve analyzed the CI logs for the failure in Backend Build and Test. While the logs show GitHub Runners often have port To fix this and make the CI 'green', we should move to Files to modify:
This change will ensure the tests are fully isolated and resilient to the CI environment. Great work on the Testcontainers setup otherwise! 🚀"
|
…se LocalServerPort
|
🤖 Hi @Gustavo-Felix, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
|
🤖 I'm sorry @Gustavo-Felix, but I was unable to process your request. Please see the logs for more details. |
|
🤖 Hi @Gustavo-Felix, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
|
🤖 I'm sorry @Gustavo-Felix, but I was unable to process your request. Please see the logs for more details. |
|
Hi @Gustavo-Felix! Great progress with the The Action required:
Why this works: Once these files are added, the CI should finally turn green! 🚀 |
|
🤖 Hi @Gustavo-Felix, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
|
🤖 Hi @Gustavo-Felix, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
|
🤖 I'm sorry @Gustavo-Felix, but I was unable to process your request. Please see the logs for more details. |
|
🤖 Hi @Gustavo-Felix, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
|
🤖 I'm sorry @Gustavo-Felix, but I was unable to process your request. Please see the logs for more details. |
|
🤖 Hi @Gustavo-Felix, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
|
🤖 I'm sorry @Gustavo-Felix, but I was unable to process your request. Please see the logs for more details. |
fixes #77
Description
Currently, integration tests rely on mocked or local configurations, which may not fully represent real infrastructure behavior. To improve test reliability, consistency, and confidence, we should introduce Testcontainers to run integration tests against real, isolated services using Docker.
Resolution (What was done)
Dependency Management: Added Testcontainers dependencies to the project to support container orchestration during the test lifecycle.
Reusable Infrastructure: Configured reusable containers (database) to ensure each test suite runs in a clean, isolated environment.
Standardization: Applied the new integration test strategy across the project, replacing mocks with real service instances.
How to Test
Prerequisite: Ensure Docker is installed and running on your machine.
Execution: Run your standard build command
./mvnw verify.Log Validation: Observe the console logs to confirm the automatic startup of the containers (e.g., Creating container for image: postgres:18).
Verification: Confirm that all integration tests pass using the real connection established by Testcontainers.
Acceptance Criteria
Testcontainers dependencies are correctly integrated.
Database containers start and stop automatically during test execution.
Integration tests run successfully using Testcontainers.