testing: improvements for Backup related tests #1438
Conversation
googlebot
added
the
cla: yes
|
Once all the test pass, maybe I'll trigger 5 coverage builds almost simultaneously. |
There was a problem hiding this comment.
I think we should separate those feature improvements to other PRs, what do you think?
Reviewed 2 of 5 files at r1, 14 of 14 files at r2.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @tmatsuo)
google/cloud/spanner/database_admin_connection.h, line 337 at r2 (raw file):
* running checked for completion */ std::shared_ptr<DatabaseAdminConnection> MakeDatabaseAdminConnection(
This also fixes #1211 which ideally should go in a separate PR.
google/cloud/spanner/instance_admin_connection.h, line 227 at r2 (raw file):
* running checked for completion */ std::shared_ptr<InstanceAdminConnection> MakeInstanceAdminConnection(
This also fixes #1212, which ideally should happen in a separate PR, please split.
google/cloud/spanner/integration_tests/backup_integration_test.cc, line 83 at r2 (raw file):
auto s = spanner_testing::CleanupStaleInstances(project_id_, instance_name_regex_); EXPECT_STATUS_OK(s) << s.message();
This will actually fail the test when the cleanup fails, maybe just log? I am not sure.
google/cloud/spanner/integration_tests/instance_admin_integration_test.cc, line 80 at r2 (raw file):
return; } auto s = spanner_testing::CleanupStaleInstances(project_id_,
Thanks for the refactoring btw, much cleaner.
google/cloud/spanner/testing/cleanup_stale_instances.cc, line 34 at r2 (raw file):
spanner::MakeInstanceAdminConnection()); std::vector<std::string> instance_ids = [&instance_admin_client, &project_id,
At this point I would just write [&]().
google/cloud/spanner/testing/compiler_supports_regexp.h, line 24 at r2 (raw file):
namespace spanner_testing { inline namespace SPANNER_CLIENT_NS { inline bool CompilerSupportsRegexp() {
Thank you for refactoring this.
Codecov Report
@@ Coverage Diff @@
## master #1438 +/- ##
==========================================
+ Coverage 93.63% 95.32% +1.69%
==========================================
Files 186 189 +3
Lines 15719 15696 -23
==========================================
+ Hits 14718 14962 +244
+ Misses 1001 734 -267
Continue to review full report at Codecov.
|
|
The coverage build took 35:43, better than before :) @coryan Thanks for the review, I'll split the PR, but before doing so, let me run 5 simultaneous coverage builds as an endurance test. |
|
I started 5 coverage-presubmit builds |
|
All the 5 coverage-presubmit builds passed with the latency of 33:21, 35:14, 35:49, 34:52, and 36:11. Much better :) |
The integration test for backups can take very long. We prefer to fail the test.
also use shorter timeout for the samples
also delete any backups within the instance
tmatsuo
force-pushed
the
enable-backup-test
branch
from
e1b1096
to
9798640
Compare
There was a problem hiding this comment.
Rebased to master, with some cleanups. PTAL
Reviewable status: 0 of 12 files reviewed, 6 unresolved discussions (waiting on @coryan)
google/cloud/spanner/database_admin_connection.h, line 337 at r2 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
This also fixes #1211 which ideally should go in a separate PR.
Done.
google/cloud/spanner/instance_admin_connection.h, line 227 at r2 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
This also fixes #1212, which ideally should happen in a separate PR, please split.
Done.
google/cloud/spanner/integration_tests/backup_integration_test.cc, line 83 at r2 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
This will actually fail the test when the cleanup fails, maybe just log? I am not sure.
If we only log, we won't notice too many leaks (likely because the cleanup code not working) until a test failure due to too many instances. I'd catch it sooner than later. Thoughts?
google/cloud/spanner/integration_tests/instance_admin_integration_test.cc, line 80 at r2 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
Thanks for the refactoring btw, much cleaner.
Ack
google/cloud/spanner/testing/cleanup_stale_instances.cc, line 34 at r2 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
At this point I would just write
[&]().
Done.
google/cloud/spanner/testing/compiler_supports_regexp.h, line 24 at r2 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
Thank you for refactoring this.
Ack
|
FYI, it successfully deleted an instance |
There was a problem hiding this comment.
Reviewed 12 of 12 files at r4.
Reviewable status:complete! all files reviewed, all discussions resolved
google/cloud/spanner/integration_tests/backup_integration_test.cc, line 83 at r2 (raw file):
Previously, tmatsuo (Takashi Matsuo) wrote…
If we only log, we won't notice too many leaks (likely because the cleanup code not working) until a test failure due to too many instances. I'd catch it sooner than later. Thoughts?
SGTM.
coryan
changed the title
|
Pro tip: to fix more than one bug you cannot just say |
…ud-cpp-spanner#1438) * test: shorten polling policies in backup integration tests The integration test for backups can take very long. We prefer to fail the test. * factor out some logic from backup_integration_test also use shorter timeout for the samples * remove the longer timeout for some tests * factor out the code for cleaning up stale instances also delete any backups within the instance * Enable backup related tests for the coverage build * use spanner::ExponentialBackoffPolicy Co-authored-by: Carlos O'Ryan <coryan@google.com>
Fixes #1432 and fixes #1429
We may still want to disable backup related tests for the coverage build. If so, we can run the backup related tests in nightly build.
This change is