8265828: [TestBug] Save and restore the default Locale in javafx.base unit test LocalDateTimeStringConverterTest #954

lukostyra · 2022-11-17T16:59:09Z

The change moves Locale setting in the test to @BeforeClass and @AfterClass calls. @BeforeClass method call stores current default VM locale and applies Locale.US, while @AfterClass method restores old VM locale after all tests are completed.

I tested it both on Mac and Windows, in both cases Locale is changed, restored properly and tests pass.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8265828: [TestBug] Save and restore the default Locale in javafx.base unit test LocalDateTimeStringConverterTest

Reviewers

Kevin Rushforth (@kevinrushforth - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx pull/954/head:pull/954
$ git checkout pull/954

Update a local copy of the PR:
$ git checkout pull/954
$ git pull https://git.openjdk.org/jfx pull/954/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 954

View PR using the GUI difftool:
$ git pr show -t 954

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/954.diff

… unit test LocalDateTimeStringConverterTest

bridgekeeper · 2022-11-17T16:59:37Z

👋 Welcome back lukostyra! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

mlbridge · 2022-11-18T14:43:24Z

Webrevs

kevinrushforth

Looks good.

kevinrushforth · 2022-11-18T15:45:46Z

...s/javafx.base/src/test/java/test/javafx/util/converter/LocalDateTimeStringConverterTest.java

@@ -94,6 +93,17 @@ public LocalDateTimeStringConverterTest(LocalDateTimeStringConverter converter,
        this.parser = parser;
    }

+    @BeforeClass public static void setupBeforeAll() {


Minor: we usually put annotations on a separate line, although some files (like this one) put the @Test annotation on the same line, splitting them is preferred. I'll approve it as-is, and reapprove if you decide to change (I'll leave it up to you).

Since I have to make some changes to this PR, I will update this as well.

openjdk · 2022-11-18T15:47:03Z

@lukostyra This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8265828: [TestBug] Save and restore the default Locale in javafx.base unit test LocalDateTimeStringConverterTest

Reviewed-by: kcr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 2 new commits pushed to the master branch:

d040c1f: 8297680: JavaDoc example for PseudoClass has minor typo
3376228: 8294809: ListenerHelper for managing and disconnecting listeners

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kevinrushforth) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

Maran23 · 2022-11-18T16:00:33Z

...s/javafx.base/src/test/java/test/javafx/util/converter/LocalDateTimeStringConverterTest.java

@@ -56,11 +57,9 @@ public class LocalDateTimeStringConverterTest {

    private static final DateTimeFormatter aFormatter = DateTimeFormatter.ofPattern("dd MM yyyy HH mm ss");
    private static final DateTimeFormatter aParser = DateTimeFormatter.ofPattern("yyyy MM dd hh mm ss a");
+    private static Locale oldLocale;


Isn't the creation of the DateTimeFormatter using the default locale? If so, this should probably be done after the locale is set.

This is a good point. Moving the initialization of those two fields to the setupBeforeAll method seems safest.

That is a fair point.

I'll have to change the code a bit, as implementations() method is called before a @BeforeClass-tagged method (which is probably why originally Locale.setDefault() was called there) and aFormatter/aParser are already used there, expected to be initialized.

Maybe JUnit5 can help you here.
You can check out my recently merged PR for an example for the usage of the parameterized api introduced in JUnit5.
#936

I guess they fixed this rather irritating "feature" in JUnit5. Another, possibly less-intrusive, approach would be to initialize all of the static fields in a static { ... } block (with a comment as to why you can't use an @BeforeClass method.

kevinrushforth

Pending response to comment raised by Marius.

andy-goryachev-oracle · 2022-11-18T16:50:21Z

...s/javafx.base/src/test/java/test/javafx/util/converter/LocalDateTimeStringConverterTest.java

@@ -56,11 +57,9 @@ public class LocalDateTimeStringConverterTest {

    private static final DateTimeFormatter aFormatter = DateTimeFormatter.ofPattern("dd MM yyyy HH mm ss");
    private static final DateTimeFormatter aParser = DateTimeFormatter.ofPattern("yyyy MM dd hh mm ss a");
+    private static Locale oldLocale;


I wonder how many other tests we have that depend on specific Locale? Perhaps we need to apply the same treatment to:

LocalDateStringConverterTest

LocalTimeStringConverterTest

I could also change those, as they use DateTimeFormatter as well which uses Locale underneath as discussed above. @kevinrushforth what do you think?

Yes, it seems reasonable to include those tests as well, since those tests have the same problem.

kevinrushforth

see inline comments

kevinrushforth · 2022-11-18T17:17:29Z

...s/javafx.base/src/test/java/test/javafx/util/converter/LocalDateTimeStringConverterTest.java

@@ -56,11 +57,9 @@ public class LocalDateTimeStringConverterTest {

    private static final DateTimeFormatter aFormatter = DateTimeFormatter.ofPattern("dd MM yyyy HH mm ss");
    private static final DateTimeFormatter aParser = DateTimeFormatter.ofPattern("yyyy MM dd hh mm ss a");
+    private static Locale oldLocale;


I guess they fixed this rather irritating "feature" in JUnit5. Another, possibly less-intrusive, approach would be to initialize all of the static fields in a static { ... } block (with a comment as to why you can't use an @BeforeClass method.

kevinrushforth · 2022-11-18T17:18:34Z

...s/javafx.base/src/test/java/test/javafx/util/converter/LocalDateTimeStringConverterTest.java

@@ -56,11 +57,9 @@ public class LocalDateTimeStringConverterTest {

    private static final DateTimeFormatter aFormatter = DateTimeFormatter.ofPattern("dd MM yyyy HH mm ss");
    private static final DateTimeFormatter aParser = DateTimeFormatter.ofPattern("yyyy MM dd hh mm ss a");
+    private static Locale oldLocale;


Yes, it seems reasonable to include those tests as well, since those tests have the same problem.

…ocale * Locale initialization was moved to @BeforeClass method. * DateTimeFormatter objects are allocated after Locale initialization * Since LocalDateTimeStringConverter depends on DateTimeFormatter and on VM's Locale, creation of it was moved to @before method.

Treatment done in this commit is similar to the previous change.

lukostyra · 2022-11-25T15:04:38Z

After implementing CR fixes it turned out that these tests started to fail at random. Upon more investigation it turned out that the order in which JUnit calls test methods matters a lot. In general:

@Parametrized.Parameters-tagged methods are grouped and called first in an undefined moment, before any test class is executed
Then the order is more expected and predictable; for each test class JUnit calls: @BeforeClass, constructor, @Before, test, @After, ..., @AfterClass.

Additionally, Test Class order is selected at random.

That means that modifying a VM-wide setting like Locale cannot reside in @Parametrized.Parameters method or in static initializer block. When I added Locale change in all Local*StringConverterTest test classes, during step 1. one test captured machine's default Locale and changed it to en-US, while other tests captured previously set en-US Locale and set it again to en-US. Then JUnit progresses into step 2 and executes each test class, so once a test class completes, it might happen to be the one that captured machine's default Locale. It will revert Locale for other tests, effectively breaking them.

I had to refactor initialization code for these tests in order to resolve all problems and make them more independent. In short:

Locale change has to be done in @BeforeClass-tagged static method
Locale revert has to happen in @AfterClass-tagged static method
DateTimeFormatter instances have to be created after Locale is set, also in @BeforeClass method
Local*StringConverter classes that are checked by these tests have to be allocated afterwards, as they are also depending on set Locale and sometimes on DateTimeFormatter instances created earlier.
Test also acquired Format Locale for further use, so that part had to be moved after Locale setup as well.
As all these fields (like Local*StringConverter instance or current format Locale) are members of the test class, they must be setup somewhere after @BeforeClass method. The best spot I had for these is a @Before-tagged method.

The solution I made does not feel the most convenient but with above constraints it was difficult to come up with anything cleaner/less intrusive. This might also be a side-effect of me not knowing a mechanism in JUnit that could help us in such situation, so if there are any bits worth looking into I'd be happy to add them and simplify this change a bit.

openjdk · 2022-11-25T15:10:14Z

@lukostyra this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8265828-locale
git fetch https://git.openjdk.org/jfx master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

kevinrushforth · 2022-11-28T14:37:37Z

After implementing CR fixes it turned out that these tests started to fail at random. Upon more investigation it turned out that the order in which JUnit calls test methods matters a lot.

Thank you for getting to the bottom of this.

In general:

@Parametrized.Parameters-tagged methods are grouped and called first in an undefined moment, before any test class is executed

This was the surprising find, and explains the odd problems you were seeing.

Then the order is more expected and predictable; for each test class JUnit calls: @BeforeClass, constructor, @Before, test, @After, ..., @AfterClass.

Additionally, Test Class order is selected at random.

Right. This is documented behavior and is (or at least should be) well understood. It's why we are careful to avoid mutating static data or global state in a @Test method (at least not without restoring it in an @After method).

That means that modifying a VM-wide setting like Locale cannot reside in @Parametrized.Parameters method or in static initializer block.

Yes, this is the conclusion I would draw as well.

…-locale

kevinrushforth

The refactoring looks good to me. I left a few comments on one of the tests. Those comments apply to all of them.

modules/javafx.base/src/test/java/test/javafx/util/converter/LocalDateStringConverterTest.java

...s/javafx.base/src/test/java/test/javafx/util/converter/LocalDateTimeStringConverterTest.java

modules/javafx.base/src/test/java/test/javafx/util/converter/LocalTimeStringConverterTest.java

kevinrushforth

Looks good. You can remove the now-unused import of java.security.InvalidParameterException (I'll reapprove when you do).

lukostyra · 2022-11-29T14:35:05Z

/integrate

openjdk · 2022-11-29T14:38:15Z

@lukostyra
Your change (at version f0fa376) is now ready to be sponsored by a Committer.

kevinrushforth · 2022-11-29T14:49:51Z

/sponsor

openjdk · 2022-11-29T14:51:31Z

Going to push as commit 4ad8582.
Since your change was applied there have been 2 commits pushed to the master branch:

d040c1f: 8297680: JavaDoc example for PseudoClass has minor typo
3376228: 8294809: ListenerHelper for managing and disconnecting listeners

Your commit was automatically rebased without conflicts.

openjdk · 2022-11-29T14:51:58Z

@kevinrushforth @lukostyra Pushed as commit 4ad8582.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8265828: [TestBug] Save and restore the default Locale in javafx.base…

1750b24

… unit test LocalDateTimeStringConverterTest

openjdk bot added the rfr Ready for review label Nov 18, 2022

kevinrushforth approved these changes Nov 18, 2022

View reviewed changes

openjdk bot added the ready Ready to be integrated label Nov 18, 2022

Maran23 reviewed Nov 18, 2022

View reviewed changes

kevinrushforth self-requested a review November 18, 2022 16:33

kevinrushforth requested changes Nov 18, 2022

View reviewed changes

kevinrushforth self-requested a review November 18, 2022 16:35

openjdk bot removed the ready Ready to be integrated label Nov 18, 2022

andy-goryachev-oracle reviewed Nov 18, 2022

View reviewed changes

kevinrushforth reviewed Nov 18, 2022

View reviewed changes

kevinrushforth self-requested a review November 18, 2022 22:13

lukostyra added 2 commits November 25, 2022 14:54

Refactor remaining LocalStringConverter tests

35b7847

Treatment done in this commit is similar to the previous change.

openjdk bot added merge-conflict Pull request has merge conflict with target branch and removed rfr Ready for review labels Nov 25, 2022

Merge branch 'master' of https://git.openjdk.org/jfx into JDK-8265828…

681623d

…-locale

openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Nov 28, 2022

Remove trailing whitespaces

e5cd522

openjdk bot added the rfr Ready for review label Nov 28, 2022

kevinrushforth reviewed Nov 28, 2022

View reviewed changes

kevinrushforth self-requested a review November 28, 2022 15:01

andy-goryachev-oracle suggested changes Nov 28, 2022

View reviewed changes

Fix code review comments

b1db3c0

kevinrushforth approved these changes Nov 29, 2022

View reviewed changes

openjdk bot added the ready Ready to be integrated label Nov 29, 2022

Remove unnecessary InvalidParameterException import

f0fa376

openjdk bot removed the ready Ready to be integrated label Nov 29, 2022

kevinrushforth approved these changes Nov 29, 2022

View reviewed changes

openjdk bot added the ready Ready to be integrated label Nov 29, 2022

openjdk bot added the sponsor Ready to sponsor label Nov 29, 2022

openjdk bot added the integrated Pull request has been integrated label Nov 29, 2022

openjdk bot closed this Nov 29, 2022

openjdk bot removed ready Ready to be integrated rfr Ready for review sponsor Ready to sponsor labels Nov 29, 2022

lukostyra deleted the JDK-8265828-locale branch November 29, 2022 14:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8265828: [TestBug] Save and restore the default Locale in javafx.base unit test LocalDateTimeStringConverterTest #954

8265828: [TestBug] Save and restore the default Locale in javafx.base unit test LocalDateTimeStringConverterTest #954

lukostyra commented Nov 17, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Nov 17, 2022

mlbridge bot commented Nov 18, 2022 •

edited

Loading

kevinrushforth left a comment

kevinrushforth Nov 18, 2022

lukostyra Nov 18, 2022

openjdk bot commented Nov 18, 2022 •

edited

Loading

Maran23 Nov 18, 2022 •

edited

Loading

kevinrushforth Nov 18, 2022

lukostyra Nov 18, 2022

Maran23 Nov 18, 2022

kevinrushforth Nov 18, 2022

kevinrushforth left a comment

andy-goryachev-oracle Nov 18, 2022

lukostyra Nov 18, 2022

kevinrushforth Nov 18, 2022

kevinrushforth left a comment

kevinrushforth Nov 18, 2022

kevinrushforth Nov 18, 2022

lukostyra commented Nov 25, 2022

openjdk bot commented Nov 25, 2022

kevinrushforth commented Nov 28, 2022

kevinrushforth left a comment

kevinrushforth left a comment

lukostyra commented Nov 29, 2022

openjdk bot commented Nov 29, 2022

kevinrushforth commented Nov 29, 2022

openjdk bot commented Nov 29, 2022

openjdk bot commented Nov 29, 2022

8265828: [TestBug] Save and restore the default Locale in javafx.base unit test LocalDateTimeStringConverterTest #954

8265828: [TestBug] Save and restore the default Locale in javafx.base unit test LocalDateTimeStringConverterTest #954

Conversation

lukostyra commented Nov 17, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Nov 17, 2022

mlbridge bot commented Nov 18, 2022 • edited Loading

Webrevs

kevinrushforth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Nov 18, 2022 • edited Loading

Maran23 Nov 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinrushforth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinrushforth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukostyra commented Nov 25, 2022

openjdk bot commented Nov 25, 2022

kevinrushforth commented Nov 28, 2022

kevinrushforth left a comment

Choose a reason for hiding this comment

kevinrushforth left a comment

Choose a reason for hiding this comment

lukostyra commented Nov 29, 2022

openjdk bot commented Nov 29, 2022

kevinrushforth commented Nov 29, 2022

openjdk bot commented Nov 29, 2022

openjdk bot commented Nov 29, 2022

lukostyra commented Nov 17, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 18, 2022 •

edited

Loading

openjdk bot commented Nov 18, 2022 •

edited

Loading

Maran23 Nov 18, 2022 •

edited

Loading