CAMEL-24049: Fix flaky JMS Spring tests - queue name isolation and readiness#24660
CAMEL-24049: Fix flaky JMS Spring tests - queue name isolation and readiness#24660gnodet wants to merge 1 commit into
Conversation
…adiness checks - Rename hardcoded queue names (foo, bar) to class-scoped names in JmsSpringLoadBalanceFailOverIT.xml to prevent cross-test contamination - Add route IDs to Spring XML routes for readiness check targeting - Add JmsTestHelper.waitForJmsConsumerRoutes() calls to ensure JMS consumers are fully subscribed before sending messages - Replace bare MockEndpoint.assertIsSatisfied() with timed assertions - Fix queue name mismatch in RouteIdTransactedIT (was using old class name) Affected tests: JmsSpringLoadBalanceFailOverJMSIT, RouteIdTransactedIT, JmsRouteUsingSpringIT, JmsRouteUsingSpringAndJmsNameIT, JmsRouteUsingSpringJMSTemplateIT, JmsRouteUsingSpringWithAutoWireIT Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
gnodet
left a comment
There was a problem hiding this comment.
Claude Code review on behalf of @gnodet
Summary
Well-structured fix for flaky JMS Spring tests, addressing two distinct root causes:
-
Queue name collisions -- Generic queue names (
foo,bar) inJmsSpringLoadBalanceFailOverIT.xmlcould be contaminated by other tests sharing the same Artemis broker. Renaming toJmsSpringLoadBalanceFailOver.foo/.bareliminates this. Similarly,RouteIdTransactedITwas still using the old class nameRouteIdTransactedTestas the queue name -- a leftover from a class rename. -
JMS consumer readiness races -- Messages sent before JMS consumers are fully subscribed to the broker get lost. Adding
JmsTestHelper.waitForJmsConsumerRoutes()(which uses Awaitility internally) ensures consumers are registered before tests send messages.
Review Notes
Testing conventions compliance (CLAUDE.md):
- No
Thread.sleep()introduced -- the PR correctly usesJmsTestHelper.waitForJmsConsumerRoutes()which delegates to Awaitility. MockEndpoint.assertIsSatisfied(context, 30, TimeUnit.SECONDS)is correctly used instead of wrapping with Awaitility (per CLAUDE.md: "Do NOT wrap MockEndpoint assertions with Awaitility").- The readiness check in
JmsRouteTest.setUpRequirements()(@BeforeEach) runs after the context is started viaTransientCamelContextExtension, so the route uptime check is valid.
Queue name isolation:
JmsSpringLoadBalanceFailOverIT.xml:foo/barrenamed toJmsSpringLoadBalanceFailOver.foo/.bar-- prevents cross-test contamination.RouteIdTransactedIT: queue name corrected fromRouteIdTransactedTesttoRouteIdTransactedITto match the current class name.
Route IDs:
- Added
routeId("jmsRoute1")androuteId("jmsRoute2")inJmsRouteTest.createRouteBuilder()to support readiness checks. These IDs are per-CamelContextso no collision risk across test instances. - Added
id="loadBalanceRoute",id="fooConsumerRoute",id="barConsumerRoute"in the Spring XML -- correctly targeted by thewaitForJmsConsumerRoutes()call.
Git history check:
- Prior fixes (commits
bcccded6e33ande38a0788492) increased the MockEndpoint timeout but did not address the queue name mismatch or consumer readiness -- this PR correctly layers the remaining fixes on top. - The prior fix
05aed0bd195forJmsSpringLoadBalanceFailOverJMSITadded timeout and@Timeout(60)but did not isolate queue names or add readiness checks.
Scanner coverage: No static analysis tools (PMD, Checkstyle, semgrep) are available in this environment. Rely on CI for static analysis results.
LGTM -- no blocking issues found.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 1 tested, 0 compile-only — current: 12 all testedMaveniverse Scalpel detected 1 affected modules (current approach: 12). Modules only in current approach (11)
Skip-tests mode would test 1 modules (1 direct + 0 downstream), skip tests for 0 (generated code, meta-modules) Modules Scalpel would test (1)
All tested modules (12 modules)
|
oscerd
left a comment
There was a problem hiding this comment.
Reviewed against the project's async-testing rules with particular attention to the base-class change — clean, no change requests.
The one structural risk in this diff was adding the jmsRoute1/jmsRoute2 readiness wait to JmsRouteTest.setUpRequirements() (a @BeforeEach inherited by subclasses): if any subclass replaced the routes without those IDs, the wait would time out. Verified it's safe — JmsRouteTest is abstract and both concrete subclasses (JmsRouteWithObjectMessageTest, JmsRouteUsingSpringIT) inherit createRouteBuilder() unchanged; the Spring variant's XML (jmsRouteUsingSpring.xml) contributes only the ActiveMQ component bean, no routes, so the ID'd routes come from the same inherited builder via the @RouteFixture in AbstractJMSTest. Both subclasses therefore get the readiness wait for free, which is a nice side effect.
Other checks, all fine:
waitForJmsConsumerRoutesis Awaitility-based with a hardatMostcap, and treats an unknown route id as not-yet-ready — so a future misconfigured id surfaces as a clear timeout rather than an NPE. NoThread.sleep, no Awaitility-wrapping ofassertIsSatisfied.- The
resultEndpoint.assertIsSatisfied()→MockEndpoint.assertIsSatisfied(context, 30, TimeUnit.SECONDS)migrations follow the prescribed timed-assertion pattern. - The
RouteIdTransactedTest→RouteIdTransactedITqueue rename is consistent across all three occurrences (both sends and the consumer URI), and the class-scopedJmsSpringLoadBalanceFailOver.foo/.barnames remove genuine cross-test contamination on the shared Artemis broker —JmsSpringLoadBalanceFailOverIT.xmlis consumed only byJmsSpringLoadBalanceFailOverJMSIT, and no assertions are weakened anywhere. - Repeating the readiness wait at the start of both
@Ordertests inRouteIdTransactedITis right, since the Spring context persists across the ordered methods and the uptime check is cheap.
Test-only change (no docs needed), commit follows the CAMEL-XXXX: convention, and CI is green on both JDK 17 and 25 builds.
Reviewed with Claude Code (Fable 5) on behalf of oscerd. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
Summary
Fix flaky JMS tests that use Spring XML configuration by addressing two root causes: queue name collisions between tests sharing a singleton Artemis broker, and race conditions where messages are sent before JMS consumers are fully subscribed.
Changes
foo,bar) to class-scoped names (e.g.,JmsSpringLoadBalanceFailOver.foo) inJmsSpringLoadBalanceFailOverIT.xmlto prevent cross-test contamination via the shared brokerJmsTestHelper.waitForJmsConsumerRoutes()can target themJmsTestHelper.waitForJmsConsumerRoutes()calls before sending messages to ensure JMS consumers are fully subscribed to the brokerMockEndpoint.assertIsSatisfied()calls withMockEndpoint.assertIsSatisfied(context, 30, TimeUnit.SECONDS)for deterministic timeoutsRouteIdTransactedIT(was using old class nameRouteIdTransactedTest)Affected tests (7 test classes, 13 test methods)
JmsSpringLoadBalanceFailOverJMSITRouteIdTransactedITJmsRouteUsingSpringITJmsRouteTest)JmsRouteUsingSpringAndJmsNameITJmsRouteTest)JmsRouteUsingSpringJMSTemplateITJmsRouteTest)JmsRouteUsingSpringWithAutoWireITJmsRouteTest)All 13 test methods pass locally.
Claude Code on behalf of gnodet
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com