Skip to content

Conversation

@findepi
Copy link
Member

@findepi findepi commented Jan 25, 2023

Extracted from #15842

The method was not a necessary for subclasses to implement, because the
base class can know the schema name directly. This also makes the code
more consistent, as there already were some code pieces in the class
which get schema name from session setup directly.
@findepi findepi added test no-release-notes This pull request does not require release notes entry labels Jan 25, 2023
@cla-bot cla-bot bot added the cla-signed label Jan 25, 2023
Copy link
Member

Choose a reason for hiding this comment

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

Could you rename the class in trino-iceberg/pom.xml too?

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch, thanks!

Concrete test classes start with "Test" and typically do not end with
"Test". `BaseConnectorTest` (BCT) and `BaseConnectorSmokeTest` (BCST)
subclasses are noticeable exceptions.
- Avoid reassigning variable in test. Mutable variable asserted on is more
  error-prone than asserting directly without assigning to a variable. The
  latter approach is equally (if not more) readable, if formatted
  properly.
- Update test queries to follow SQL code style.
@findepi findepi force-pushed the findepi/improve-baseicebergmaterializedviewtest-code-style-2206dc branch from 61b5ddf to 647e60c Compare January 26, 2023 08:55
@findepi
Copy link
Member Author

findepi commented Jan 26, 2023

/test-with-secrets sha=647e60cc1181d3b78cf6a585f7e3be1bf54cf6cd

@findepi findepi merged commit 3091cf1 into trinodb:master Jan 26, 2023
@findepi findepi deleted the findepi/improve-baseicebergmaterializedviewtest-code-style-2206dc branch January 26, 2023 13:04
@github-actions github-actions bot added this to the 407 milestone Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed no-release-notes This pull request does not require release notes entry test

Development

Successfully merging this pull request may close these issues.

4 participants