On Thu, 15 May 2025 10:03:11 GMT, PAWAN CHAWDHARY <d...@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
