gnodet-bot commented on code in PR #26864:
URL: https://github.com/apache/camel/pull/26864#discussion_r4104444647
##########
dsl/camel-jbang/camel-jbang-it/src/test/java/org/apache/camel/dsl/jbang/it/support/JBangTestSupport.java:
##########
@@ -107,9 +107,22 @@ protected void afterEach(TestInfo testInfo) {
// The JBang installation baseline is entirely hidden (.jbang/,
.bashrc, .camel-jbang/).
try {
execInContainer("find /home/jbang -maxdepth 1 -mindepth 1 -not
-name '.*' -exec rm -rf {} +");
- } catch (Exception e) {
+ } catch (Exception | AssertionError e) {
logger.debug("failed to clean up test files from /home/jbang:
{}", e.getMessage());
}
+ try {
+ execute("config unset runtime");
+ execute("config unset gav");
+ execute("config unset directory");
+ String forceRunVersion =
System.getProperty(CliProperties.FORCE_RUN_VERSION, "");
+ if (!forceRunVersion.isEmpty()) {
+ execute("version set " + forceRunVersion);
+ } else {
+ execute("config unset camel-version");
+ }
+ } catch (Exception e) {
Review Comment:
⚠️ **Inconsistent catch clause — `AssertionError` not caught here.**
The `/home/jbang` sweep four lines above (line 110) was widened from `catch
(Exception e)` to `catch (Exception | AssertionError e)` precisely because
`execInContainer()` throws `AssertionError` in local-process mode. The same
reasoning applies here: `execute()` calls `containerService.execute()` which
uses `Assertions.assertThat(...)` — so it throws `AssertionError`, not
`Exception`, on failure in local-process mode.
If `execute("config unset runtime")` fails with `AssertionError`, this catch
clause won't handle it and the error will escape the `finally` block, aborting
`afterEach` before `FileUtil.removeDir()` — the exact same bug described in the
PR description for the `/home/jbang` sweep.
```suggestion
} catch (Exception | AssertionError e) {
```
##########
dsl/camel-jbang/camel-jbang-it/src/test/java/org/apache/camel/dsl/jbang/it/support/JBangTestSupport.java:
##########
@@ -107,9 +107,22 @@ protected void afterEach(TestInfo testInfo) {
// The JBang installation baseline is entirely hidden (.jbang/,
.bashrc, .camel-jbang/).
try {
execInContainer("find /home/jbang -maxdepth 1 -mindepth 1 -not
-name '.*' -exec rm -rf {} +");
- } catch (Exception e) {
+ } catch (Exception | AssertionError e) {
logger.debug("failed to clean up test files from /home/jbang:
{}", e.getMessage());
}
+ try {
+ execute("config unset runtime");
+ execute("config unset gav");
+ execute("config unset directory");
+ String forceRunVersion =
System.getProperty(CliProperties.FORCE_RUN_VERSION, "");
+ if (!forceRunVersion.isEmpty()) {
+ execute("version set " + forceRunVersion);
+ } else {
+ execute("config unset camel-version");
+ }
+ } catch (Exception e) {
Review Comment:
⚠️ **Single try-catch wraps four independent cleanup commands — first
failure skips the rest.**
If `execute("config unset runtime")` throws, `gav`, `directory`, and
`camel-version` are never cleaned up. Each unset is independent and should
survive the failure of another. Wrap each individually:
```suggestion
for (String key : new String[]{"runtime", "gav", "directory"}) {
try {
execute("config unset " + key);
} catch (Exception | AssertionError e) {
logger.debug("failed to unset config key {}: {}", key,
e.getMessage());
}
}
try {
String forceRunVersion =
System.getProperty(CliProperties.FORCE_RUN_VERSION, "");
if (!forceRunVersion.isEmpty()) {
execute("version set " + forceRunVersion);
} else {
execute("config unset camel-version");
}
} catch (Exception | AssertionError e) {
logger.debug("failed to reset camel-version: {}",
e.getMessage());
}
```
--
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]
