-
Notifications
You must be signed in to change notification settings - Fork 40
Fix to enable tests for TIMESTAMPTZ #3079
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
Conversation
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request refactors several integration tests to unconditionally include TIMESTAMPTZ columns, which is a good change to improve test coverage for this data type. The changes are mostly correct and consistently applied. However, I found a critical copy-paste error in one test file where an incorrect column name is used when setting a TIMESTAMPTZ value. This will likely lead to test failures and should be corrected.
...est/java/com/scalar/db/storage/jdbc/ConsensusCommitAdminIntegrationTestWithJdbcDatabase.java
Outdated
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly updates several integration tests to unconditionally include the TIMESTAMPTZ data type, which helps improve test coverage for this feature. The changes involve swapping the c11 and c12 columns' data types and making the TIMESTAMPTZ column (c11) a non-conditional part of the test schemas. The modifications are applied consistently across all relevant test files and their corresponding assertions. The code changes look good and achieve the stated goal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors integration tests to properly separate TIMESTAMPTZ testing from conditional timestamp support checks. The key change moves TIMESTAMPTZ column definitions and operations outside the isTimestampTypeSupported() conditional blocks, enabling these tests to run independently of TIMESTAMP support.
Key Changes:
- Moved TIMESTAMPTZ column (c11) definitions and operations outside conditional blocks
- Moved TIMESTAMP column (c12) definitions and operations inside conditional blocks
- Updated test assertions to align with the new column arrangement
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| DistributedTransactionAdminIntegrationTestBase.java | Restructured column definitions to test TIMESTAMPTZ independently of TIMESTAMP support |
| DistributedStorageAdminIntegrationTestBase.java | Restructured column definitions to test TIMESTAMPTZ independently of TIMESTAMP support |
| JdbcTransactionAdminIntegrationTest.java | Restructured column definitions to test TIMESTAMPTZ independently of TIMESTAMP support |
| SingleCrudOperationTransactionAdminIntegrationTestWithJdbcDatabase.java | Restructured column definitions to test TIMESTAMPTZ independently of TIMESTAMP support |
| JdbcAdminIntegrationTest.java | Restructured column definitions to test TIMESTAMPTZ independently of TIMESTAMP support |
| JdbcAdminCaseSensitivityIntegrationTest.java | Restructured column definitions to test TIMESTAMPTZ independently of TIMESTAMP support |
| ConsensusCommitAdminIntegrationTestWithJdbcDatabase.java | Restructured column definitions to test TIMESTAMPTZ independently of TIMESTAMP support |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
komamitsu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a minor comment not directly related to this PR, but LGTM! 👍
...ntegration-test/java/com/scalar/db/storage/jdbc/JdbcAdminCaseSensitivityIntegrationTest.java
Outdated
Show resolved
Hide resolved
Torch3333
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
feeblefakie
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you!
komamitsu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 👍
brfrn169
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you!
Description
This PR fixes some integration test cases to enable tests for TIMESTAMPTZ.
Related issues and/or PRs
N/A
Changes made
Checklist
Additional notes (optional)
N/A
Release notes
N/A