Skip to content

CAMEL-19545: Replace sleep-based synchronization in camel-stream tests#24304

Draft
rkdfx wants to merge 1 commit into
apache:mainfrom
rkdfx:camel-19545-stream-thread-sleep
Draft

CAMEL-19545: Replace sleep-based synchronization in camel-stream tests#24304
rkdfx wants to merge 1 commit into
apache:mainfrom
rkdfx:camel-19545-stream-thread-sleep

Conversation

@rkdfx

@rkdfx rkdfx commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Description

Use flushes and Awaitility-driven waiting in the automated stream tests, and replace the disabled manual test's fixed sleep with a timed latch wait. This removes flaky timing assumptions from the camel-stream test suite.

Target

  • I checked that the commit is targeting the correct branch (Camel 4 uses the main branch)

Tracking

  • If this is a large change, bug fix, or code improvement, I checked there is a JIRA issue filed for the change (usually before you start working on it).

Apache Camel coding standards and style

  • I checked that each commit in the pull request has a meaningful subject line and body.
  • I have run mvn clean install -DskipTests locally from root folder and I have committed all auto-generated changes.

AI-assisted contributions

  • If this PR includes AI-generated code, commits have proper co-authorship attribution (e.g., Co-authored-by trailers) and the PR description identifies the AI tool used.

Use flushes and Awaitility-driven waiting in the automated stream tests,
and replace the disabled manual test's fixed sleep with a timed latch wait.
This removes flaky timing assumptions from the camel-stream test suite.

Signed-off-by: Ravi <13908473+rkdfx@users.noreply.github.com>
@rkdfx

rkdfx commented Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

I'm running the mvn clean install -DskipTests locally but it's failing, it's unrelated to my changes. I'll try to clean and test again - Failed to execute goal io.github.ascopes:protobuf-maven-plugin:4.1.3:generate (default) on project camel-kserve: Generation failed - PROTOC FAILED: Protoc failed with an error. Check the build logs above to find the root cause.

@gnodet gnodet 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.

The goal of removing Thread.sleep() from these tests is sound, but the execution needs work — only one of the four changes actually replaces sleep with proper event-based synchronization. The others just delete sleeps and add fos.flush(), which trades one kind of fragility for another.

ScanStreamFileManualTest — drop this change

// Before:
Thread.sleep(60000);
// After:
new CountDownLatch(1).await(60, TimeUnit.SECONDS);

This test is @Disabled("For manual testing") — the 60-second pause is intentional so a human can interact with the file while the route runs. A CountDownLatch(1) that is never counted down is just Thread.sleep with extra steps: less readable, and not Awaitility. This file should be left unchanged.

ScanStreamFileTest.testScanRefreshedFile() — good ✅

await().atMost(10, TimeUnit.SECONDS).until(() -> mock.getReceivedCounter() >= 2);

This is exactly the right pattern — event-based, deterministic, no flaky timing assumption. This is the model the other tests should follow.

ScanStreamFileTest.testScanFile() / testScanFileAlreadyWritten() — incomplete

These simply remove the sleeps and add fos.flush(). They now rely entirely on MockEndpoint.assertIsSatisfied() having an implicit 10-second internal latch timeout (MockEndpoint.waitForCompleteLatch defaults to 10s when resultWaitTime is 0). That works in practice, but it's implicit and not obvious to any reader of the test.

fos.flush() pushes bytes from Java's buffer to the OS — it does not guarantee the consumer's BufferedReader.readLine() sees the data on the very next poll cycle. The consumer polls every 200ms (scanStreamDelay=200). Without any explicit wait, these tests silently depend on the mock's internal timeout to absorb that latency. This should use Awaitility like testScanRefreshedFile does, for consistency and clarity.

ScanStreamFileWithFilterTest — correct in effect, but inconsistent

Removing the interleaved sleeps between writes is fine here (the filter only matches "Hello Boy", so processing order doesn't matter, and moving fos.close() before assertIsSatisfied is actually better). But again, no Awaitility — it silently relies on MockEndpoint's 10s timeout.

Suggestions

  1. Drop the ScanStreamFileManualTest change — the sleep is intentional in a disabled manual test.
  2. Keep the testScanRefreshedFile() Awaitility change as-is.
  3. Add Awaitility to testScanFile(), testScanFileAlreadyWritten(), and ScanStreamFileWithFilterTest instead of just removing sleeps — e.g. await().atMost(10, TimeUnit.SECONDS).untilAsserted(() -> MockEndpoint.assertIsSatisfied(context)). This makes the synchronization explicit and consistent with the good pattern already in the PR.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants