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

Migrate Kafka tests to use BaseConnectorTest #7114

Closed
losipiuk opened this issue Mar 2, 2021 · 6 comments · Fixed by #11114
Closed

Migrate Kafka tests to use BaseConnectorTest #7114

losipiuk opened this issue Mar 2, 2021 · 6 comments · Fixed by #11114
Assignees

Comments

@losipiuk
Copy link
Member

losipiuk commented Mar 2, 2021

#6989 introduced new BaseConnectorTest test class which replaces AbstractTestDistributedQueries and AbstractTestIntegrationSmokeTest.

Scope of this ticket is to restructure Kafka test code to follow new pattern.
TestKafkaIntegrationSmokeTest should be renamed to TestKafkaConnectorTest and extend BaseConnectorTest instead of AbstractTestIntegrationSmokeTest

A PR which restructures MySQL tests is a good reference: #7011

@losipiuk losipiuk added good first issue Good for newcomers jdbc Relates to Trino JDBC driver labels Mar 2, 2021
@Alicewillbe
Copy link

Alicewillbe commented Mar 5, 2021

I'm tracking this issue for a while and it is still assigned to no one. May I take this issue?

Fixes regarding this issue made on my own fork repo's branch (not master) can be found here: link

@losipiuk
Copy link
Member Author

losipiuk commented Mar 5, 2021

Sure, thanks !

@findepi
Copy link
Member

findepi commented Mar 5, 2021

@Alicewillbe sure, go for it, thanks!

See #7182 as it changes something around Kafka tests, but hopefully shouldn't conflict

@Alicewillbe
Copy link

@findepi @losipiuk Thanks for supporting!
I agree that no conflict should happen between #7182 and this one, from the perspective of simple refactoring.
Just in case, should I wait until #7182 merged?

@losipiuk
Copy link
Member Author

losipiuk commented Mar 5, 2021

@findepi @losipiuk Thanks for supporting!
I agree that no conflict should happen between #7182 and this one, from the perspective of simple refactoring.
Just in case, should I wait until #7182 merged?

No need IMO. Feel free to attack it already.

@losipiuk
Copy link
Member Author

Dropped good-first-issue as the problem here is not that easy to solve due to the fact that Kafka is currently not covered with tests based on AbstractDistributedTestQueries. See #7185 for more info.

@ebyhr ebyhr removed the jdbc Relates to Trino JDBC driver label Feb 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants