On Fri, 17 Nov 2023 19:36:21 GMT, Naoto Sato <na...@openjdk.org> wrote:
>> Justin Lu has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> reflect review comments
>
> test/jdk/java/util/Currency/PropertiesTest.sh line 30:
>
>> 28: # @summary tests the capability of replacing the currency data with user
>> 29: # specified currency properties file
>> 30: # @requires vm.flagless
>
> Does this actually do anything? Since it is a shell script, it does not call
> any `ProcessBuilder` methods. In fact, the script includes `${TESTVMOPTS}` on
> launching the test java process correctly.
IIUC, using `@requires vm.flagless` simply signifies that any JVMs launched
within the test do not support VM flags, (regardless if it was launched from
ProcessBuilder).
However, it is odd that this test was flagged since it does pass
`${TESTVMOPTS}` to `run()` like you stated.
Perhaps @lmesnik has further understanding of what to do here?
> test/jdk/java/util/logging/LoggingDeadlock2.java line 167:
>
>> 165:
>> 166: private static final List<String> javaChildArgs = Arrays.asList(
>> 167: "-classpath", classpath, "LoggingDeadlock2$JavaChild");
>
> Could use `List.of()`
Fixed here and in other instances.
> test/jdk/java/util/zip/EntryCount64k.java line 3:
>
>> 1: /*
>> 2: * Copyright (c) 2013 Google Inc. All rights reserved.
>> 3: * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
>
> Is this OK? The header reads `DO NOT ALTER`.
Yes, I think you're right. I based my decision off
https://github.com/openjdk/jdk/pull/15454
But after reading the copyright guidelines, we do not modify third party
copyright. It should probably be removed in that test as well.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16705#discussion_r1397843189
PR Review Comment: https://git.openjdk.org/jdk/pull/16705#discussion_r1397843173
PR Review Comment: https://git.openjdk.org/jdk/pull/16705#discussion_r1397843159
