Use Testcontainers for API integration tests #236
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
Modernizes integration tests by replacing hardcoded relay dependencies with Testcontainers, enabling automated Docker-based test environments. This improves test reliability and removes the need for manual relay setup.
- Replaced hardcoded relay URIs with dynamic Testcontainers-managed relay instances
- Added Testcontainers dependency and configuration for nostr-rs-relay Docker image
- Updated documentation to reflect Docker requirement instead of manual relay setup
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pom.xml | Added Testcontainers dependency version property |
| nostr-java-api/pom.xml | Added Testcontainers JUnit Jupiter dependency |
| nostr-java-api/src/test/resources/relay-container.properties | Configured default relay container image |
| nostr-java-api/src/test/java/nostr/api/integration/BaseRelayIntegrationTest.java | Implemented Testcontainers infrastructure with dynamic relay URI generation |
| nostr-java-api/src/test/java/nostr/api/unit/NIP99ImplTest.java | Fixed test assertion to avoid object equality issues |
| Multiple integration test files | Replaced hardcoded relay URIs with dynamic relay URI method calls |
| README.md | Updated documentation to reflect Docker requirement for integration tests |
| relayUri = String.format("ws://localhost:%d", RELAY.getMappedPort(RELAY_PORT)); | ||
| } | ||
|
|
||
| @DynamicPropertySource | ||
| static void registerRelayProperties(DynamicPropertyRegistry registry) { | ||
| relayUri = String.format("ws://localhost:%d", RELAY.getMappedPort(RELAY_PORT)); |
There was a problem hiding this comment.
The relayUri is being set in both ensureDockerAvailable() and registerRelayProperties() methods. This creates a race condition where the value might be overwritten, and the container might not be started when getMappedPort() is called in ensureDockerAvailable().
| relayUri = String.format("ws://localhost:%d", RELAY.getMappedPort(RELAY_PORT)); | |
| } | |
| @DynamicPropertySource | |
| static void registerRelayProperties(DynamicPropertyRegistry registry) { | |
| relayUri = String.format("ws://localhost:%d", RELAY.getMappedPort(RELAY_PORT)); | |
| } | |
| @DynamicPropertySource | |
| static void registerRelayProperties(DynamicPropertyRegistry registry) { |
Copilot uses AI. Check for mistakes.
|
|
||
| @DynamicPropertySource | ||
| static void registerRelayProperties(DynamicPropertyRegistry registry) { | ||
| relayUri = String.format("ws://localhost:%d", RELAY.getMappedPort(RELAY_PORT)); |
There was a problem hiding this comment.
Duplicate assignment of relayUri. The @DynamicPropertySource method should not reassign the static field as it may be called multiple times and creates inconsistency with the @BeforeAll method.
| relayUri = String.format("ws://localhost:%d", RELAY.getMappedPort(RELAY_PORT)); |
Copilot uses AI. Check for mistakes.
| import org.springframework.test.context.DynamicPropertyRegistry; | ||
| import org.springframework.test.context.DynamicPropertySource; |
There was a problem hiding this comment.
[nitpick] Spring Framework imports are being used in a class that doesn't appear to be a Spring test. Consider using pure Testcontainers approach without Spring dependencies if Spring context is not required.
| import org.springframework.test.context.DynamicPropertyRegistry; | |
| import org.springframework.test.context.DynamicPropertySource; |
Copilot uses AI. Check for mistakes.
Summary
nostr-rs-relayTestcontainerrelay-container.propertiesTesting
mvn -q verify(fails: Could not find a valid Docker environment)https://chatgpt.com/codex/tasks/task_b_688a9b0841408331ad01ccb463386a41