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

[Kevin Rushforth]

On Mon, 28 Nov 2022 14:44:17 GMT, Lukasz Kostyra <duke at openjdk.org> wrote:

>> 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.
>
> Lukasz Kostyra has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains four commits:
> 
>  - Merge branch 'master' of https://git.openjdk.org/jfx into JDK-8265828-locale
>  - Refactor remaining LocalStringConverter tests
>    
>    Treatment done in this commit is similar to the previous change.
>  - LocalDateTimeStringConverterTest: Refactor test to properly utilize Locale
>    
>    * 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.
>  - 8265828: [TestBug] Save and restore the default Locale in javafx.base unit test LocalDateTimeStringConverterTest

modules/javafx.base/src/test/java/test/javafx/util/converter/LocalDateStringConverterTest.java line 54:

> 52: @RunWith(Parameterized.class)
> 53: public class LocalDateStringConverterTest {
> 54:     private static final LocalDate VALID_DATE = LocalDate.of(1985, 1, 12);

I presume that this is not locale sensitive?

modules/javafx.base/src/test/java/test/javafx/util/converter/LocalDateStringConverterTest.java line 123:

> 121:     @Before
> 122:     public void setup() {
> 123:         if (this.converter == null) {

There is no need to check for `null` here. It always will be, since `converter` is initialized to `null` in the constructor and there is no other assignment.

modules/javafx.base/src/test/java/test/javafx/util/converter/LocalDateStringConverterTest.java line 143:

> 141:                 break;
> 142:             default:
> 143:                 throw new InvalidParameterException("Invalid converter variant: " + this.converterVariant.toString());

This does not seem like the right exception to throw. I would just use `fail(String)` here.

modules/javafx.base/src/test/java/test/javafx/util/converter/LocalDateStringConverterTest.java line 150:

> 148:     @After
> 149:     public void teardown() {
> 150:     }

Minor: Since this method is empty, I think you can remove it (along with the import).

-------------

PR: https://git.openjdk.org/jfx/pull/954