[JBANG-IT] Add cleanups for tests that can produce state leaks - #25948
Conversation
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
gnodet
left a comment
There was a problem hiding this comment.
Good work adding @AfterEach cleanup to prevent state leaks when running JBang integration tests on bare metal. The base class JBangTestSupport.afterEach() change is especially valuable — centralizing config cleanup there covers all subclasses consistently.
A few gaps I noticed in the cleanup coverage:
Missing execute("stop") in three modified classes:
The PR adds @AfterEach cleanup methods to DevModeITCase, HistoryITCase, and RunCommandITCase, but these only clean up files — they don't call execute("stop") to terminate background Camel processes started via executeBackground(). This is inconsistent with the pattern applied to 17 other classes in this same PR (CamelGetITCase, CamelLogITCase, CmdLoadITCase, etc.) and contradicts the stated goal of "ending camel processes in classes that run them."
- DevModeITCase — 6
executeBackground()calls, cleanup only doescleanupProfileFiles() - HistoryITCase — 1
executeBackground()call, cleanup only doescleanupInbox() - RunCommandITCase — 15+
executeBackground()calls, cleanup only doescleanupContainerFiles()
Adding execute("stop") to these three @AfterEach methods would make the cleanup consistent.
Redundant cleanup in CamelConfigITCase:
The resetConfig() method in CamelConfigITCase unsets runtime, gav, and directory, but the base class JBangTestSupport.afterEach() now also unsets all three (plus camel-version). Since JUnit 5 runs subclass @AfterEach before superclass @AfterEach, the subclass cleanup is entirely redundant. Consider removing the resetConfig() method or trimming it to only CamelConfigITCase-specific cleanup.
📋 PR Metadata
| Aspect | Current | Suggested |
|---|---|---|
| Labels | dsl |
+ test |
| Milestone | (none) | 4.23.0 |
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 1 tested, 0 compile-only — current: 0 all testedMaveniverse Scalpel detected 1 affected modules (current approach: 0).
|
When running tests on bare metal using
-Dcli.service.skip.install=true -Dcamel-cli.instance.type=local-camel-cli-processsome configurations can affect subsequent tests. This PR aims to reset the environment to the default state after each test is finished by unsetting configuration and ending camel processes in classes that run them. Also setting CamelDebugITCase as container-only.