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

Making TestCassandraConnectorTest more maintainable #11040

Merged
merged 1 commit into from
Feb 24, 2022
Merged

Making TestCassandraConnectorTest more maintainable #11040

merged 1 commit into from
Feb 24, 2022

Conversation

tangjiangling
Copy link
Member

@tangjiangling tangjiangling commented Feb 15, 2022 •
edited by ebyhr
Loading

Description

Fix some issues in TestCassandraConnectorTest that are not easy to maintain:

  • Unrelated types in a table
    • It reduces maintainability when we want to add test cases to a type
  • The definition and data are managed in the different class
    • We need to look for the different class when we want to check the expected values

General information

Is this change a fix, improvement, new feature, refactoring, or other?

Refactoring.

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

N/A.

How would you describe this change to a non-technical end user or system administrator?

N/A.

Related issues, pull requests, and links

Fixes #10860 (remaining part of the issue)

Documentation

(x) No documentation is needed.

Release notes

(x) No release notes entries required.

@cla-bot cla-bot bot added the cla-signed label Feb 15, 2022
@tangjiangling tangjiangling requested a review from ebyhr February 15, 2022 05:45
hashhar
hashhar reviewed Feb 15, 2022
View reviewed changes
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

Thanks.

plugin/trino-cassandra/src/test/java/io/trino/plugin/cassandra/TestCassandraTable.java Show resolved Hide resolved
@tangjiangling tangjiangling force-pushed the make-TestCassandraConnectorTest-more-maintainable branch from cdc5680 to 227aa86 Compare February 15, 2022 10:11
hashhar
hashhar reviewed Feb 16, 2022
View reviewed changes
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

Looks nice. Thanks for the improvement. Some comments and questions.

plugin/trino-cassandra/src/test/java/io/trino/plugin/cassandra/TestCassandraTable.java Outdated Show resolved Hide resolved
plugin/trino-cassandra/src/test/java/io/trino/plugin/cassandra/TestCassandraTable.java Outdated Show resolved Hide resolved
plugin/trino-cassandra/src/test/java/io/trino/plugin/cassandra/TestCassandraTable.java Show resolved Hide resolved
@tangjiangling tangjiangling force-pushed the make-TestCassandraConnectorTest-more-maintainable branch from 227aa86 to b99189e Compare February 16, 2022 11:43
@tangjiangling
Copy link
Member Author

Updated.

hashhar
hashhar approved these changes Feb 16, 2022
View reviewed changes
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

LGTM.

@ebyhr Do you want to take a look as well?

@tangjiangling
Copy link
Member Author

CI (:trino-iceberg) #10932

ebyhr
ebyhr reviewed Feb 17, 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 remove the bullet point of "Unrelated types in a table" from the commit body.

plugin/trino-cassandra/src/test/java/io/trino/plugin/cassandra/TestCassandraConnectorTest.java Outdated Show resolved Hide resolved
plugin/trino-cassandra/src/test/java/io/trino/plugin/cassandra/TestCassandraTable.java Outdated Show resolved Hide resolved
plugin/trino-cassandra/src/test/java/io/trino/plugin/cassandra/TestCassandraTable.java Outdated Show resolved Hide resolved
plugin/trino-cassandra/src/test/java/io/trino/plugin/cassandra/TestCassandraTable.java Outdated Show resolved Hide resolved
plugin/trino-cassandra/src/test/java/io/trino/plugin/cassandra/TestCassandraTable.java Outdated Show resolved Hide resolved
plugin/trino-cassandra/src/test/java/io/trino/plugin/cassandra/TestCassandraTable.java Outdated Show resolved Hide resolved
plugin/trino-cassandra/src/test/java/io/trino/plugin/cassandra/TestCassandraTable.java Outdated Show resolved Hide resolved
plugin/trino-cassandra/src/test/java/io/trino/plugin/cassandra/TestCassandraConnectorTest.java Outdated Show resolved Hide resolved
plugin/trino-cassandra/src/test/java/io/trino/plugin/cassandra/TestCassandraConnectorTest.java Outdated Show resolved Hide resolved
plugin/trino-cassandra/src/test/java/io/trino/plugin/cassandra/TestCassandraConnectorTest.java Show resolved Hide resolved
@tangjiangling
Copy link
Member Author

tangjiangling commented Feb 17, 2022 •
edited
Loading

I also found some unused code in TestTable that was introduced by #6410.

Edited: Ignore.

@tangjiangling tangjiangling force-pushed the make-TestCassandraConnectorTest-more-maintainable branch from b99189e to 48f0d1e Compare February 17, 2022 04:05
@tangjiangling tangjiangling force-pushed the make-TestCassandraConnectorTest-more-maintainable branch from 48f0d1e to 5351934 Compare February 19, 2022 09:38
@tangjiangling tangjiangling force-pushed the make-TestCassandraConnectorTest-more-maintainable branch from 5351934 to 5d14f5c Compare February 22, 2022 06:47
@tangjiangling
Copy link
Member Author

Updated.

@tangjiangling tangjiangling force-pushed the make-TestCassandraConnectorTest-more-maintainable branch from 5d14f5c to cbec2d6 Compare February 22, 2022 07:57
This was referenced Feb 24, 2022
Closed
Closed
@ebyhr ebyhr merged commit 7e99818 into trinodb:master Feb 24, 2022
@ebyhr
Copy link
Member

ebyhr commented Feb 24, 2022

Merged, thanks!

@tangjiangling tangjiangling deleted the make-TestCassandraConnectorTest-more-maintainable branch February 24, 2022 07:13
@github-actions github-actions bot added this to the 372 milestone Feb 24, 2022
@mosabua mosabua mentioned this pull request Feb 25, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hashhar hashhar hashhar approved these changes

@ebyhr ebyhr ebyhr approved these changes

Assignees
No one assigned
Labels
cla-signed
Milestone
372
Development

Successfully merging this pull request may close these issues.

Add SqlDataTypeTest to Cassandra connector
3 participants
@tangjiangling @ebyhr @hashhar