Migrate Thrift connector tests to use BaseConnectorTest. by HiLany · Pull Request #10233 · trinodb/trino

HiLany · 2021-12-08T09:41:21Z

Purpose:

Migrate Thrift connector tests to use BaseConnectorTest.

Resolves #7117

Scope:

Test of the Thrift Plugin

cla-bot · 2021-12-08T09:41:23Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

findepi · 2021-12-08T10:35:57Z

@HiLany how does this relate to @xuboqing's #7975?

HiLany · 2021-12-08T11:03:01Z

@HiLany how does this relate to @xuboqing's #7975?

I don't know what the current status of this 7975 is, it seems that it hasn't been active for a long time. So I wonder if I can continue to complete this work. If @xuboqing still working this, I will close 10233.

findepi · 2021-12-08T11:10:56Z

@xuboqing could you please comment?

@HiLany is your work based on the older PR, or independent?

HiLany · 2021-12-08T11:17:01Z

@xuboqing could you please comment?

@HiLany is your work based on the older PR, or independent?

independent.

yikf · 2021-12-08T11:35:54Z

@HiLany Could you describe the PR why we need to migrate, Sorry for i don't understand the older PR.

@findepi FYI, Shall we make a template about PULL-REQUEST-TEMPLATE, like Spark, see: pr_template, So that we can better understand PR.

findepi · 2021-12-08T14:32:45Z

@findepi FYI, Shall we make a template about PULL-REQUEST-TEMPLATE, like Spark, see: pr_template, So that we can better understand PR.

@yikf thanks for bringing this up. Let's move this topic to slack's #dev.

findinpath · 2021-12-09T06:02:35Z

...no-thrift/src/test/java/io/trino/plugin/thrift/integration/TestThriftDistributedQueries.java

@HiLany Why did you delete this test?
Was this intentional?

see #7117.

We need to merge TestThriftIntegrationSmokeTest and TestThriftDistributedQueries to form TestThriftConnectorTest.
and BaseConnectorTest is also an implementation class of AbstractTestQueries.

Should we keep TestThriftDistributedQueries?

@HiLany I see. Thanks for pointing this out. You are right. The test is not anymore needed in the context of your PR

findinpath · 2021-12-09T06:07:09Z

...n/trino-thrift/src/test/java/io/trino/plugin/thrift/integration/TestThriftConnectorTest.java

There probably needs to be provided an own implementation of io.trino.testing.BaseConnectorTest#hasBehavior method here due to the fact that this connector does not support all the operations tested in BaseConnectorTest.
Use io.trino.plugin.cassandra.TestCassandraConnectorTest#hasBehavior as a template.

Use io.trino.plugin.thrift.ThriftMetadata and io.trino.spi.connector.ConnectorMetadata to see what is possible in this connector.

There probably needs to be provided an own implementation of io.trino.testing.BaseConnectorTest#hasBehavior method here due to the fact that this connector does not support all the operations tested in BaseConnectorTest.
Use io.trino.plugin.cassandra.TestCassandraConnectorTest#hasBehavior as a template.

Thank you for the reviewed! I will do it。

Use io.trino.plugin.thrift.ThriftMetadata and io.trino.spi.connector.ConnectorMetadata to see what is possible in this connector.

Sorry，I don't understand it。Do you mean to test the functionality of ThriftMetadata?

Sorry，I don't understand it。Do you mean to test the functionality of ThriftMetadata?

By checking the code of the previously mentioned classes, you'll be able to figure out what values of the io.trino.testing.TestingConnectorBehavior you can add to the hasBehavior method .

Note that these values control whether a test needs to be executed or not

skipTestUnless(hasBehavior(SUPPORTS_CREATE_TABLE));

findinpath · 2021-12-09T06:08:54Z

trino-thrift has several test failures here: https://github.com/trinodb/trino/runs/4456769688?check_suite_focus=true

Please have a look at them to bring the PR in a better shape.

findinpath · 2021-12-09T08:03:33Z

@HiLany please also change your commit message

[fix-7717]Migrate Thrift connector tests to use BaseConnectorTest. towards

Migrate Thrift connector tests to use BaseConnectorTest

Resolves: #7717

Use this guideline as a reference: https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md#format-git-commit-messages

HiLany · 2021-12-09T08:07:02Z

Thanks for pointing this out. i will resolve it.

cla-bot · 2021-12-09T10:43:26Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

HiLany · 2021-12-09T10:49:11Z

@HiLany please also change your commit message

[fix-7717]Migrate Thrift connector tests to use BaseConnectorTest. towards
Migrate Thrift connector tests to use BaseConnectorTest

Resolves: #7717
Use this guideline as a reference: https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md#format-git-commit-messages

Hello,@findinpath .Could you review my code again？ Thank you！

findinpath · 2021-12-09T11:08:17Z

@HiLany LGTM
Please take care of removing the unnecessary/too verbose comments.

Well done. Thank you for the effort.

findinpath · 2021-12-09T10:50:53Z

...n/trino-thrift/src/test/java/io/trino/plugin/thrift/integration/TestThriftConnectorTest.java

I think that the //dml, //ddl , //query optimizations don't really improve the readability of the code.
Please consider removing them.

I think so, I will remove it in the next commit.

findinpath · 2021-12-09T10:54:11Z

plugin/trino-thrift/src/test/java/io/trino/plugin/thrift/integration/ThriftQueryRunner.java

This comment is too verbose/low level for the context of ThriftQueryRunner.
Please consider removing it.

cla-bot · 2021-12-09T11:59:11Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

hashhar

LGTM % checkstyle failure (see logs of failed CI jobs).

HiLany · 2021-12-10T01:56:50Z

LGTM % checkstyle failure (see logs of failed CI jobs).

Thanks, let me check target/checkstyle-result.xml to see what went wrong.

cla-bot · 2021-12-10T03:04:09Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

HiLany · 2021-12-10T03:10:48Z

hello @hashhar ，Could you review my code again？ Thank you！

findinpath · 2021-12-10T06:38:46Z

@HiLany were you able to sign the CLA document ?
https://github.com/trinodb/trino/blob/master/.github/CONTRIBUTING.md

HiLany · 2021-12-10T06:58:53Z

@HiLany were you able to sign the CLA document ? https://github.com/trinodb/trino/blob/master/.github/CONTRIBUTING.md

I have submitted cla to cla@trino.io, but I didn’t get a reply.

hashhar · 2021-12-10T07:23:52Z

The CLAs are processed in batches periodically. Please wait a while.
You can see if your CLA has been processed by searching for your GitHub username under https://github.com/trinodb/cla/blob/master/contributors

hashhar

LGTM % remove link to GitHub issue from commit message.

We prefer that the commit messages stand on their own and don't need someone to refer to external information to make sense of it. In this case the issue doesn't have any useful information.

If you instead want to make sure that the issue is closed once someone merges this PR, edit the Pull Request description to add Fixes #<issue> instead.

cc: @martint for the CLA. This is good to go % CLA.

cla-bot · 2021-12-13T12:24:10Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

HiLany · 2021-12-13T12:26:34Z

LGTM % remove link to GitHub issue from commit message.

We prefer that the commit messages stand on their own and don't need someone to refer to external information to make sense of it. In this case the issue doesn't have any useful information.

If you instead want to make sure that the issue is closed once someone merges this PR, edit the Pull Request description to add Fixes #<issue> instead.

cc: @martint for the CLA. This is good to go % CLA.

Thanks for pointing this out! I have modified commit information and submitted it again。

hashhar · 2021-12-15T06:08:15Z

@martint Can you help with the CLA, looks good to go otherwise.

HiLany · 2021-12-24T02:29:11Z

@martint Can you help with the CLA, looks good to go otherwise.

I have been added to https://github.com/trinodb/trino/blob/master/.github/CONTRIBUTING.md.

Thanks. @martint @hashhar .

HiLany · 2021-12-27T09:36:42Z

@hashhar Can this pr be merged into master?

hashhar · 2021-12-28T10:20:38Z

@cla-bot check

cla-bot · 2021-12-28T10:20:41Z

The cla-bot has been summoned, and re-checked this pull request!

hashhar · 2021-12-28T10:21:30Z

Thanks for working on this @HiLany. Merged.

findepi requested a review from findinpath December 8, 2021 10:34

findepi changed the title ~~[fix-7717]Migrate Thrift connector tests to use BaseConnectorTest.~~ Migrate Thrift connector tests to use BaseConnectorTest. Dec 8, 2021

findinpath suggested changes Dec 9, 2021

View reviewed changes

HiLany force-pushed the fix-7117 branch from ae83b0b to e703fb8 Compare December 9, 2021 10:43

HiLany requested a review from findinpath December 9, 2021 10:47

findinpath suggested changes Dec 9, 2021

View reviewed changes

HiLany force-pushed the fix-7117 branch from e703fb8 to 536cbfb Compare December 9, 2021 11:59

findinpath approved these changes Dec 9, 2021

View reviewed changes

hashhar approved these changes Dec 9, 2021

View reviewed changes

HiLany force-pushed the fix-7117 branch from 536cbfb to 6f45b54 Compare December 10, 2021 03:04

HiLany requested a review from hashhar December 10, 2021 03:08

HiLany requested a review from findinpath December 10, 2021 04:50

hashhar approved these changes Dec 13, 2021

View reviewed changes

Migrate Thrift connector tests to use BaseConnectorTest

84bb5c9

HiLany force-pushed the fix-7117 branch from 6f45b54 to 84bb5c9 Compare December 13, 2021 12:24

cla-bot bot added the cla-signed label Dec 28, 2021

hashhar approved these changes Dec 28, 2021

View reviewed changes

hashhar merged commit b77916f into trinodb:master Dec 28, 2021

github-actions bot added this to the 368 milestone Dec 28, 2021

mosabua mentioned this pull request Dec 30, 2021

Add Trino 368 release notes #10433

Merged

ebyhr mentioned this pull request Mar 2, 2022

Migrate Thrift connector tests to use BaseConnectorTest #7117 #7975

Closed

Conversation

HiLany commented Dec 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cla-bot bot commented Dec 8, 2021

Uh oh!

findepi commented Dec 8, 2021

Uh oh!

HiLany commented Dec 8, 2021

Uh oh!

findepi commented Dec 8, 2021

Uh oh!

HiLany commented Dec 8, 2021

Uh oh!

yikf commented Dec 8, 2021

Uh oh!

findepi commented Dec 8, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

findinpath commented Dec 9, 2021

Uh oh!

findinpath commented Dec 9, 2021

Uh oh!

HiLany commented Dec 9, 2021

Uh oh!

cla-bot bot commented Dec 9, 2021

Uh oh!

HiLany commented Dec 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

findinpath commented Dec 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cla-bot bot commented Dec 9, 2021

Uh oh!

hashhar left a comment

Choose a reason for hiding this comment

Uh oh!

HiLany commented Dec 10, 2021

Uh oh!

cla-bot bot commented Dec 10, 2021

Uh oh!

HiLany commented Dec 10, 2021

Uh oh!

findinpath commented Dec 10, 2021

Uh oh!

HiLany commented Dec 10, 2021

Uh oh!

hashhar commented Dec 10, 2021

Uh oh!

hashhar left a comment

Choose a reason for hiding this comment

Uh oh!

cla-bot bot commented Dec 13, 2021

Uh oh!

HiLany commented Dec 13, 2021

Uh oh!

hashhar commented Dec 15, 2021

Uh oh!

HiLany commented Dec 24, 2021

Uh oh!

HiLany commented Dec 27, 2021

Uh oh!

hashhar commented Dec 28, 2021

HiLany commented Dec 8, 2021 •

edited

Loading

HiLany commented Dec 9, 2021 •

edited

Loading