Re: RFR: JDK-8316559: Refactor some util/Calendar tests to JUnit [v6]
> 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 one additional commit since the last revision: Revert "rename files from bugXXX to something more descriptive" Renaming the files messes up the version history. This reverts commit a65b30987ba030c7698351b3afbc328b4eeb797b. - Changes: - all: https://git.openjdk.org/jdk/pull/15853/files - new: https://git.openjdk.org/jdk/pull/15853/files/a65b3098..4e60ce55 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15853=05 - incr: https://webrevs.openjdk.org/?repo=jdk=15853=04-05 Stats: 18 lines in 9 files changed: 0 ins; 0 del; 18 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 [v5]
> 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 one additional commit since the last revision: rename files from bugXXX to something more descriptive - Changes: - all: https://git.openjdk.org/jdk/pull/15853/files - new: https://git.openjdk.org/jdk/pull/15853/files/db54d231..a65b3098 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15853=04 - incr: https://webrevs.openjdk.org/?repo=jdk=15853=03-04 Stats: 18 lines in 9 files changed: 0 ins; 0 del; 18 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 [v4]
> 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 one additional commit since the last revision: white space - Changes: - all: https://git.openjdk.org/jdk/pull/15853/files - new: https://git.openjdk.org/jdk/pull/15853/files/99fd7b24..db54d231 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15853=03 - incr: https://webrevs.openjdk.org/?repo=jdk=15853=02-03 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 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 [v3]
> 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 one additional commit since the last revision: Add more comments, drop @library tag for tests previously extending IntlTest - Changes: - all: https://git.openjdk.org/jdk/pull/15853/files - new: https://git.openjdk.org/jdk/pull/15853/files/a0cfa852..99fd7b24 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15853=02 - incr: https://webrevs.openjdk.org/?repo=jdk=15853=01-02 Stats: 101 lines in 9 files changed: 69 ins; 4 del; 28 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 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=15853=01 - incr: https://webrevs.openjdk.org/?repo=jdk=15853=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
Re: RFR: JDK-8316559: Refactor some util/Calendar tests to JUnit
On Wed, 20 Sep 2023 23:20:43 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.
test/jdk/java/util/Calendar/BuddhistCalendarTest.java line 141:
> 139: @Test
> 140: public void buddhistSetTest() {
> 141: Calendar cal = getBuddhistCalendar();
Suggestion:
Calendar cal = getBuddhistCalendar();
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15853#discussion_r1333947857
Re: RFR: JDK-8316559: Refactor some util/Calendar tests to JUnit
On Wed, 20 Sep 2023 23:20:43 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.
test/jdk/java/util/Calendar/Bug4766302.java line 32:
> 30: import java.util.GregorianCalendar;
> 31:
> 32: @SuppressWarnings("serial")
Is removing this OK?
test/jdk/java/util/Calendar/bug4028518.java line 41:
> 39: public class bug4028518 {
> 40:
> 41: // Ensure modifying cloned gregCalendar does not modify the original
Seems that the comment is saying the other way around
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)
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?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15853#discussion_r1333653061
PR Review Comment: https://git.openjdk.org/jdk/pull/15853#discussion_r1333655945
PR Review Comment: https://git.openjdk.org/jdk/pull/15853#discussion_r1333663335
PR Review Comment: https://git.openjdk.org/jdk/pull/15853#discussion_r1333665846
