RFR: 8352926: New test TestDockerMemoryMetricsSubgroup.java fails [v5]

[Leonid Mesnik]

On Thu, 15 May 2025 10:03:11 GMT, PAWAN CHAWDHARY <duke at openjdk.org> wrote:

>> 8352926: New test TestDockerMemoryMetricsSubgroup.java fails
>
> PAWAN CHAWDHARY has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Remove unused import

Changes requested by lmesnik (Reviewer).

test/hotspot/jtreg/containers/docker/TestMemoryWithSubgroups.java line 73:

> 71:             return;
> 72:         }
> 73:         if (IS_DOCKER && ContainerRuntimeVersionTestUtils.DOCKER_VERSION_20_10_0.compareTo(ContainerRuntimeVersionTestUtils.getContainerRuntimeVersion()) > 0) {

Better to replace this with 
`isContainerVersionSupported()`
...
and implement all logic in the the ContainerRuntimeVersionTestUtils

test/lib/jdk/test/lib/containers/docker/ContainerRuntimeVersionTestUtils.java line 38:

> 36:     private final int micro;
> 37:     private static final ContainerRuntimeVersionTestUtils DEFAULT = new ContainerRuntimeVersionTestUtils(0, 0, 0);
> 38:     public static final ContainerRuntimeVersionTestUtils DOCKER_VERSION_20_10_0 = new ContainerRuntimeVersionTestUtils(20, 10, 0);

Please add comment about meaning of the version or even better rename 
DOCKER_MINIMAL_SUPPORTED_VERSION = ....

test/lib/jdk/test/lib/containers/docker/ContainerRuntimeVersionTestUtils.java line 53:

> 51:         } else if (this.major < other.major) {
> 52:             return -1;
> 53:         } else { // equal major

no need to add `else {` here

test/lib/jdk/test/lib/containers/docker/ContainerRuntimeVersionTestUtils.java line 58:

> 56:             } else if (this.minor < other.minor) {
> 57:                 return -1;
> 58:             } else { // equal majors and minors

no need to add `else {` here

test/lib/jdk/test/lib/containers/docker/ContainerRuntimeVersionTestUtils.java line 81:

> 79:         } catch (Exception e) {
> 80:             System.out.println("Failed to parse container runtime version: " + version);
> 81:             return DEFAULT;

Any reason to don't fail here?

test/lib/jdk/test/lib/containers/docker/ContainerRuntimeVersionTestUtils.java line 94:

> 92:         } catch (Exception e) {
> 93:             System.out.println(Container.ENGINE_COMMAND + " --version command failed. Returning null");
> 94:             return null;

Any reason to don't fail here?

-------------

PR Review: https://git.openjdk.org/jdk/pull/24948#pullrequestreview-2844326936
PR Review Comment: https://git.openjdk.org/jdk/pull/24948#discussion_r2091561369
PR Review Comment: https://git.openjdk.org/jdk/pull/24948#discussion_r2091564985
PR Review Comment: https://git.openjdk.org/jdk/pull/24948#discussion_r2091555059
PR Review Comment: https://git.openjdk.org/jdk/pull/24948#discussion_r2091555200
PR Review Comment: https://git.openjdk.org/jdk/pull/24948#discussion_r2091567641
PR Review Comment: https://git.openjdk.org/jdk/pull/24948#discussion_r2091566692