Fix the way locale information is passed to Surefire #315

ksobolew · 2022-05-25T15:21:54Z

This moves primarily the user.language and user.region from systemPropertyVariables to explicit property setting on the JVM. These two methods to set them are supposed to be the same, but passing via system properties does not actually work. Which is annoying, because when the are set in both ways, Surefire complains about it:

[WARN] The system property user.language is configured twice! The property appears in <argLine/> and any of <systemPropertyVariables/>, <systemProperties/> or user property.
[WARN] The system property user.region is configured twice! The property appears in <argLine/> and any of <systemPropertyVariables/>, <systemProperties/> or user property.

It seems that the user.timezone actually works as expected, but is moved along with the other two for consistency.

ksobolew · 2022-05-25T15:22:12Z

cc: @findepi

airbase/pom.xml

findepi · 2022-05-25T21:31:18Z

These two are supposed to be the same, but passing via system properties does not actually work.

See #98 (comment)

Does this mean #98 never actually worked?
@findepi reported prestodb/presto#9313 (comment) it did work.

@ksobolew how do you test these changes?

ksobolew · 2022-05-26T07:57:57Z

@ksobolew how do you test these changes?

I just run ./mvnw verify -pl :trino-main -Dtest=TestRealOperators in Trino. My locale is pl_PL, so I get test failures of the sort: java.lang.AssertionError: expected [7.541985E2] but found [7,541985E2], which clearly indicates that the system default locale was used instead of the one specified in the Surefire config.

@findepi reported prestodb/presto#9313 (comment) it did work.

Maybe it broke in the meantime? That configuration was added 4 years ago... I did some googling and found problem reports about that, but no fix in Surefire was ever mentioned.

findepi · 2022-05-26T08:32:02Z

Thanks for testing this out.
Do we also need to move the time zone?
Perhaps not, for the timezone we have https://github.com/findepi/trino/blob/7fd62db50aec490907e93412441fe65dbfea6052/testing/trino-tests/src/test/java/io/trino/tests/TestVerifyTrinoTestsTestSetup.java#L28.

Does this test pass for you locally?

(even if it does, i would somewhat prefer to set timezone region and lang same way)

cc @electrum

ksobolew · 2022-05-26T09:06:24Z

Thanks for testing this out. Do we also need to move the time zone? Perhaps not, for the timezone we have https://github.com/findepi/trino/blob/7fd62db50aec490907e93412441fe65dbfea6052/testing/trino-tests/src/test/java/io/trino/tests/TestVerifyTrinoTestsTestSetup.java#L28.

Maybe we need similar test for the locale?

Does this test pass for you locally?

Yes, this makes the tests pass for me.

ksobolew · 2022-05-26T09:25:04Z

Maybe we need similar test for the locale?

See https://github.com/trinodb/trino/pull/12560/files#diff-30ea3cddc958abc26c37e93c555a0f8cbc422ca55e7b1cc907a1e1aea72fcd6fR33

ksobolew · 2022-05-26T09:28:14Z

Addressed comments. One interesting thing to note is that this is actually broken in Maven, but IntelliJ correctly passes these parameters so these test work fine in the IDE.

findepi · 2022-05-27T11:23:13Z

airbase/pom.xml

@@ -440,6 +437,9 @@
                        <!-- ${argLine} is for Jacoco: https://www.eclemma.org/jacoco/trunk/doc/prepare-agent-mojo.html -->
                        <argLine>
                            ${argLine}
+                            -Duser.timezone=${air.test.timezone}


add a code comment why this goes in argsLine and not systemPropertyVariables

This moves primarily the `user.language` and `user.region` from `systemPropertyVariables` to explicit property setting on the JVM. These two methods to set them are supposed to be the same, but passing via system properties does not actually work. Which is annoying, because when the are set in both ways, Surefire complains about it: ``` [WARN] The system property user.language is configured twice! The property appears in <argLine/> and any of <systemPropertyVariables/>, <systemProperties/> or user property. [WARN] The system property user.region is configured twice! The property appears in <argLine/> and any of <systemPropertyVariables/>, <systemProperties/> or user property. ``` It seems that the `user.timezone` actually works as expected, but is moved along with the other two for consistency.

findepi · 2022-06-22T08:21:51Z

As a result of this change, my IntelliJ no longer sets -Duser.timezone=${air.test.timezone} when running tests, so they fail to start because of checks like
https://github.com/trinodb/trino/blob/4aacac24cb2811970abaa5acfa47427206568562/plugin/trino-hive/src/test/java/io/trino/plugin/hive/HiveQueryRunner.java#L238
or
https://github.com/trinodb/trino/blob/b2fb2b5a88e7452348e8f67d379fb555d3c49a59/plugin/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlTypeMapping.java#L153

@ksobolew does it work on your side?

ksobolew · 2022-06-22T08:26:06Z

Hm, I haven't encountered any problems 🤔

ksobolew · 2022-06-22T08:26:54Z

Not yet, anyway

findepi · 2022-06-22T08:37:53Z

Hm, I haven't encountered any problems 🤔

Do Trino's TestPostgreSqlTypeMapping and TestHiveConnectorTest work for you?

ksobolew · 2022-06-22T09:12:13Z

You're right, it's not working in IDEA 😞

ksobolew · 2022-06-22T10:11:27Z

Looks like IDEA is confused by ${argLine} in Airbase's surefire configuration:

                        <argLine>
                            ${argLine}
                            ...
                        </argLine>

Once I removed it, it worked. Thing is, if there's no Jacoco, this does not get expanded and sits there as literal ${argLine}.

nineinchnick · 2022-06-22T10:18:57Z

See #300 that should provide a default value when Jacoco is disabled

nineinchnick · 2022-06-22T10:20:28Z

Would definining the properties twice, both in systemPropertyVariables and argLine work?

findepi · 2022-06-22T10:28:05Z

See #300 that should provide a default value when Jacoco is disabled

Seems like it was merged long ago, and IntelliJ doesn't pick the time zone ~today.

Would definining the properties twice, both in systemPropertyVariables and argLine work?

would prefer to avoid duplication, but maybe we need to do this

this will make surefire issue warnings, like mentioned in the PR description

These two methods to set them are supposed to be the same, but passing via system properties does not actually work. Which is annoying, because when the are set in both ways, Surefire complains about it: [...]

It seems that the user.timezone actually works as expected, but is moved along with the other two for consistency.

or maybe we move back the user.timezone (provided IntelliJ can pick the other to)

BTW @ksobolew is there a surefire bug report about this?

ksobolew · 2022-06-22T10:37:06Z

See #300 that should provide a default value when Jacoco is disabled

It was then changed in #301 to activate on air.check.skip-all instead of air.check.skip-jacoco (for a valid reason that it didn't work). But in Trino we explicitly set <air.check.skip-jacoco>true</air.check.skip-jacoco> so the profile jacoco-check still gets activated. So jacoco-defaults does not and argLine is not set.

ksobolew · 2022-06-22T10:37:46Z

BTW @ksobolew is there a surefire bug report about this?

Actually with Surefire it works just fine

ksobolew · 2022-06-22T10:46:49Z

Would definining the properties twice, both in systemPropertyVariables and argLine work?

Not sure, probably yes. But there are two "but"s:

Specifying both is not an error, but the runner logs warnings about it (not fatal, so we can live with it)
This actually prevents passing all additional arguments to IDEA (though probably not all are useful when running in the IDE)

nineinchnick · 2022-06-22T10:47:03Z

Can Trino define a default for argLine if it has Jacoco always disabled?

ksobolew · 2022-06-22T10:48:25Z

Can Trino define a default for argLine if it has Jacoco always disabled?

I think it can. This will make enabling jacoco harder, though.

Can we activate the jacoco profiles explicitly?

findepi · 2022-06-22T11:16:38Z

BTW @ksobolew is there a surefire bug report about this?

Actually with Surefire it works just fine

This PR (#315) is a workaround for somre surefire's behavior.
Was that problematic behavior reported?

This actually prevents passing all additional arguments to IDEA (though probably not all are useful when running in the IDE)

Can you expand on this? What would be worse than it is today, or before this PR was merged?

Can Trino define a default for argLine if it has Jacoco always disabled?

The bug is in airbase. In trino we can workaround, sure, but can we fix this here?

ksobolew · 2022-06-22T11:21:37Z

Was that problematic behavior reported?

Oh, right :) I guess not. The best I could find was https://stackoverflow.com/questions/8901880/set-surefire-plugin-locale-in-maven-pom-file

The bug is in airbase. In trino we can workaround, sure, but can we fix this here?

Fair question. I hope so!

findepi · 2022-06-22T11:26:07Z

Was that problematic behavior reported?

Oh, right :) I guess not. T

We should have reported this before/while working on this PR. Can you please follow-up accordingly?
(Also, maybe it's easy to fix in the surefire plugin.)

ksobolew · 2022-06-22T12:37:41Z

Reported it as https://issues.apache.org/jira/browse/SUREFIRE-2104

ksobolew · 2022-06-22T12:41:09Z

This actually prevents passing all additional arguments to IDEA (though probably not all are useful when running in the IDE)

Can you expand on this? What would be worse than it is today, or before this PR was merged?

The presence of the unexpanded ${argLine} causes IDEA to not include anything that's in the argLine configuration element. So this is a pre-existing issue, we only started to see it "thanks" to these two properties.

findepi · 2023-05-08T10:39:40Z

Can Trino define a default for argLine if it has Jacoco always disabled?

I was not successful in making this work.

IntelliJ no longer sets -Duser.timezone=${air.test.timezone} when running tests, so they fail to start because of checks like
https://github.com/trinodb/trino/blob/4aacac24cb2811970abaa5acfa47427206568562/plugin/trino-hive/src/test/java/io/trino/plugin/hive/HiveQueryRunner.java#L238
or
https://github.com/trinodb/trino/blob/b2fb2b5a88e7452348e8f67d379fb555d3c49a59/plugin/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlTypeMapping.java#L153

is it only for me, or is this problem persistent?
@ksobolew how did you solve this on your side?

ksobolew · 2023-05-22T10:50:52Z

Can Trino define a default for argLine if it has Jacoco always disabled?

I was not successful in making this work.

IntelliJ no longer sets -Duser.timezone=${air.test.timezone} when running tests, so they fail to start because of checks like
https://github.com/trinodb/trino/blob/4aacac24cb2811970abaa5acfa47427206568562/plugin/trino-hive/src/test/java/io/trino/plugin/hive/HiveQueryRunner.java#L238
or
https://github.com/trinodb/trino/blob/b2fb2b5a88e7452348e8f67d379fb555d3c49a59/plugin/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlTypeMapping.java#L153

is it only for me, or is this problem persistent? @ksobolew how did you solve this on your side?

It was some time ago and I don't remember exactly what the problem was, but I checked now and indeed, it doesn't work. In fact, it seems that IDEA does not pick any of the arguments specified inside argLine. Reverting this commit at least makes the timezone to be passed correctly, which is the opposite of what seemingly was the original issue.

findepi · 2023-05-23T14:42:19Z

Reverting this commit at least makes the timezone to be passed correctly

awesome, let's do this, thanks!

which is the opposite of what seemingly was the original issue.

the PR title talks about locale

ksobolew · 2023-05-23T15:33:12Z

which is the opposite of what seemingly was the original issue.

the PR title talks about locale

These are all system properties so they are handled the same way. We pin locale for the same reason we pin the time zone, and they have the same issues.

ksobolew · 2023-05-23T15:39:13Z

and they have the same issues

I read the original commit message and it seems that it's wrong - they are not handled the same way.

findepi · 2023-05-24T09:30:13Z

Is there anything preventing us from restoring correct time zone handling for intellij?

ksobolew · 2023-05-24T09:36:37Z

No, I don't think there is anything that prevents us, except for making sure that it works.

We want to see if anything breaks. There's a suspicion that this is not an issue anymore, and causes issues of its own.

ksobolew · 2023-05-25T12:57:40Z

@findepi did the partial revert in #358. Would be cool if you could verify locally if it fixes the issue for you (I did some tests and it seems it does for me).

findepi reviewed May 25, 2022

View reviewed changes

airbase/pom.xml Outdated Show resolved Hide resolved

ksobolew force-pushed the kudi/fix-locale-setting branch from 70dc6b7 to f099204 Compare May 26, 2022 09:19

ksobolew mentioned this pull request May 26, 2022

Bump Airbase to 126 trinodb/trino#12560

Merged

ksobolew force-pushed the kudi/fix-locale-setting branch from f099204 to 4634826 Compare May 26, 2022 09:26

ksobolew force-pushed the kudi/fix-locale-setting branch from 4634826 to 5e4e918 Compare May 27, 2022 08:44

findepi reviewed May 27, 2022

View reviewed changes

ksobolew force-pushed the kudi/fix-locale-setting branch from 5e4e918 to 70ca37d Compare May 27, 2022 12:21

findepi approved these changes May 30, 2022

View reviewed changes

findepi merged commit d4630aa into airlift:master May 31, 2022

ksobolew deleted the kudi/fix-locale-setting branch May 31, 2022 08:17

findepi mentioned this pull request Jun 22, 2022

Explicitly set argLine property trinodb/trino#12928

Closed

ksobolew added a commit to ksobolew/trino that referenced this pull request May 25, 2023

[TEMP] Effectively revert airlift/airbase#315

1880677

We want to see if anything breaks. There's a suspicion that this is not an issue anymore, and causes issues of its own.

Fix the way locale information is passed to Surefire #315

Fix the way locale information is passed to Surefire #315

Conversation

ksobolew commented May 25, 2022 • edited Loading

ksobolew commented May 25, 2022

findepi commented May 25, 2022

ksobolew commented May 26, 2022 • edited Loading

findepi commented May 26, 2022

ksobolew commented May 26, 2022

ksobolew commented May 26, 2022

ksobolew commented May 26, 2022

findepi May 27, 2022

Choose a reason for hiding this comment

ksobolew May 27, 2022

Choose a reason for hiding this comment

findepi commented Jun 22, 2022

ksobolew commented Jun 22, 2022

ksobolew commented Jun 22, 2022

findepi commented Jun 22, 2022

ksobolew commented Jun 22, 2022

ksobolew commented Jun 22, 2022

nineinchnick commented Jun 22, 2022

nineinchnick commented Jun 22, 2022

findepi commented Jun 22, 2022

ksobolew commented Jun 22, 2022

ksobolew commented Jun 22, 2022

ksobolew commented Jun 22, 2022

nineinchnick commented Jun 22, 2022

ksobolew commented Jun 22, 2022

findepi commented Jun 22, 2022 • edited Loading

ksobolew commented Jun 22, 2022

findepi commented Jun 22, 2022

ksobolew commented Jun 22, 2022

ksobolew commented Jun 22, 2022

findepi commented May 8, 2023

ksobolew commented May 22, 2023

findepi commented May 23, 2023

ksobolew commented May 23, 2023

ksobolew commented May 23, 2023

findepi commented May 24, 2023

ksobolew commented May 24, 2023

ksobolew commented May 25, 2023

ksobolew commented May 25, 2022 •

edited

Loading

ksobolew commented May 26, 2022 •

edited

Loading

findepi commented Jun 22, 2022 •

edited

Loading