Skip to content

Speed up AdminCLI ITs.#344

Open
chenkins wants to merge 3 commits intomainfrom
speedup-admin-cli-it
Open

Speed up AdminCLI ITs.#344
chenkins wants to merge 3 commits intomainfrom
speedup-admin-cli-it

Conversation

@chenkins
Copy link
Copy Markdown
Contributor

@chenkins chenkins commented Apr 3, 2026

Speed up AdminCLI ITs by implementing TestExecutionListener to setup/teardown Katta Server for all AdminCLI IT only once.

…teardown Katta Server for all AdminCLI IT only once.
Copilot AI review requested due to automatic review settings April 3, 2026 21:24
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR speeds up admin-cli integration tests by moving the Testcontainers docker-compose environment lifecycle from per-test-class (@BeforeAll/@AfterAll) to a single per-test-plan setup/teardown via a JUnit Platform TestExecutionListener.

Changes:

  • Register a JUnit Platform TestExecutionListener via META-INF/services for the admin-cli test runtime.
  • Add AdminCLIIntegrationTestSetupListener to start/stop the docker-compose environment once per test plan when @Tag("cli") tests are present.
  • Remove docker-compose setup/teardown from AbstractAdminCLIIT so individual IT classes no longer start/stop containers.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File Description
admin-cli/src/test/resources/META-INF/services/org.junit.platform.launcher.TestExecutionListener Registers the listener so JUnit discovers it during test execution.
admin-cli/src/test/java/cloud/katta/testsetup/AdminCLIIntegrationTestSetupListener.java Implements global docker-compose startup/shutdown for cli-tagged tests.
admin-cli/src/test/java/cloud/katta/testsetup/AbstractAdminCLIIT.java Removes per-class container lifecycle hooks; keeps per-test authentication/client setup.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings April 7, 2026 19:28
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +41 to +44
final Properties props = new Properties();
try {
props.load(AbstractAdminCLIIT.class.getResourceAsStream(envFile));
}
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

props.load(AbstractAdminCLIIT.class.getResourceAsStream(envFile)) doesn’t close the underlying InputStream. Use try-with-resources (and optionally a null-check with a clearer exception) to avoid leaking file handles during test execution.

Copilot uses AI. Check for mistakes.
@chenkins chenkins marked this pull request as ready for review April 7, 2026 21:08
@chenkins chenkins requested review from Copilot, dkocher and ylangisc April 7, 2026 21:08
@chenkins chenkins enabled auto-merge (squash) April 7, 2026 21:09
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

catch(URISyntaxException e) {
throw new RuntimeException(e);
}
compose.start();
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If compose.start() throws after partially starting containers, the environment may be left running because teardown might not execute. Consider wrapping startup in try/catch and stopping the compose environment on failure (or using a shutdown hook) so CI agents don't leak containers on startup errors.

Suggested change
compose.start();
try {
compose.start();
}
catch(Exception e) {
try {
compose.stop();
}
catch(Exception stopException) {
log.warn("Failed to stop docker-compose test environment after startup failure", stopException);
}
compose = null;
throw e;
}

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants