Re: RFR: JDK-8316559: Refactor some util/Calendar tests to JUnit [v2]
On Fri, 22 Sep 2023 20:45:39 GMT, Lance Andersen wrote: > Overall, this is fine. > > I would like to suggest comments to introduce all tests and DataProviders. > Extra credit for helper methods. > > From a future maintainers Point of view, having more info in the tests is > beneficial. Thanks for reviewing Lance. Agreed, there's been times where I've wished there was more context in the test file itself, rather than having to navigate to the JBS issue or older diffs for more info. Updated to have comments for tests, data providers, and helper methods as you suggested (will do the same for future JUnit conversions). - PR Comment: https://git.openjdk.org/jdk/pull/15853#issuecomment-1732078935
Re: RFR: JDK-8316559: Refactor some util/Calendar tests to JUnit [v2]
On Fri, 22 Sep 2023 19:50:49 GMT, Justin Lu wrote: >> Please review this PR which converts some tests under _Calendar_ to use >> JUnit. These tests either previously used the internal _IntlTest_, or used >> no framework at all. >> >> Any files named BugXXX.java will be renamed after review. > > 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 Overall, this is fine. I would like to suggest comments to introduce all tests and DataProviders. Extra credit for helper methods. >From a future maintainers Point of view, having more info in the tests is >beneficial. - Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15853#pullrequestreview-1640620487
Re: RFR: JDK-8316559: Refactor some util/Calendar tests to JUnit [v2]
On Fri, 22 Sep 2023 19:50:49 GMT, Justin Lu wrote: >> Please review this PR which converts some tests under _Calendar_ to use >> JUnit. These tests either previously used the internal _IntlTest_, or used >> no framework at all. >> >> Any files named BugXXX.java will be renamed after review. > > 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 Marked as reviewed by naoto (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/15853#pullrequestreview-1640604118
Re: RFR: JDK-8316559: Refactor some util/Calendar tests to JUnit [v2]
On Fri, 22 Sep 2023 19:44:00 GMT, Justin Lu wrote:
>> 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.
Not sure why IntelliJ claims it redundant, but compiling the test case alone,
without that `@SuppressWarnings` with `-Xlint` gives me this:
warning: [serial] serializable class MyCalendar has no definition of
serialVersionUID
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15853#discussion_r1334788356
Re: RFR: JDK-8316559: Refactor some util/Calendar tests to JUnit [v2]
On Thu, 21 Sep 2023 22:18:04 GMT, Naoto Sato 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
Re: RFR: JDK-8316559: Refactor some util/Calendar tests to JUnit [v2]
> Please review this PR which converts some tests under _Calendar_ to use > JUnit. These tests either previously used the internal _IntlTest_, or used no > framework at all. > > Any files named BugXXX.java will be renamed after review. 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 - Changes: - all: https://git.openjdk.org/jdk/pull/15853/files - new: https://git.openjdk.org/jdk/pull/15853/files/43e17eb8..a0cfa852 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15853&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15853&range=00-01 Stats: 51 lines in 7 files changed: 35 ins; 1 del; 15 mod Patch: https://git.openjdk.org/jdk/pull/15853.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15853/head:pull/15853 PR: https://git.openjdk.org/jdk/pull/15853
Re: RFR: JDK-8316559: Refactor some util/Calendar tests to JUnit [v2]
On Fri, 22 Sep 2023 06:44:16 GMT, Andrey Turbanov 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/BuddhistCalendarTest.java line 141:
>
>> 139: @Test
>> 140: public void buddhistSetTest() {
>> 141: Calendar cal = getBuddhistCalendar();
>
> Suggestion:
>
> Calendar cal = getBuddhistCalendar();
Fixed, thank you.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15853#discussion_r1334759775
