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.

Sign up for GitHub

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

Jump to bottom

Added BaseConnectorSmokeTest as a base class for Pinot SmokeTest #13619

Merged
merged 2 commits into from Sep 8, 2022
Merged

Added BaseConnectorSmokeTest as a base class for Pinot SmokeTest #13619

merged 2 commits into from Sep 8, 2022

Conversation

ntantri
Copy link
Member

@ntantri ntantri commented Aug 11, 2022

Description

This change is a refactoring to use BaseConnectorSmokeTest for AbstractPinotIntegration Test class

This test refactoring is for Apache Pinot

Related issues pull requests and links

Documentation

✅ No documentation is needed.

Release notes

✅ No release notes entries required.

# Section
* Fix some things. ({issue}`13059`)

@cla-bot cla-bot bot added the cla-signed label Aug 11, 2022
@ntantri ntantri marked this pull request as ready for review August 13, 2022 18:30
@ntantri ntantri force-pushed the change-pinot-testing-base-class branch from d90e183 to 834a8cc Compare August 13, 2022 18:32
@ntantri ntantri requested a review from ebyhr August 13, 2022 18:35
@ntantri ntantri force-pushed the change-pinot-testing-base-class branch from 834a8cc to abce44f Compare August 13, 2022 20:25
ebyhr
ebyhr reviewed Aug 14, 2022
View reviewed changes
Copy link
Member

@ebyhr ebyhr left a comment

Choose a reason for hiding this comment

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

Please squash commits and fix the commit title.
https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md#format-git-commit-messages

...no-pinot/src/test/java/io/trino/plugin/pinot/AbstractPinotIntegrationConnectorSmokeTest.java Outdated Show resolved Hide resolved
...no-pinot/src/test/java/io/trino/plugin/pinot/AbstractPinotIntegrationConnectorSmokeTest.java Outdated Show resolved Hide resolved
...no-pinot/src/test/java/io/trino/plugin/pinot/AbstractPinotIntegrationConnectorSmokeTest.java Outdated Show resolved Hide resolved
...no-pinot/src/test/java/io/trino/plugin/pinot/AbstractPinotIntegrationConnectorSmokeTest.java Outdated Show resolved Hide resolved
...no-pinot/src/test/java/io/trino/plugin/pinot/AbstractPinotIntegrationConnectorSmokeTest.java Outdated Show resolved Hide resolved
testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorSmokeTest.java Outdated Show resolved Hide resolved
testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorSmokeTest.java Outdated Show resolved Hide resolved
testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorSmokeTest.java Outdated Show resolved Hide resolved
testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorSmokeTest.java Outdated Show resolved Hide resolved
testing/trino-testing/src/main/java/io/trino/testing/TestingConnectorBehavior.java Outdated Show resolved Hide resolved
@ntantri ntantri force-pushed the change-pinot-testing-base-class branch 3 times, most recently from e4daf19 to dc78376 Compare August 20, 2022 18:13
@ntantri ntantri requested a review from ebyhr August 20, 2022 19:44
@ntantri ntantri force-pushed the change-pinot-testing-base-class branch 3 times, most recently from eedac40 to cda4857 Compare August 23, 2022 20:50
ebyhr
ebyhr reviewed Aug 23, 2022
View reviewed changes
Copy link
Member

@ebyhr ebyhr left a comment

Choose a reason for hiding this comment

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

The commit title is too long. Could you simplify them?
https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md#format-git-commit-messages

plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/AbstractPinotIntegrationSmokeTest.java Outdated Show resolved Hide resolved
...no-pinot/src/test/java/io/trino/plugin/pinot/AbstractPinotIntegrationConnectorSmokeTest.java Outdated Show resolved Hide resolved
...no-pinot/src/test/java/io/trino/plugin/pinot/AbstractPinotIntegrationConnectorSmokeTest.java Outdated Show resolved Hide resolved
...no-pinot/src/test/java/io/trino/plugin/pinot/AbstractPinotIntegrationConnectorSmokeTest.java Outdated Show resolved Hide resolved
...no-pinot/src/test/java/io/trino/plugin/pinot/AbstractPinotIntegrationConnectorSmokeTest.java Outdated Show resolved Hide resolved
...no-pinot/src/test/java/io/trino/plugin/pinot/AbstractPinotIntegrationConnectorSmokeTest.java Outdated
Comment on lines 1082 to 1098
" FROM " + MIXED_CASE_COLUMN_NAMES_TABLE +
" WHERE longcol = 3"))
Copy link
Member

Choose a reason for hiding this comment

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

Reminder.

@ntantri ntantri force-pushed the change-pinot-testing-base-class branch 4 times, most recently from 442686f to 48c3de8 Compare September 2, 2022 21:07
@ntantri ntantri requested a review from ebyhr September 2, 2022 22:35
@ebyhr ebyhr added the no-release-notes This pull request does not require release notes entry label Sep 5, 2022
ebyhr
ebyhr reviewed Sep 5, 2022
View reviewed changes
.../trino-pinot/src/test/java/io/trino/plugin/pinot/BasePinotIntegrationConnectorSmokeTest.java Outdated Show resolved Hide resolved
.../trino-pinot/src/test/java/io/trino/plugin/pinot/BasePinotIntegrationConnectorSmokeTest.java Outdated Show resolved Hide resolved
.../trino-pinot/src/test/java/io/trino/plugin/pinot/BasePinotIntegrationConnectorSmokeTest.java Outdated Show resolved Hide resolved
.../trino-pinot/src/test/java/io/trino/plugin/pinot/BasePinotIntegrationConnectorSmokeTest.java Outdated Show resolved Hide resolved
.../trino-pinot/src/test/java/io/trino/plugin/pinot/BasePinotIntegrationConnectorSmokeTest.java Outdated Show resolved Hide resolved
.../trino-pinot/src/test/java/io/trino/plugin/pinot/BasePinotIntegrationConnectorSmokeTest.java Outdated Show resolved Hide resolved
.../trino-pinot/src/test/java/io/trino/plugin/pinot/BasePinotIntegrationConnectorSmokeTest.java Outdated Show resolved Hide resolved
@ntantri ntantri force-pushed the change-pinot-testing-base-class branch 5 times, most recently from 19d1ebf to 3c4084d Compare September 6, 2022 17:39
@ebyhr
Copy link
Member

ebyhr commented Sep 7, 2022

Could you please confirm CI failures?

@ntantri ntantri force-pushed the change-pinot-testing-base-class branch 2 times, most recently from e0c08b0 to 7229c83 Compare September 7, 2022 07:00
@ntantri ntantri force-pushed the change-pinot-testing-base-class branch 2 times, most recently from 3e2e9dd to 8c8d79f Compare September 7, 2022 10:46
@ntantri ntantri requested a review from ebyhr September 7, 2022 13:31
ebyhr
ebyhr approved these changes Sep 7, 2022
View reviewed changes
Copy link
Member

@ebyhr ebyhr left a comment

Choose a reason for hiding this comment

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

Please change the commit title as

  1. Extract method to prepare Pinot testing tables
  2. Extend BaseConnectorSmokeTest in Pinot tests

The point is avoiding past tense and clarifying the change of "refactor". Please note that the 2nd commit isn't refactoring actually.

https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md#format-git-commit-messages

.../trino-pinot/src/test/java/io/trino/plugin/pinot/BasePinotIntegrationConnectorSmokeTest.java Outdated Show resolved Hide resolved
.../trino-pinot/src/test/java/io/trino/plugin/pinot/BasePinotIntegrationConnectorSmokeTest.java Outdated Show resolved Hide resolved
.../trino-pinot/src/test/java/io/trino/plugin/pinot/BasePinotIntegrationConnectorSmokeTest.java Outdated Show resolved Hide resolved
.../trino-pinot/src/test/java/io/trino/plugin/pinot/BasePinotIntegrationConnectorSmokeTest.java Outdated Show resolved Hide resolved
.../trino-pinot/src/test/java/io/trino/plugin/pinot/BasePinotIntegrationConnectorSmokeTest.java Outdated Show resolved Hide resolved
.../trino-pinot/src/test/java/io/trino/plugin/pinot/BasePinotIntegrationConnectorSmokeTest.java Outdated Show resolved Hide resolved
.../trino-pinot/src/test/java/io/trino/plugin/pinot/BasePinotIntegrationConnectorSmokeTest.java Outdated Show resolved Hide resolved
@ntantri ntantri force-pushed the change-pinot-testing-base-class branch from 8c8d79f to c38f940 Compare September 8, 2022 17:18
@ntantri
Extend BaseConnectorSmokeTest in Pinot tests
6bd1802 
Made the AbstractPinotIntegrationSmokeTest to extended the same to BaseConnectorSmokeTest
Renamed AbstractPinotIntegrationSmokeTest to BasePinotIntegrationConnectorSmokeTest
@ntantri ntantri force-pushed the change-pinot-testing-base-class branch from c38f940 to 6bd1802 Compare September 8, 2022 18:52
@ntantri ntantri requested a review from ebyhr September 8, 2022 20:49
ebyhr
ebyhr reviewed Sep 8, 2022
View reviewed changes
.../trino-pinot/src/test/java/io/trino/plugin/pinot/BasePinotIntegrationConnectorSmokeTest.java
@Override
public void testShowCreateTable()
{
assertQueryFails("SHOW CREATE TABLE region", "No PropertyMetadata for property: pinotColumnName");
Copy link
Member

Choose a reason for hiding this comment

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

#14071

@ebyhr ebyhr merged commit 1c51576 into trinodb:master Sep 8, 2022
@ebyhr
Copy link
Member

ebyhr commented Sep 8, 2022

Merged. Extending BaseConnectorSmokeTest exposed some bugs. Thanks for your work!

@github-actions github-actions bot added this to the 396 milestone Sep 9, 2022
@elonazoulay
Copy link
Member

This is great!

@ntantri
Copy link
Member Author

ntantri commented Sep 9, 2022

Thanks for patiently reviewing all the work @ebyhr!

@elonazoulay elonazoulay mentioned this pull request Sep 9, 2022
Closed
@colebow colebow mentioned this pull request Sep 13, 2022
Merged
@ebyhr ebyhr mentioned this pull request Sep 14, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ebyhr ebyhr ebyhr approved these changes

Assignees

@ntantri ntantri

Labels
cla-signed no-release-notes This pull request does not require release notes entry
Milestone
396
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ntantri @ebyhr @elonazoulay