Skip to content

[Solace] Fix flaky SolaceIO integration tests - #40740

Open
schizophrenicmaniac wants to merge 2 commits into
apache:masterfrom
schizophrenicmaniac:fix/40734-the-precommit-java-solace-io-direct-job
Open

schizophrenicmaniac wants to merge 2 commits into
apache:masterfrom
schizophrenicmaniac:fix/40734-the-precommit-java-solace-io-direct-job

Conversation

@schizophrenicmaniac

Copy link
Copy Markdown
Contributor

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 with getMappedPort after 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, SolaceIOMultipleSempIT and SolaceIOCustomSessionServiceFactoryIT.

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.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📋 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 addFixedExposedPort with ServerSocket(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.waitForCounterAndCancel prevents 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();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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).

@github-actions

Copy link
Copy Markdown
Contributor

Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment assign set of reviewers

@github-actions github-actions Bot added the build label Oct 23, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Assigning reviewers:

R: @Abacn for label java.
Backup reviewer: kennknowles (if the main reviewer is unresponsive, you can escalate to them by tagging their username)

Note: If you would like to opt out of this review, comment assign to next reviewer.

Available commands:

  • stop reviewer notifications - opt out of the automated review tooling
  • remind me after tests pass - tag the comment author after tests pass
  • waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

@codecov

codecov Bot commented Oct 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 0.00%. Comparing base (a8d6afd) to head (84c3c5c).
⚠️ Report is 53 commits behind head on master.

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     
Flag Coverage Δ
python ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@aIbrahiim

Copy link
Copy Markdown
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The PreCommit Java Solace IO Direct job is flaky

3 participants