Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Test Report locale independent #868

Merged
merged 6 commits into from
Jan 2, 2025
Merged

Make Test Report locale independent #868

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cabd005
Make SimpleReport locale independent
StefMa Dec 23, 2024
2f61b5d
Test for locale independence
StefMa Dec 23, 2024
b652fc8
Merge branch 'apple:main' into fix-test-report
StefMa Dec 24, 2024
33e756f
Add missing import
StefMa Dec 24, 2024
f94d715
Fix formatting
StefMa Dec 24, 2024
77f1571
Wrap test into try/finally
StefMa Dec 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions pkl-cli/src/test/kotlin/org/pkl/cli/CliTestRunnerTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import org.pkl.commons.readString
import org.pkl.commons.toUri
import org.pkl.commons.writeString
import org.pkl.core.Release
import java.util.Locale

class CliTestRunnerTest {
@Test
Expand Down Expand Up @@ -487,6 +488,47 @@ class CliTestRunnerTest {
)
}

@Test
fun `CliTestRunner locale independence test`(@TempDir tempDir: Path) {
val originalLocale = Locale.getDefault()
Locale.setDefault(Locale.GERMANY)

val code =
"""
amends "pkl:test"

facts {
["localeTest"] {
1 == 1
}
}
"""
.trimIndent()
val input = tempDir.resolve("test.pkl").writeString(code).toString()
val out = StringWriter()
val err = StringWriter()
val opts = CliBaseOptions(sourceModules = listOf(input.toUri()), settings = URI("pkl:settings"))
val testOpts = CliTestOptions()
val runner = CliTestRunner(opts, testOpts, consoleWriter = out, errWriter = err)
runner.run()

assertThat(out.toString().stripFileAndLines(tempDir))
.isEqualTo(
"""
module test
facts
✔ localeTest

100.0% tests pass [1 passed], 100.0% asserts pass [1 passed]

"""
.trimIndent()
)
assertThat(err.toString()).isEqualTo("")

Locale.setDefault(originalLocale)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also note that mutating global JVM state is only safe as long as test execution is single-threaded (which is the default in Gradle).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

private fun String.stripFileAndLines(tmpDir: Path): String {
// handle platform differences in handling of file URIs
// (file:/// on *nix vs. file:/ on Windows)
Expand Down
3 changes: 2 additions & 1 deletion pkl-core/src/main/java/org/pkl/core/stdlib/test/report/SimpleReport.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

import java.io.IOException;
import java.io.Writer;
import java.util.Locale;
import java.util.List;
import java.util.stream.Collectors;
import org.pkl.core.TestResults;
Expand Down Expand Up @@ -144,7 +145,7 @@ private void makeStatsLine(
sb.append(
color,
() ->
sb.append(String.format("%.1f%%", passRate)).append(" ").append(kind).append(" pass"));
sb.append(String.format(Locale.ROOT, "%.1f%%", passRate)).append(" ").append(kind).append(" pass"));

if (isFailed) {
sb.append(" [").append(failed).append('/').append(total).append(" failed]");
Expand Down