Force the default locale to have intTest working even if the default locale is not en_US #886
Conversation
…locale is not en_US
jbonofre
requested review from
adutra,
ashvina,
dennishuo,
dimas-b,
eric-maynard,
jackye1995,
vvcephei,
collado-mike,
snazy,
RussellSpitzer,
takidau,
flyrain and
ebyhr
as code owners
| @@ -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") | |||
There was a problem hiding this comment.
What is the issue if the locale is not en_US?
Don't we need this for test as well?
There was a problem hiding this comment.
IDK about what JB hit, but in general String upper/lower case conversion depends on the locale even for the ASCII char sub-set
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
Also, we can fix string manipulation in Polaris code, but I would not be 100% sure about dependencies :)
There was a problem hiding this comment.
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 ;-)
There was a problem hiding this comment.
True, I'm totally fine preparing for i18n 👍
Let's then fix the test or the related server code that fails in #885 ?
There was a problem hiding this comment.
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 ;)
|
What's our thoughts about this one ? |
|
cc @gh-yzou |
I would personally fix the test by making it resilient to other locales. |
|
Do I understand it correctly that the test fails under some locale because the error message produced by the server running with that locale is not in English? |
|
It feels little bit wired to me that we are enforcing this setup for all tests, will we be able to set those up for the particular test points? |
|
From my POV, this may be a signal of a bigger problem. If the server provides error messages (to be confirmed) in different language or chartsets depending on its locale, that would lead to a degraded UX from the client's perspective because clients (humans or programs) do not necessarily know the locale of the server and may run in a different locale. Let's confirm the basic server behaviour in this case first. |
No description provided.