You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by "gnodet (via GitHub)" <gi...@apache.org> on 2026/09/04 09:43:29 UTC
[PR] CAMEL-24618: Fix flaky SpringFileAntPathMatcherRemoteFileFilterTest [camel]
gnodet opened a new pull request, #26120:
URL: https://github.com/apache/camel/pull/26120
## Problem
`SpringFileAntPathMatcherRemoteFileFilterTest.testAntPatchMatcherFilter` is flaky with two distinct failure modes:
1. **`expected <1> but was <9>` — state contamination**: The FTP consumer URI had no `delete` option, so consumed files remained on the FTP server. Since `FtpServiceExtension.afterAll()` is a no-op (the FTP server is never stopped/reset), files accumulated across test retries, causing the mock to receive duplicate messages.
2. **`expected <1> but was <0>` — timing race**: The consumer had `initialDelay=2000ms`, and with the default `MockEndpoint.resultWaitTime` (10s), the assertion could expire before the first poll completed on slow CI environments.
## Fix
- **Add `delete=true`** to the FTP consumer URI in the Spring XML context — files are deleted after processing, preventing accumulation across retries
- **Change `initialDelay` from 2000 to 0** — the consumer starts polling immediately
- **Set `resultWaitTime(30000)`** on the mock endpoint — generous timeout for slow CI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24618: Fix flaky SpringFileAntPathMatcherRemoteFileFilterTest [camel]
Posted by "utafrali (via GitHub)" <gi...@apache.org>.
utafrali commented on code in PR #26120:
URL: https://github.com/apache/camel/pull/26120#discussion_r3932847665
##########
.github/workflows/sonar-scan.yml:
##########
@@ -143,11 +140,11 @@ jobs:
-Dsonar.pullrequest.branch=${{ env.pr_head_ref }}
-Dsonar.pullrequest.base=${{ env.pr_base_ref }}
-Dsonar.pullrequest.key=${{ env.pr_number }}
- -Dsonar.pullrequest.github.repository=apache/camel
+ -Dsonar.pullrequest.github.repository=gnodet/camel
Review Comment:
This hardcodes `-Dsonar.pullrequest.github.repository=gnodet/camel`, `-Dsonar.projectKey=gnodet_camel`, and `-Dsonar.organization=gnodet`. If this lands on `apache/camel`, sonar analysis will publish to your personal fork's Sonar project instead of `apache_camel`. These look like local testing overrides that shouldn't be committed. Revert to `apache/camel`, `apache_camel`, `apache`.
##########
.github/workflows/sonar-scan.yml:
##########
@@ -28,11 +28,8 @@ concurrency:
jobs:
sonar:
- # Temporarily disabled until SonarCloud quality gate is adjusted (INFRA-27808)
if: >
Review Comment:
Removing `github.repository == 'apache/camel'` from the condition means the workflow will now attempt to run on every fork that receives a `workflow_run` event. Combined with the hardcoded `gnodet/camel` project key below, this looks like fork-local plumbing rather than an intentional main-branch change. Keep the repository guard.
##########
.github/workflows/sonar-build.yml:
##########
@@ -37,15 +37,15 @@ concurrency:
jobs:
build:
- # Temporarily disabled until SonarCloud quality gate is adjusted (INFRA-27808)
- if: false && github.repository == 'apache/camel'
+ if: true
Review Comment:
`if: false && ...` was replaced by `if: true`, dropping both the INFRA-27808 gate and the `github.repository == 'apache/camel'` guard in one go. The linked ticket comment above still says the workflow is disabled pending SonarCloud quality-gate work — either update/remove the comment and confirm INFRA-27808 is resolved, or keep the repo guard.
##########
.github/actions/incremental-build/incremental-build.sh:
##########
@@ -480,11 +547,11 @@ main() {
fi
fi
- # Fetch the diff (PR diff via API, or git diff for push builds)
+ # Compute the diff using local git history (merge-base for PRs, HEAD~1 for push builds)
local diff_body
if [ -n "$prId" ]; then
- echo "Fetching PR #${prId} diff..."
- diff_body=$(fetchDiff "$prId" "$repository")
+ echo "Computing diff against origin/${GITHUB_BASE_REF:-main}..."
Review Comment:
Switching PR diff computation from the GitHub API to a local `git diff merge-base..HEAD` is a meaningful behavior change (previously the diff matched exactly what GitHub shows on the PR; now it includes any local merge commits or force-push artifacts differently). Worth calling out in the PR description and testing on a PR that has been rebased/force-pushed — the two diffs can diverge when the base branch has moved forward and the PR hasn't been rebased.
##########
.mvn/extensions.xml:
##########
@@ -23,6 +23,6 @@
<extension>
<groupId>eu.maveniverse.maven.scalpel</groupId>
<artifactId>extension3</artifactId>
- <version>0.3.0</version>
+ <version>0.3.3</version>
Review Comment:
Bumping Scalpel from 0.3.0 to 0.3.3 is unrelated to the flaky-test fix advertised in the PR title. Please split: (1) the CAMEL-24618 test fix, (2) the Scalpel bump + shadow-comparison wiring, (3) the sonar/pr-build workflow changes. Right now this PR mixes three independent changes under a title that only names one.
##########
.github/workflows/full-test-suite.yml:
##########
@@ -0,0 +1,255 @@
+#
Review Comment:
A new 255-line workflow (`full-test-suite.yml`) is landing under a PR titled "Fix flaky SpringFileAntPathMatcherRemoteFileFilterTest". Split this out — it deserves its own PR, its own JIRA (or none if infra-only), and its own review. Bundling it here hides it from reviewers looking at the ticket scope.
##########
.github/workflows/pr-build-main.yml:
##########
@@ -60,7 +60,7 @@ permissions:
jobs:
build:
- if: github.repository == 'apache/camel'
+ if: github.repository == 'apache/camel' || github.repository == 'gnodet/camel'
Review Comment:
`github.repository == 'apache/camel' || github.repository == 'gnodet/camel'` hardcodes a personal fork into an apache/camel workflow. If you need to test on your fork, use `github.repository_owner != ''` or drop the guard locally, not in the upstream file.
##########
.github/workflows/sonar-build.yml:
##########
@@ -37,15 +37,15 @@ concurrency:
jobs:
build:
- # Temporarily disabled until SonarCloud quality gate is adjusted (INFRA-27808)
- if: false && github.repository == 'apache/camel'
+ if: true
name: Build for Sonar Analysis
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
- with:
- persist-credentials: false
-
+ - name: Fetch base branch for Scalpel change detection
+ run: |
+ git fetch --deepen=200 2>/dev/null || true
Review Comment:
The `persist-credentials: false` line was removed from the checkout step. This is a hardening default (OSSF Scorecard recommends it for `actions/checkout`) — restore it unless there is a specific reason later steps need the credential helper.
##########
tests/camel-itest/src/test/resources/org/apache/camel/itest/ftp/SpringFileAntPathMatcherRemoteFileFilterTest-context.xml:
##########
@@ -30,7 +30,7 @@
<template id="camelTemplate"/>
<!-- use myFilter as filter to allow setting ANT paths for which files to scan for -->
- <endpoint id="myFTPEndpoint" uri="ftp://admin@localhost:${SpringFileAntPathMatcherRemoteFileFilterTest.ftpPort}/antpath?password=admin&recursive=true&delay=10000&initialDelay=2000&filter=#myAntFilter"/>
+ <endpoint id="myFTPEndpoint" uri="ftp://admin@localhost:${SpringFileAntPathMatcherRemoteFileFilterTest.ftpPort}/antpath?password=admin&recursive=true&delay=10000&initialDelay=0&delete=true&filter=#myAntFilter"/>
Review Comment:
The URI change itself is the right fix, but consider adding `readLock=changed` (or `readLock=none` with a comment explaining why it's safe here) alongside `delete=true`. Without a read lock, on a slow FTP handshake the consumer can begin reading a file that the producer is still writing — historically a source of a different flake class in the FTP itests. If you've already ruled this out for this test, leave it, but a one-line XML comment near `delete=true` explaining the state-contamination fix would help the next reader.
##########
tests/camel-itest/src/test/java/org/apache/camel/itest/ftp/SpringFileAntPathMatcherRemoteFileFilterTest.java:
##########
@@ -56,6 +56,7 @@ public class SpringFileAntPathMatcherRemoteFileFilterTest {
void testAntPatchMatherFilter() throws Exception {
result.expectedBodiesReceived(expectedBody);
+ result.setResultWaitTime(30000);
Review Comment:
Prefer the timed overload `MockEndpoint.assertIsSatisfied(context, 30, TimeUnit.SECONDS)` in the assertion below instead of calling `setResultWaitTime(30000)`. It reads more clearly at the assertion site and keeps the wait budget next to the assertion it applies to. Per the project guidelines, mock-based waits should use MockEndpoint's native timed API rather than setter side-effects.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24618: Fix flaky SpringFileAntPathMatcherRemoteFileFilterTest [camel]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #26120:
URL: https://github.com/apache/camel/pull/26120#issuecomment-5539523459
<!-- ci-tested-modules -->
:test_tube: **CI tested the following changed modules:**
- `tests/camel-itest`
---
<details><summary>:microscope: Scalpel shadow comparison — Scalpel: 1 tested, 0 compile-only — current: 0 all tested</summary>
[Maveniverse Scalpel](https://github.com/maveniverse/scalpel) detected **1 affected modules** (current approach: 0).
<details><summary>:warning: Modules only in Scalpel (1)</summary>
- `camel-itest`
</details>
**Skip-tests mode would test 1 modules** (1 direct + 0 downstream), **skip tests for 0** (generated code, meta-modules)
<details><summary>Modules Scalpel would test (1)</summary>
- `camel-itest`
</details>
> :information_source: Shadow mode — Scalpel observes but does not affect test execution. [Learn more](https://github.com/maveniverse/scalpel)
</details>
---
:gear: [View full build and test results](https://github.com/apache/camel/actions/runs/33864578043)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24618: Fix flaky SpringFileAntPathMatcherRemoteFileFilterTest [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #26120:
URL: https://github.com/apache/camel/pull/26120#issuecomment-5572935918
@gnodet is this ready ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24618: Fix flaky SpringFileAntPathMatcherRemoteFileFilterTest [camel]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #26120:
URL: https://github.com/apache/camel/pull/26120#issuecomment-5573356439
> @gnodet is this ready ?
No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24618: Fix flaky SpringFileAntPathMatcherRemoteFileFilterTest [camel]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet merged PR #26120:
URL: https://github.com/apache/camel/pull/26120
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24618: Fix flaky SpringFileAntPathMatcherRemoteFileFilterTest [camel]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #26120:
URL: https://github.com/apache/camel/pull/26120#issuecomment-5539309341
:star2: Thank you for your contribution to the Apache Camel project! :star2:
:robot: CI automation will test this PR automatically.
:camel: Apache Camel Committers, please review the following items:
* First-time contributors **require MANUAL approval** for the GitHub Actions to run
* You can use the command `/component-test (camel-)component-name1 (camel-)component-name2..` to request a test from the test bot although they are normally detected and executed by CI.
* You can label PRs using `skip-tests` and `test-dependents` to fine-tune the checks executed by this PR.
* Build and test logs are available in the summary page. **Only** [Apache Camel committers](https://camel.apache.org/community/team/#committers) have access to the summary.
:warning: Be careful when sharing logs. Review their contents before sharing them publicly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24618: Fix flaky SpringFileAntPathMatcherRemoteFileFilterTest [camel]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #26120:
URL: https://github.com/apache/camel/pull/26120#issuecomment-5539538550
Addressed all review comments:
- **Unrelated CI/Sonar/Scalpel changes**: The PR was rebased onto `upstream/main` — the diff now contains only the 2 test files. The earlier review was on a stale base that included fork-local CI changes.
- **Use timed `assertIsSatisfied`**: Replaced `setResultWaitTime(30000)` + `result.assertIsSatisfied()` with `MockEndpoint.assertIsSatisfied(context, 30, TimeUnit.SECONDS)` — clearer and keeps the timeout at the assertion site.
- **Add explanatory XML comment**: Added a comment above the endpoint explaining why `delete=true` is needed (state contamination from `FtpServiceExtension` never resetting the FTP directory).
- **`readLock=changed`**: Not needed here — the producer writes via `template.sendBodyAndHeader()` which completes synchronously before the consumer polls. The consumer's `initialDelay=0` + `delay=10000` means the first poll happens after context start, by which point all 5 files are fully written. No partial-read risk.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org