feat: add Docker/Podman test utilities for conditional test execution#740
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the testing infrastructure by introducing a dedicated module for managing tests that rely on Docker or Podman. It provides a robust mechanism to conditionally execute, skip, or fail such tests based on the environment's container engine availability and user-defined properties, thereby improving test reliability and developer experience across different environments, including CI/CD pipelines. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a very useful test utility module, test-utils-docker, for conditionally executing tests that depend on Docker or Podman. The implementation using a JUnit 5 extension is clean and the logic for checking container availability and handling the skip property is sound.
I've added a few suggestions to improve thread-safety, debuggability, and documentation accuracy in the new module. Overall, this is a great addition that will improve the testing experience in different environments.
test-utils-docker/src/main/java/io/a2a/testutils/docker/DockerAvailability.java
Show resolved
Hide resolved
test-utils-docker/src/main/java/io/a2a/testutils/docker/DockerAvailability.java
Outdated
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a very useful test utility for conditionally running Docker-based tests. The implementation is clean and well-documented. I've made a few suggestions to improve the robustness and align the behavior of the skip flag with common user expectations. Specifically, I've recommended that the -DskipDockerTests=true flag should always cause tests to be skipped, regardless of whether Docker is available. I've also suggested adding a timeout when checking for Docker/Podman to prevent the build from hanging. Overall, this is a great addition to the project's testing infrastructure.
test-utils-docker/src/main/java/io/a2a/testutils/docker/DockerRequiredExtension.java
Outdated
Show resolved
Hide resolved
test-utils-docker/src/main/java/io/a2a/testutils/docker/DockerAvailability.java
Outdated
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a well-designed utility for conditionally executing tests that require Docker or Podman. The new test-utils-docker module with the @RequiresDocker annotation is a great addition for improving the test execution experience in different environments. The implementation is clean and the feature is well-documented in the new README file.
I've found an area for improvement in DockerAvailability.java concerning exception handling, for which I've left a specific comment.
test-utils-docker/src/main/java/io/a2a/testutils/docker/DockerAvailability.java
Outdated
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new test-utils-docker module to conditionally execute tests based on Docker/Podman availability. The implementation is solid, using a JUnit 5 ExecutionCondition to check for a container runtime and a system property to allow skipping tests. The new @RequiresDocker annotation is applied to existing container-based tests, improving the test execution experience in environments without Docker. My review includes one suggestion to improve the robustness of the Docker/Podman detection logic by ensuring process output streams are handled correctly to prevent potential hangs.
test-utils-docker/src/main/java/io/a2a/testutils/docker/DockerAvailability.java
Outdated
Show resolved
Hide resolved
Add new test-utils-docker module with @RequiresDocker annotation for conditional execution of container-based tests. Features: - Detects both Docker and Podman availability - Fails tests when containers unavailable (clear error message) - Skips tests with -DskipDockerTests=true system property - Modules always compile regardless of container availability - No Testcontainers dependency (avoids classpath conflicts) Applied to: - extras/queue-manager-replicated/tests-single-instance - extras/queue-manager-replicated/tests-multi-instance/tests
bce54fa to
87f622b
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a helpful test utility for conditionally executing Docker-based tests. The implementation is solid, creating a new test-utils-docker module with a @RequiresDocker annotation and a corresponding JUnit 5 extension. The utility correctly detects Docker and Podman, and provides a clear mechanism for skipping tests. The code is well-structured and documented. I have one minor suggestion to slightly improve the double-checked locking implementation for clarity and performance.
test-utils-docker/src/main/java/io/a2a/testutils/docker/DockerAvailability.java
Show resolved
Hide resolved
|
@kabir thanks! |
Add new test-utils-docker module with @RequiresDocker annotation for conditional execution of container-based tests.
Features: