Repository navigation
[Solace] Fix flaky SolaceIO integration tests - #40740
schizophrenicmaniac wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
📋 Review Summary
This PR effectively addresses three sources of flakiness in the Solace IO integration tests: port allocation race conditions, fixed-duration sleep timeouts during unbounded reads, and unverified SEMP setup commands. The changes are clean, well-isolated to test utilities, and properly cancel running pipelines after assertions complete.
🔍 General Feedback
- Switching from
addFixedExposedPortwithServerSocket(0)to standard Testcontainers dynamic port mapping (getMappedPort) eliminates the classic port-binding race condition. - Polling the metric counter and explicitly cancelling the unbounded read pipeline in
SolaceTestUtils.waitForCounterAndCancelprevents both premature timeouts on slow CI runners and leaked background pipelines. - Two minor suggestions are noted below regarding container port exposure and exception handling when cancelling pipelines.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
| break; | ||
| } | ||
| } | ||
| pipelineResult.cancel(); |
There was a problem hiding this comment.
🟡 Medium: If an exception is thrown inside the while loop (e.g., from waitUntilFinish or getCounterMetric), pipelineResult.cancel() will not be called, leaving the unbounded streaming pipeline running in the JVM. Consider wrapping the polling loop in a try ... finally block so the pipeline is always cancelled if it hasn't reached a terminal state.
| .withVpn(VPN_NAME) | ||
| .withCredentials(USERNAME, PASSWORD) | ||
| .withTopic(TOPIC_NAME, Service.SMF) | ||
| .withExposedPorts(SEMP_PORT, JCSMP_PORT) |
There was a problem hiding this comment.
🟢 Low: GenericContainer.withExposedPorts(...) overwrites the container's exposed ports list rather than appending to it, which replaces the port added by .withTopic(TOPIC_NAME, Service.SMF) on the line above. While it works here because JCSMP_PORT (55555) is the same as Service.SMF.getPort(), SolaceContainer already exposes port 8080 (SEMP_PORT) in its constructor and withTopic(..., Service.SMF) already calls addExposedPort(55555), so .withExposedPorts(SEMP_PORT, JCSMP_PORT) is redundant (or addExposedPorts could be used if you want to be explicit without overwriting).
|
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
|
Assigning reviewers: R: @Abacn for label java. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #40740 +/- ##
===========================================
- Coverage 80.04% 0 -80.05%
===========================================
Files 490 0 -490
Lines 82916 0 -82916
===========================================
- Hits 66373 0 -66373
+ Misses 16543 0 -16543
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
LGTM but I checked the actual failure logs and the root cause is in run Solace IO build script with `Unsupported class file major version 69 so the java-version default pin is enough to unblock so why add the test changes (ports / read poll / SEMP retries)? @schizophrenicmaniac |
Fixes #40734
The Solace IO Direct precommit fails often because of how the integration tests are set up. This PR changes three things.
The container used fixed host ports from
findAvailablePort(). That opens a socket on port 0, closes it, and only binds the port when the container starts, so something else can take the port in between. The ports are now normally exposed, and the mapped ports are read withgetMappedPortafter the container starts.The read tests waited a fixed 15 seconds and then checked the counter. On a slow runner that can be too short, and since the read never finishes, the pipeline was left running in the test JVM afterwards. A small helper now polls the read counter until it reaches the expected count, with a 2 minute limit, and then cancels the pipeline. This applies to
SolaceIOIT,SolaceIOMultipleSempITandSolaceIOCustomSessionServiceFactoryIT.Creating the queue and its subscription through SEMP only logged errors, and curl exits 0 on HTTP errors anyway. If SEMP wasn't ready yet, the setup kept going and the read test later failed with a wrong count. Those two calls now use
curl --fail, retry for up to 30 seconds, and fail the setup with a clear error if they still don't succeed.These are test-only changes. I couldn't run the ITs locally since I don't have Docker on this machine, so I'm relying on this PR's precommit run, ideally re-run a few times. Unit tests, checkstyle and spotbugs pass for the module.