test: skip backup test if it takes too long

[olavloite]

Fixes #683

[codecov]

Codecov Report

Merging #684 (73de11f) into master (d093089) will increase coverage by 0.00%.
The diff coverage is n/a.

@@ Coverage Diff @@ ## master #684 +/- ## ========================================= Coverage 84.12% 84.13% - Complexity 2509 2512 +3  ========================================= Files 142 142 Lines 13911 13911 Branches 1323 1323 ========================================= + Hits 11703 11704 +1  - Misses 1662 1663 +1  + Partials 546 544 -2 

Impacted Files	Coverage Δ	Complexity Δ
...m/google/cloud/spanner/connection/SpannerPool.java	84.44% <0.00%> (-1.67%)	31.00% <0.00%> (ø%)
...ud/spanner/SessionPoolAsyncTransactionManager.java	86.99% <0.00%> (+1.62%)	13.00% <0.00%> (+2.00%)
.../cloud/spanner/spi/v1/SpannerErrorInterceptor.java	65.00% <0.00%> (+5.00%)	3.00% <0.00%> (ø%)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d093089...ffd8385. Read the comment docs.

[thiagotnunes]

I am not sure this is the best way to tackle this flakiness, because the backup not finishing might be an indication of a real problem. This test would then be non-deterministic in this case, we would not know if it was skipped due to a real failure or due to taking too long.

I am wondering if we should not remove this test entirely.

[olavloite]

I am not sure this is the best way to tackle this flakiness, because the backup not finishing might be an indication of a real problem. This test would then be non-deterministic in this case, we would not know if it was skipped due to a real failure or due to taking too long.

I am wondering if we should not remove this test entirely.

We could also consider assigning it a separate category that is skipped by default, but that is included in nightly (or other specific) builds.

[generated-files-bot]

Warning: This pull request is touching the following templated files:

• .kokoro/presubmit/integration.cfg

[thiagotnunes]

@olavloite I think that works, thanks for fixing it

[olavloite]

@thiagotnunes I'm afraid that this works locally, but not in CI at the moment. It complains about two things:

1. Apparently the integration.cfg file is a generated file. Do you know how we can create/edit the template for it?
2. Adding an environment variable to the build is also not allowed by default, so the integration test fails with the following error:

[15:32:49][ERROR] Found un-allowed variables: SKIP_BACKUP_INTEGRATION_TESTS="true" (No matching key in allowed env vars) 

Any idea how to fix that?

[thiagotnunes]

@olavloite ah ok, there is a file I can modify to allow for that env var. There is one env var that is already defined that maybe we can use: INTEGRATION_TEST_ARGS. Could you try using that one first?

[generated-files-bot]

Warning: This pull request is touching the following templated files:

• .kokoro/presubmit/integration.cfg

[olavloite]

@olavloite ah ok, there is a file I can modify to allow for that env var. There is one env var that is already defined that maybe we can use: INTEGRATION_TEST_ARGS. Could you try using that one first?

I tried with INTEGRATION_TEST_ARGS, but that seems to be reserved for other usages. Setting a value for it in the integration.cfg file for pre-submit checks causes the following error:

[ERROR] Unknown lifecycle phase "SKIP_BACKUP_INTEGRATION_TESTS". You must specify a valid lifecycle phase or a goal in the format <plugin-prefix>:<goal> or <plugin-group-id>:<plugin-artifact-id>[:<plugin-version>]:<goal>. Available lifecycle phases are: validate, initialize, generate-sources, process-sources, generate-resources, process-resources, compile, process-classes, generate-test-sources, process-test-sources, generate-test-resources, process-test-resources, test-compile, process-test-classes, test, prepare-package, package, pre-integration-test, integration-test, post-integration-test, verify, install, deploy, pre-clean, clean, post-clean, pre-site, site, post-site, site-deploy. -> [Help 1] 

[olavloite]

I'm inclined to close this PR as the backup tests have started being successful again lately. It might be that these problems were a side effect of the problems that we were seeing with RESOURCE_EXHAUSTED errors for too many administrative requests. @thiagotnunes WDYT?

[thiagotnunes]

@olavloite I agree