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

Introduce BaseConnectorTest, merging AbstractTestDistributedQueries and AbstractTestIntegrationSmokeTest #6989

Merged
merged 7 commits into from
Feb 23, 2021

Conversation

findepi
Copy link
Member

@findepi findepi commented Feb 22, 2021

Remake of the idea presented in #5871.
Idea is @kokosing 's and all bugs are mine own.

For #6997

This is a preparatory step to run full `BaseConnectorTest` in this
configuration as well.
This is so that connectors do not need to test both with
caching enabled and disabled. It is a preparatory step to merge
`AbstractTestDistributedQueries`- and
`AbstractTestIntegrationSmokeTest`-based test class hierarchies into
one.
The test is added back as it was, with only the class being deprecated
and javadoc changed accordingly.
Extend `BaseConnectorTest` from `AbstractTestDistributedQueries`
(removing duplicated test methods) and run it against base-jdbc with H2.
@findepi findepi requested review from losipiuk and kokosing February 22, 2021 21:38
@cla-bot cla-bot bot added the cla-signed label Feb 22, 2021
@findepi findepi force-pushed the findepi/base-connector-test branch 2 times, most recently from 900eca5 to 7cef5ca Compare February 22, 2021 21:53
…SmokeTest

Merge `TestPostgreSqlDistributedQueries` into
`TestPostgreSqlIntegrationSmokeTest` utilizing `BaseConnectorTest`
base class.
Merge `TestRedisDistributed` into `TestRedisIntegrationSmokeTest`
utilizing `BaseConnectorTest` base class.
@kokosing
Copy link
Member

I used to have a test that was verifying that all tests from deprecated smoke tests class are added to BaseConnectorTest which I don't see here.

@findepi
Copy link
Member Author

findepi commented Feb 23, 2021

I used to have a test that was verifying that all tests from deprecated smoke tests class are added to BaseConnectorTest which I don't see here.

Good point and I am aware.
I think it will be a good addition. Do you want to port it over from #5871 as a follow up?

@kokosing
Copy link
Member

Do you want to port it over from #5871 as a follow up?

Ok.

@@ -20,9 +20,11 @@
import io.trino.sql.planner.plan.FilterNode;
import io.trino.sql.planner.plan.MarkDistinctNode;
import io.trino.sql.planner.plan.ProjectNode;
import io.trino.testing.AbstractTestIntegrationSmokeTest;
import io.trino.testing.BaseConnectorTest;
Copy link
Member

Choose a reason for hiding this comment

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

nit: commit summary to long in Merge TestPostgreSql...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants