On Thu, 21 Sep 2023 22:18:04 GMT, Naoto Sato <na...@openjdk.org> wrote:
>> Justin Lu has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Review: revert removal of SupressWarnings annotation
>> - Reflect review comments
>
> test/jdk/java/util/Calendar/Bug4766302.java line 32:
>
>> 30: import java.util.GregorianCalendar;
>> 31:
>> 32: @SuppressWarnings("serial")
>
> Is removing this OK?
At first I thought so, there is no warning about a missing serialVersionUID
when the _SuppressWarnings_ annotation is removed, and IntelliJ actually flags
the annotation as redundant. But since it was added separately and
intentionally in [JDK-8165296](https://bugs.openjdk.org/browse/JDK-8165296), I
would rather leave it alone on second thought.
> test/jdk/java/util/Calendar/bug4243802.java line 48:
>
>> 46: void setUp() {
>> 47: Locale.setDefault(Locale.US);
>> 48: TimeZone.setDefault(TimeZone.getTimeZone("America/Los_Angeles"));
>
> If the test is not going to restore the original defaults with `tearDown()`,
> I'd expect `othervm` explicitly on `@run` directive. (applies to other
> locations)
Thanks for correcting that, I had a misconception that JUnit would be ran with
an independent JVM (not potentially reused), which is why I removed the saving
of the default TZ and Locale. Adjusted so that they are preserved and reset
with a tearDown() method.
> test/jdk/java/util/Calendar/bug4316678.java line 58:
>
>> 56: @Test
>> 57: public void serializationTest() throws IOException,
>> ClassNotFoundException {
>> 58: TimeZone.setDefault(TimeZone.getTimeZone("PST"));
>
> Not needed?
Missed that, thank you.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/15853#discussion_r1334759820
PR Review Comment: https://git.openjdk.org/jdk/pull/15853#discussion_r1334759634
PR Review Comment: https://git.openjdk.org/jdk/pull/15853#discussion_r1334759740
