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

[Backport] Uniformly set Calcite systemProperties for All Unit tests #5977

Merged
merged 1 commit into from
Jul 9, 2018

Conversation

jihoonson
Copy link
Contributor

Backport of #5451 to 0.12.2.

Looks like many travis jobs are failing because of lack of this patch.

Fixes test failures reported in -
apache#4909

Issue is that If some test skips setting up Calcite system properties
with proper encoding and loads calcite classes that use that property,
All subsequent tests in the same JVM fails.

To reproduce the issue - ExpressionsTest and CalciteQueryTest from IDE
in this order.

A better fix would be to not use System Properties in calcite, This
will work for now.

All new Calcite Unit tests that are added need to inherit
CalciteTestBase.
@jihoonson jihoonson added this to the 0.12.2 milestone Jul 7, 2018
@gianm
Copy link
Contributor

gianm commented Jul 7, 2018

You might want to backport #5959 too. This patch's tests failed because of that one. I restarted them both so let's hope for the best.

@jihoonson
Copy link
Contributor Author

Thanks @gianm. This and #5975 needs each other to pass the CI. I would like to merge them without Ci check, and then see if both patches work. Or I can make them to a single PR. What do you think?

@gianm
Copy link
Contributor

gianm commented Jul 9, 2018

This one managed to pass CI after a couple restarts so I'll merge it. Please merge master into #5975 when you can.

@gianm gianm merged commit 757d5e8 into apache:0.12.2 Jul 9, 2018
@jihoonson
Copy link
Contributor Author

Merged #5975. Thanks.

leventov pushed a commit to metamx/druid that referenced this pull request Jul 20, 2018
… (apache#5977)

Fixes test failures reported in -
apache#4909

Issue is that If some test skips setting up Calcite system properties
with proper encoding and loads calcite classes that use that property,
All subsequent tests in the same JVM fails.

To reproduce the issue - ExpressionsTest and CalciteQueryTest from IDE
in this order.

A better fix would be to not use System Properties in calcite, This
will work for now.

All new Calcite Unit tests that are added need to inherit
CalciteTestBase.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants