Make Test Report locale independent #868
Conversation
|
@bioball I also updated the description with (maybe) hopefuly information 🙃 |
| ) | ||
| assertThat(err.toString()).isEqualTo("") | ||
|
|
||
| Locale.setDefault(originalLocale) |
There was a problem hiding this comment.
These assertions will throw errors, which means that Locale.setDefault(...) won't get called if any of these tests fail.
Let's put lines 496 through 527 in a try/finally, where Locale.setDefault is in the finally so we can ensure that this test cleans up the state that it's mucking with
There was a problem hiding this comment.
Also note that mutating global JVM state is only safe as long as test execution is single-threaded (which is the default in Gradle).
There was a problem hiding this comment.
These assertions will throw errors, which means that Locale.setDefault(...) won't get called if any of these tests fail.
In case the test fails, won't the jvm "shutdown" anyways (and report the error)?
So doesn't this even matter to restore the default in that case?
Also note that mutating global JVM state is only safe as long as test execution is single-threaded (which is the default in Gradle).
Do we have the default here? 😅
When I running tests, it looks like Gradle runs the test parallel.
At least for different Gradle modules.
So I guess changing the Locale might also change the behviour on other modules/tests that runs at that time, correct? 🤔
There was a problem hiding this comment.
By default, each Test task starts one JVM and runs tests in a single thread. I'm not aware of Pkl overriding this default. Multiple Test tasks may nevertheless run in parallel.
fixes #861
According to AI (and javadoc 😅) the problem is that String.format takes Locale.getDefault if not specified. Indeed I have set de-EN on my machine so it takes comma decimals instead of dot decimals.
I haven't tested this yet, just created it on mobile...This is my macobok setting (not sure if relevant or the correct screen):
If I run the newly added test without
Locale.ROOTinSimpleReport, I get the following error:Running with that
Locale.ROOT:BUILD SUCCESSFUL in 37s🎉AI summary
This pull request includes changes to improve locale independence in the
CliTestRunnerand to ensure consistent locale usage in theSimpleReportclass. The most important changes include adding a locale independence test forCliTestRunnerand specifying the root locale for formatting inSimpleReport.Locale independence improvements:
pkl-cli/src/test/kotlin/org/pkl/cli/CliTestRunnerTest.kt: Added a new testCliTestRunner locale independence testto verify thatCliTestRunnerworks correctly regardless of the default locale.pkl-core/src/main/java/org/pkl/core/stdlib/test/report/SimpleReport.java: Importedjava.util.Localeto use for specifying locale in formatting.pkl-core/src/main/java/org/pkl/core/stdlib/test/report/SimpleReport.java: Modified themakeStatsLinemethod to useLocale.ROOTinString.formatto ensure consistent formatting regardless of the default locale.