Skip to content

Conversation

@scotthart
Copy link
Member

@scotthart scotthart commented Apr 29, 2025

This change is Reviewable

@scotthart scotthart requested a review from a team as a code owner April 29, 2025 21:49
@product-auto-label product-auto-label bot added the api: spanner Issues related to the Spanner API. label Apr 29, 2025
@codecov
Copy link

codecov bot commented Apr 29, 2025

Codecov Report

Attention: Patch coverage is 15.78947% with 32 lines in your changes missing coverage. Please review.

Project coverage is 92.91%. Comparing base (7595793) to head (815bc2e).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
...r/integration_tests/data_types_integration_test.cc 15.78% 32 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #15118      +/-   ##
==========================================
- Coverage   92.92%   92.91%   -0.01%     
==========================================
  Files        2389     2389              
  Lines      214832   214870      +38     
==========================================
+ Hits       199631   199647      +16     
- Misses      15201    15223      +22     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@scotthart scotthart merged commit 1a7e550 into googleapis:main Apr 30, 2025
78 checks passed
EXPECT_THAT(result, IsOkAndHolds(UnorderedElementsAreArray(data)));
}

TEST_F(DataTypeIntegrationTest, SelectIntervalScalar) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Integration tests for a new Spanner type should include an addition to the DataTypes table in testing/database_integration_test.cc, and "WriteRead{,Array}" test cases in this file to exercise the new column.

Copy link
Member Author

Choose a reason for hiding this comment

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

For reasons I am not privy to, the Interval data type cannot be used as a column in a table.

EXPECT_THAT(result, IsOkAndHolds(UnorderedElementsAreArray(data)));
}

TEST_F(DataTypeIntegrationTest, SelectIntervalScalar) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Integration tests for a new Spanner type should include additions to MutationsTests.SpannerTypes in mutations_test.cc.

Copy link
Member Author

Choose a reason for hiding this comment

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

As I understand it, mutations can only be performed on table columns.

}

TEST_F(DataTypeIntegrationTest, SelectIntervalScalar) {
if (UsingEmulator()) GTEST_SKIP();
Copy link
Contributor

Choose a reason for hiding this comment

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

The Spanner integrations tests have been written to expect particular behavior from the emulator, even it is an error, rather than just skipping the test altogether. See all the other UsingEmulator() calls in this file, for example. This allows us to learn when the emulator changes behavior and react accordingly, rather than just forgetting about the whole issue forever more.

[It also helped the emulator team, who (at least previously) ran our tests to check their implementation. Rather than just seeing successes no matter what they did, expecting the old (error) behavior allowed them to verify that things had indeed changed, and also to compare their new behavior to that of the service.]

Copy link
Member Author

Choose a reason for hiding this comment

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

created #15162

std::chrono::hours(1))};
std::time_t now_seconds =
std::chrono::system_clock::to_time_t(std::chrono::system_clock::now());
std::time_t one_hour_later_seconds = now_seconds + 3600;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do chrono arithmetic in the chrono domain.

Copy link
Member Author

Choose a reason for hiding this comment

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

created #15163

Copy link
Member Author

@scotthart scotthart left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions

EXPECT_THAT(result, IsOkAndHolds(UnorderedElementsAreArray(data)));
}

TEST_F(DataTypeIntegrationTest, SelectIntervalScalar) {
Copy link
Member Author

Choose a reason for hiding this comment

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

For reasons I am not privy to, the Interval data type cannot be used as a column in a table.

EXPECT_THAT(result, IsOkAndHolds(UnorderedElementsAreArray(data)));
}

TEST_F(DataTypeIntegrationTest, SelectIntervalScalar) {
Copy link
Member Author

Choose a reason for hiding this comment

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

As I understand it, mutations can only be performed on table columns.

}

TEST_F(DataTypeIntegrationTest, SelectIntervalScalar) {
if (UsingEmulator()) GTEST_SKIP();
Copy link
Member Author

Choose a reason for hiding this comment

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

created #15162

std::chrono::hours(1))};
std::time_t now_seconds =
std::chrono::system_clock::to_time_t(std::chrono::system_clock::now());
std::time_t one_hour_later_seconds = now_seconds + 3600;
Copy link
Member Author

Choose a reason for hiding this comment

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

created #15163

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: spanner Issues related to the Spanner API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants