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.

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

Set the JVM locale fix to en-US when running tests #98

Merged
merged 1 commit into from
Dec 4, 2018
Merged

Set the JVM locale fix to en-US when running tests #98

merged 1 commit into from
Dec 4, 2018

Conversation

wenleix
Copy link
Contributor

@wenleix wenleix commented Dec 24, 2016

This avoids different exception messages under different system
languages.

@wenleix
Copy link
Contributor Author

wenleix commented Dec 24, 2016

This fixes prestodb/presto#6655, as I verified the tests will succeed under Polish language setting.

However, when I set the Presto root pom.xml to use 60-SNAPSHOT and run mvn clean install, I see the following error:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.0:compile (default-compile) on project presto-main: Compilation failure: Compilation failure:
[ERROR] /Users/wxie/Code/presto-all/presto/presto-main/src/main/java/com/facebook/presto/operator/scalar/ScalarFunctionImplementation.java:[62,17] reference to checkArgument is ambiguous
[ERROR] both method checkArgument(boolean,java.lang.String,java.lang.Object,java.lang.Object) in com.google.common.base.Preconditions and method checkArgument(boolean,java.lang.String,int,java.lang.Object) in com.google.common.base.Preconditions match
[ERROR] /Users/wxie/Code/presto-all/presto/presto-main/src/main/java/com/facebook/presto/operator/NestedLoopJoinPagesSupplier.java:[49,16] no suitable method found for transform(com.google.common.util.concurrent.SettableFuture<com.facebook.presto.operator.NestedLoopJoinPages>,com.google.common.util.concurrent.AsyncFunction<com.facebook.presto.operator.NestedLoopJoinPages,com.facebook.presto.operator.NestedLoopJoinPages>)
[ERROR] method com.google.common.util.concurrent.Futures.<I,O>transform(com.google.common.util.concurrent.ListenableFuture<I>,com.google.common.base.Function<? super I,? extends O>,java.util.concurrent.Executor) is not applicable
[ERROR] (cannot infer type-variable(s) I,O
[ERROR] (actual and formal argument lists differ in length))
[ERROR] method com.google.common.util.concurrent.Futures.<I,O>transform(com.google.common.util.concurrent.ListenableFuture<I>,com.google.common.base.Function<? super I,? extends O>) is not applicable
[ERROR] (cannot infer type-variable(s) I,O
[ERROR] (argument mismatch; com.google.common.util.concurrent.AsyncFunction<com.facebook.presto.operator.NestedLoopJoinPages,com.facebook.presto.operator.NestedLoopJoinPages> cannot be converted to com.google.common.base.Function<? super I,? extends O>))
[ERROR] /Users/wxie/Code/presto-all/presto/presto-main/src/main/java/com/facebook/presto/util/Threads.java:[62,27] cannot find symbol
[ERROR] symbol:   method get(com.google.common.util.concurrent.SettableFuture<java.lang.Boolean>,int,java.util.concurrent.TimeUnit,java.lang.Class<java.lang.Exception>)
[ERROR] location: class com.google.common.util.concurrent.Futures
...

Will check if it's an issue with the current airbase pom.xml .

@wenleix
Copy link
Contributor Author

wenleix commented Jan 4, 2017

The guava is a separate issue and will be fixed by prestodb/presto#6995

@findepi
Copy link
Contributor

findepi commented Jun 20, 2018

@wenleix any plans? Can we close prestodb/presto#6655 ?

@wenleix
Copy link
Contributor Author

wenleix commented Jun 21, 2018

@electrum : Any suggestions? Thanks!

@electrum
Copy link
Member

Should we add properties for this? That would allow running tests multiple times for different locales. Otherwise, I’m fine to merge this. Please rebase and add CHANGES.

@findepi
Copy link
Contributor

findepi commented Jun 21, 2018 via email

@ilfrin
Copy link

ilfrin commented Sep 10, 2018

@wenleix is there anything blocking from merging this and releasing a new version?

@electrum
Copy link
Member

electrum commented Sep 10, 2018 via email

@electrum
Copy link
Member

Chatted with @wenleix and he's going to update it. I also wanted to know if it could use <systemPropertyVariables> rather than -D arguments.

@ilfrin
Copy link

ilfrin commented Sep 13, 2018

@electrum this is great, lets get this in.
cc @wenleix

thank you!

@wenleix
Copy link
Contributor Author

wenleix commented Oct 14, 2018

@electrum : Rebased. Thanks!

CHANGES.md Outdated
@@ -1,3 +1,6 @@
Airbase 85
* Set the JVM locale fix to en-US when running tests
Copy link
Contributor

Choose a reason for hiding this comment

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

Use en-US locale when running Surefire tests.

@wenleix wenleix changed the title Set the JVM locale fix to en-US when running tests. Set the JVM locale fix to en-US when running tests Oct 15, 2018
CHANGES.md Outdated
@@ -1,3 +1,6 @@
Airbase 85
* Set the JVM locale fix to en-US when running tests
Copy link
Member

Choose a reason for hiding this comment

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

Nit: add newline after version header

@findepi
Copy link
Contributor

findepi commented Nov 25, 2018

I am happy to see this approved. @wenleix could you please rebase?

@wenleix
Copy link
Contributor Author

wenleix commented Nov 27, 2018

@findepi , @electrum : Done rebase. thanks!

CHANGES.md Outdated
@@ -3,6 +3,7 @@ Airbase 89
* Dependency updates:
- validation-api 2.0.1 (from 1.1.0)
- BVal 2.0.0 (from 1.1.1)
* Set the JVM locale fix to en-US when running Surefire tests.
Copy link
Member

Choose a reason for hiding this comment

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

Did you mean to leave the word “fix” there? It doesn’t sound right.

Copy link
Contributor

Choose a reason for hiding this comment

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

"pin" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"...fixed to en-US..." ?

Copy link
Member

@martint martint Nov 29, 2018

Choose a reason for hiding this comment

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

I think it should just say "Set the default JVM local for Surefire tests to en-US. It can be customized via the air.test.language and air.test.region properties".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @martint . Addressed.

This avoids different exception messages under different system
languages.
@wenleix
Copy link
Contributor Author

wenleix commented Dec 4, 2018

@electrum : Can you merge the PR ? Thanks!

@martint martint merged commit 6b67ab2 into airlift:master Dec 4, 2018
@wenleix wenleix deleted the surefire-locale branch December 5, 2018 05:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants