oscerd commented on PR #26734: URL: https://github.com/apache/camel/pull/26734#issuecomment-5791150317
Thanks for the review, and particularly for re-running the mutation yourself — that is the check I most want a second pair of eyes on, because a test that cannot fail is worse than no test. Pushed `f012ca85`. Three of the four findings applied: - **`NumberFormatException` naming the key** — applied with the `trim()`. My original reasoning (match `OAuthTokenValidationConfigResolver`) was weak, since that resolver sits behind an opt-in feature while this one runs on any context resolving an `http:` resource. New test asserts on the message rather than the type, because `NumberFormatException extends IllegalArgumentException` and a type-only assertion would pass either way. - **Busy-spin** — applied. A real harness defect: `socket.isClosed()` only reflects a local close, so it burned a core for every mute-server test. - **`StandardCharsets.US_ASCII`** — applied. **Not applying the test-visibility one** (5 of the 8 threads). The CLAUDE.md rule it cites is not in the file, and `core/camel-core` is 2725 public test classes to 112 package-private, with `ResourceHelperTest` next door public — details and the grep output are on that thread. Happy to be overruled by a committer, but it would be a project-wide decision rather than one new file. **On your open question** — where `camel.resource.http.connect-timeout` / `read-timeout` live permanently: I agree the upgrade guide is the wrong long-term home, since it is explicitly migration-only and nobody reads the 4.23 guide in 4.27. I deliberately have not invented a page for it here, because picking the home is a structural docs decision for the committers, not something to settle inside a bug fix. For what it is worth, the two candidates I can see are a new resource-resolution section under the user manual (nothing exists today), or folding it into the properties/configuration reference alongside the other ad-hoc `camel.*` keys. If you tell me which you prefer I will open a separate docs issue and do it — I would rather not grow this PR into a documentation restructure. 22 tests green (17 `ResourceHelperTest` + 5 new). _Claude Code on behalf of @oscerd_ -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
