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

Force the default locale to have intTest working even if the default locale is not en_US #886

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Force the default locale to have intTest working even if the default locale is not en_US #886

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 2 additions & 0 deletions quarkus/service/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,8 @@ tasks.named<Test>("intTest").configure {
// Same issue as above: allow a java security manager after Java 21
// (this setting is for the application under test, while the setting above is for test code).
systemProperty("quarkus.test.arg-line", "-Djava.security.manager=allow")
// force the locale, just in case the user is having another default locale
systemProperty("quarkus.default-locale", "en_US")
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the issue if the locale is not en_US?
Don't we need this for test as well?

Copy link
Contributor

@dimas-b dimas-b Jan 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

IDK about what JB hit, but in general String upper/lower case conversion depends on the locale even for the ASCII char sub-set

Copy link
Contributor

Choose a reason for hiding this comment

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

In other words, I'd prefer to have this setting fixed in the main server properties, not just in test.

Are there reasons to run the Server under a custom locale?

Copy link
Member Author

@jbonofre jbonofre Jan 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

For instance https://github.com/apache/polaris/blob/main/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisManagementServiceIntegrationTest.java#L366 and https://github.com/apache/polaris/blob/main/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisManagementServiceIntegrationTest.java#L383 assumed the locale is en_US: it expects must not be null.

If the locale is not en_US, let say fr_FR, then the tests fail as we have ne doit pas être nul instead of must not be null.

Same failures can happen with any default locale not en_US.

Copy link
Contributor

Choose a reason for hiding this comment

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

OTOH, don't we want to have i18n in Polaris? French customers may be happier reading error messages in French. Setting the locale to en_US sounds very... US centric :-) Can't we adapt the tests instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

@adutra that's a good point, but I think there's more work to do for i18n support. Maybe we can do step by step. My PR is only in the integration tests here. So maybe we can start by this and create an issue for i18n.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, we can fix string manipulation in Polaris code, but I would not be 100% sure about dependencies :)

Copy link
Contributor

Choose a reason for hiding this comment

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

There is definitely more work than just that to get good i18n, but OTOH if we carve the locale in stone as en_US, that's not a good start ;-)

Copy link
Contributor

Choose a reason for hiding this comment

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

True, I'm totally fine preparing for i18n 👍

Let's then fix the test or the related server code that fails in #885 ?

Copy link
Member

Choose a reason for hiding this comment

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

Question: Since quarkus.default-locale is a Quarkus setting, why no put it in application.properties? I suspect all servers should behave the same way - even on French systems ;)

val logsDir = project.layout.buildDirectory.get().asFile.resolve("logs")
// delete files from previous runs
doFirst {
Expand Down
Loading