gnodet-bot commented on code in PR #28004:
URL: https://github.com/apache/camel/pull/28004#discussion_r4302843213
##########
dsl/camel-jbang/camel-jbang-core/src/test/java/org/apache/camel/dsl/jbang/core/commands/EvalSimpleTest.java:
##########
@@ -16,15 +16,39 @@
*/
package org.apache.camel.dsl.jbang.core.commands;
+import java.nio.file.Path;
+
import org.apache.camel.dsl.jbang.core.commands.action.EvalExpressionCommand;
+import org.apache.camel.dsl.jbang.core.common.CommandLineHelper;
+import org.apache.camel.util.HomeHelper;
+import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.condition.DisabledIfSystemProperty;
import picocli.CommandLine;
+@UsesWorkingDirectory
@DisabledIfSystemProperty(named = "ci.env.name", matches = ".*",
disabledReason = "Runs only local")
class EvalSimpleTest extends CamelCommandBaseTestSupport {
+ private Path previousHome;
Review Comment:
💡 **`previousHome` shadows the parent field — consider using
`useTestHomeDir()`**
`EvalSimpleTest` declares its own `private Path previousHome` which shadows
`CamelCommandBaseTestSupport.previousHome`, and manually saves/restores the
home dir in `setup()` / `restoreHome()` — duplicating the pattern that
`useTestHomeDir()` was introduced to centralize.
The reason is valid (this test needs the real user home, not a temp one,
because it talks to a live JBang-installed Camel). That constraint is worth a
comment to explain *why* `useTestHomeDir()` is intentionally avoided here —
otherwise the next reader will either refactor it "for consistency" or wonder
if it's a bug.
Not a correctness issue: the parent's `restoreHomeDir()` is a no-op here
(its `previousHome` is null since `useTestHomeDir()` is never called), so there
is no double-restore conflict.
##########
dsl/camel-jbang/camel-jbang-core/src/test/java/org/apache/camel/dsl/jbang/core/commands/CamelCommandBaseTestSupport.java:
##########
@@ -17,20 +17,56 @@
package org.apache.camel.dsl.jbang.core.commands;
+import java.io.IOException;
+import java.nio.file.Files;
import java.nio.file.Path;
+import org.apache.camel.dsl.jbang.core.common.CommandLineHelper;
import org.apache.camel.dsl.jbang.core.common.StringPrinter;
+import org.apache.camel.util.FileUtil;
+import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
public class CamelCommandBaseTestSupport {
protected StringPrinter printer;
+ private Path previousHome;
+ private Path testHome;
+
@BeforeEach
public void setup() throws Exception {
printer = new StringPrinter();
}
+ /**
+ * Gives the test a home directory of its own: an empty directory for the
{@code .camel} directory, the user
+ * configuration and the plugins of the CLI. The tests run in parallel
forks, so a fixed directory would be shared
+ * with the tests of the other forks. The previous home is used again
after the test.
+ *
+ * @return the home directory
+ */
+ protected Path useTestHomeDir() throws IOException {
+ if (previousHome == null) {
+ previousHome = CommandLineHelper.getHomeDir();
+ }
+ testHome = Files.createTempDirectory("camel-cli-test-home");
Review Comment:
⚠️ **Temp directory leak on double `useTestHomeDir()` call**
If `useTestHomeDir()` is called twice (e.g. a subclass calls it in its own
`@BeforeEach` after the parent already called it in `setup()`), `previousHome`
is non-null on the second call so it is correctly preserved — but `testHome` is
overwritten with a fresh `createTempDirectory` before the old directory is
cleaned up. The reference to the first temp directory is lost;
`restoreHomeDir()` will only delete the last one.
This is not exploitable today (no subclass calls `useTestHomeDir()` twice),
but it is a trap for future contributors. A one-liner guard fixes it:
```suggestion
if (testHome != null) {
FileUtil.removeDir(testHome.toFile());
}
testHome = Files.createTempDirectory("camel-cli-test-home");
```
--
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]
