On Thu, 17 Nov 2022 16:59:09 GMT, Lukasz Kostyra <d...@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. 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: 1. `@Parametrized.Parameters`-tagged methods are grouped and called first in an undefined moment, before any test class is executed 2. 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. ------------- PR: https://git.openjdk.org/jfx/pull/954
