[#] Fix flaky test TopicSubscriptionEnableExpiryTest.java

Author: mattrpav

activemq-unit-tests/src/test/java/org/apache/activemq/broker/region/TopicSubscriptionEnableExpiryTest.java

@@ -38,6 +38,7 @@
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
 
 /**
  * Tests correctness of the {@code ExpiryCheckEnabled} feature on
@@ -246,9 +247,12 @@ public void testCustomLimitStrategyWithDefaultEvictionLeavesExpiryCheckEnabled()
      *   <li>Sending 250 messages with a very short TTL.
      *   <li>Waiting for all TTLs to elapse.
      *   <li>Sending one more message (triggers the code path).
-     *   <li>Asserting that the broker's expired-message counter is 0
-     *       (no expiry scan ran) while the discarded counter is > 0
-     *       (normal eviction ran as expected).
+     *   <li>Asserting that the slow-consumer backlog was cleared by the normal
+     *       eviction strategy (discarded &gt; 0) rather than the eager expiry scan,
+     *       i.e. eviction dominates any incidental expiry (expired &lt; discarded).
+     *       The expired counter is not asserted to be exactly zero because the
+     *       always-on expiry paths (per-message dispatch check, client expired-acks)
+     *       may still expire a small, timing-dependent number of messages.
      * </ol>
      */
     @Test
@@ -288,9 +292,28 @@ public void testExpiryCheckDisabledSkipsExpiredMessageScan() throws Exception {
             Destination dest = broker.getDestination(new ActiveMQTopic("TEST.EXPIRY.DISABLED"));
             long expiredCount = dest.getDestinationStatistics().getExpired().getCount();
 
-            assertEquals(
-                    "With ExpiryCheckEnabled=false, the expiry scan must not run — expired counter must be 0",
-                    0L, expiredCount);
+            // The ExpiryCheckEnabled flag only skips the eager removeExpiredMessages() scan in
+            // TopicSubscription.add(); it does NOT disable the always-on expiry paths (the per-message
+            // isExpired() check when dispatching to the consumer, and client expired-acks), which may
+            // still expire a small, timing-dependent number of messages. So the expired counter is not
+            // reliably zero. The feature's actual guarantee is that the slow-consumer backlog is cleared
+            // by the normal eviction strategy instead of the expiry scan — assert that eviction did the
+            // work and dominates any incidental expiry.
+            TopicSubscription sub = null;
+            for (Subscription s : dest.getConsumers()) {
+                if (s instanceof TopicSubscription) {
+                    sub = (TopicSubscription) s;
+                    break;
+                }
+            }
+            assertNotNull("expected a TopicSubscription on the destination", sub);
+            int evictedCount = sub.discarded();
+
+            assertTrue("eviction must clear the slow-consumer backlog when the eager expiry scan is "
+                    + "disabled (evicted=" + evictedCount + ")", evictedCount > 0);
+            assertTrue("with the eager expiry scan disabled, eviction - not the expiry scan - must clear "
+                    + "the backlog (expired=" + expiredCount + ", evicted=" + evictedCount + ")",
+                    expiredCount < evictedCount);
 
             conn.close();
         } finally {