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/07/06 21:07:33 UTC
[PR] CAMEL-23912: Fix flaky ThemeTest by using Theme's own classloader for CSS [camel]
gnodet opened a new pull request, #24475:
URL: https://github.com/apache/camel/pull/24475
## Summary
_Claude Code on behalf of gnodet_
Fix the 34% flaky rate of `ThemeTest` reported on the [Develocity dashboard](https://develocity.apache.org/scans/tests?search.rootProjectNames=camel&tests.sortField=FLAKY).
### Root Cause
`Theme.engine()` used `StyleEngine.loadStylesheet()` which internally loads CSS resources via `StyleEngine.class.getClassLoader()`. Under parallel test execution (enabled by `camel.surefire.parallel=true` in the DSL parent POM), the StyleEngine's classloader (from the tamboui-css JAR) can diverge from the project's classloader, causing intermittent "resource not found" failures. When CSS loading fails, the engine falls back to built-in defaults — the dark theme tests still pass (fallback values match the dark CSS values), but the light theme test fails because `Color.LIGHT_GREEN` ≠ `Color.rgb(0x00, 0x77, 0x00)`.
### Fix
- **Load CSS via Theme's own classloader**: Read CSS content using `Theme.class.getClassLoader().getResourceAsStream()` (with a thread-context-classloader fallback) and pass it to `StyleEngine.addStylesheet()` instead of `StyleEngine.loadStylesheet()`. This guarantees access to project resources regardless of classloader hierarchy.
- **Synchronize `accent()` and `zebra()`**: These two methods were the only unsynchronized Theme accessors, creating a potential for concurrent render tests to interfere with state between `engine()` return and `resolve()` call.
- **Use `ConcurrentHashMap` for style cache**: Defensive measure replacing the `HashMap`.
### Prior Fix Attempts (both insufficient)
- `cc39bab`: Added `testMode` flag and kept StyleEngine alive across resets
- `be06caeb` (CAMEL-23912): Added `@Isolated`, reset engine in `resetForTesting()`, load only active stylesheet
Both addressed symptoms (static state leaks) but not the root cause (classloader mismatch in CSS loading).
### Validation
- Ran full module tests (519 tests) with `parallel=true` and `parallel.factor=4` — 10/10 runs passed with 0 failures.
- ThemeTest alone: 20/20 runs passed.
--
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-23912: Fix flaky ThemeTest by using Theme's own classloader for CSS [camel]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #24475:
URL: https://github.com/apache/camel/pull/24475#issuecomment-4897621304
@copilot review
--
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-23912: Fix flaky ThemeTest by using Theme's own classloader for CSS [camel]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24475:
URL: https://github.com/apache/camel/pull/24475#issuecomment-4897624444
: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-23912: Fix flaky ThemeTest by using Theme's own classloader for CSS [camel]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24475:
URL: https://github.com/apache/camel/pull/24475#issuecomment-4897785623
<!-- ci-tested-modules -->
:test_tube: **CI tested the following changed modules:**
- `dsl/camel-jbang/camel-jbang-plugin-tui`
---
<details><summary>:microscope: Scalpel shadow comparison — Scalpel: 2 tested, 1 compile-only — current: 2 all tested</summary>
[Maveniverse Scalpel](https://github.com/maveniverse/scalpel) detected **3 affected modules** (current approach: 2).
<details><summary>:warning: Modules only in Scalpel (1)</summary>
- `camel-launcher`
</details>
**Skip-tests mode would test 2 modules** (1 direct + 1 downstream), **skip tests for 1** (generated code, meta-modules)
<details><summary>Modules Scalpel would test (2)</summary>
- `camel-jbang-plugin-tui`
- `camel-launcher-container`
</details>
<details><summary>Modules with tests skipped (1)</summary>
- `camel-launcher`
</details>
> :information_source: Shadow mode — Scalpel observes but does not affect test execution. [Learn more](https://github.com/maveniverse/scalpel)
</details>
<details><summary>All tested modules (3 modules)</summary>
- Camel :: JBang :: Plugin :: TUI
- Camel :: Launcher
- Camel :: Launcher :: Container
</details>
---
:gear: [View full build and test results](https://github.com/apache/camel/actions/runs/28823354135)
--
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-23912: Fix flaky ThemeTest by using Theme's own classloader for CSS [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus merged PR #24475:
URL: https://github.com/apache/camel/pull/24475
--
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-23912: Fix flaky ThemeTest by using Theme's own classloader for CSS [camel]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #24475:
URL: https://github.com/apache/camel/pull/24475#discussion_r3533793764
##########
dsl/camel-jbang/camel-jbang-plugin-tui/src/main/java/org/apache/camel/dsl/jbang/core/commands/tui/Theme.java:
##########
@@ -253,6 +261,28 @@ private static synchronized StyleEngine engine() {
return engine;
}
+ /**
+ * Reads a CSS resource from the classpath using this class's own classloader, which is guaranteed to have access to
+ * project resources regardless of classloader hierarchy.
+ */
+ private static String loadCssResource(String path) throws IOException {
+ InputStream is = Theme.class.getClassLoader().getResourceAsStream(path);
+ if (is == null) {
+ // Fallback to the thread context classloader in case the class's own classloader
+ // doesn't have visibility (e.g., in OSGi or custom classloader setups).
+ ClassLoader tccl = Thread.currentThread().getContextClassLoader();
+ if (tccl != null) {
+ is = tccl.getResourceAsStream(path);
+ }
+ }
+ if (is == null) {
+ throw new IOException("Theme CSS resource not found on classpath: " + path);
+ }
+ try (InputStream in = is) {
+ return new String(in.readAllBytes(), StandardCharsets.UTF_8);
+ }
+ }
Review Comment:
_Claude Code on behalf of gnodet_
Good catch — fixed in 45396a7. The classloader is now null-checked before calling `getResourceAsStream()`, so if `Theme.class.getClassLoader()` returns null (bootstrap-loaded class), we fall through cleanly to the TCCL fallback instead of throwing an NPE. The javadoc was also updated to document this path.
--
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-23912: Fix flaky ThemeTest by using Theme's own classloader for CSS [camel]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #24475:
URL: https://github.com/apache/camel/pull/24475#issuecomment-4900410849
_Claude Code on behalf of gnodet_
@ammachado Thank you for the thorough review and the independent verification of the root cause against the `tamboui-css` source! All three suggestions addressed in commit 45396a7:
1. **Javadoc wording** (suggestion #1) — Fixed. Changed from "guaranteed to have access" to "in the normal Camel class-loading hierarchy has access to project resources. Falls back to the thread-context classloader when the primary lookup returns null." This makes the fallback path clearly intentional rather than dead code.
2. **`ConcurrentHashMap` → `HashMap`** (suggestion #2) — Reverted. You're right that every `CACHE` access is already inside a `synchronized` method, so the concurrent map added no real safety — just a misleading signal. Plain `HashMap` is correct here.
3. **TCCL fallback test gap** (suggestion #3) — Acknowledged. As you noted, properly testing this would require a custom classloader rig. The primary classloader path is exercised by all 6 existing tests, and the TCCL fallback is a defensive edge case (OSGi, bootstrap-loaded classes). Leaving as a known gap for now.
--
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-23912: Fix flaky ThemeTest by using Theme's own classloader for CSS [camel]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #24475:
URL: https://github.com/apache/camel/pull/24475#discussion_r3533770634
##########
dsl/camel-jbang/camel-jbang-plugin-tui/src/main/java/org/apache/camel/dsl/jbang/core/commands/tui/Theme.java:
##########
@@ -253,6 +261,28 @@ private static synchronized StyleEngine engine() {
return engine;
}
+ /**
+ * Reads a CSS resource from the classpath using this class's own classloader, which is guaranteed to have access to
+ * project resources regardless of classloader hierarchy.
+ */
+ private static String loadCssResource(String path) throws IOException {
+ InputStream is = Theme.class.getClassLoader().getResourceAsStream(path);
+ if (is == null) {
+ // Fallback to the thread context classloader in case the class's own classloader
+ // doesn't have visibility (e.g., in OSGi or custom classloader setups).
+ ClassLoader tccl = Thread.currentThread().getContextClassLoader();
+ if (tccl != null) {
+ is = tccl.getResourceAsStream(path);
+ }
+ }
+ if (is == null) {
+ throw new IOException("Theme CSS resource not found on classpath: " + path);
+ }
+ try (InputStream in = is) {
+ return new String(in.readAllBytes(), StandardCharsets.UTF_8);
+ }
+ }
Review Comment:
`Theme.class.getClassLoader()` can legally return null (bootstrap-loaded classes). In that case this code will throw a NullPointerException before the TCCL fallback is attempted. Handle a null classloader explicitly so missing resources degrade to the intended IOException/logged fallback instead of an unexpected NPE.
--
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