Fix SpannerChangeStreamErrorTest.java and stop disabling tests #28751
Conversation
| // databaseClient.getDialect does not currently bubble up the correct message. | ||
| // Instead, the error returned is: "DEADLINE_EXCEEDED: Operation did not complete " | ||
| // "in the given time" | ||
| thrown.expectMessage("RESOURCE_EXHAUSTED - Statement: 'SELECT 'POSTGRESQL' AS DIALECT"); | ||
| thrown.expectMessage("DEADLINE_EXCEEDED: Operation did not complete in the given time"); |
There was a problem hiding this comment.
It would be fine to make this a flexible match, perhaps just DEADLINE_EXCEEDED. The correctness of SpannerChangeStream does not depend on the details. And same comment for all error message matching.
There was a problem hiding this comment.
Sure thing, done.
| @@ -300,9 +330,19 @@ public void testInvalidRecordReceived() { | |||
| mockInvalidChangeStreamRecordReceived(startTimestamp, endTimestamp); | |||
|
|
|||
| try { | |||
| final SpannerConfig changeStreamConfig = | |||
| SpannerConfig.create() | |||
There was a problem hiding this comment.
Positive comment - inlining the config also makes the test better, as the setup should be inline with the result. Great.
| thrown.expectMessage("Field not found"); | ||
| assertThat( | ||
| mockSpannerService.countRequestsOfType(ExecuteSqlRequest.class), Matchers.equalTo(0)); | ||
| mockSpannerService.countRequestsOfType(ExecuteSqlRequest.class), Matchers.greaterThan(0)); |
There was a problem hiding this comment.
This is a behavior change from expecting 0 to expecting great than 0. Just noting this. I don't have a strong opinion about it.
There was a problem hiding this comment.
I think the behavior change is fine, since previously this test was disabled, so I'm not exactly sure which part was broken.
|
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
|
We disabled the troublesome tests but I think you need to rebase or merge in master branch to get those changes. |
nancyxu123
force-pushed
the
FixErrorTest
branch
from
63f0e95 to
1c68481
Compare
Done! |
|
Everything succeeded but it failed to publish the JUnit test results. Actually I don't know if that means the tests succeeded, just that running the tests succeeded. |
Hi, I tried rerunning the SpannerChangeStreamErrorTest locally and it passes. Do you know what could cause the failure to publish the Junit test results? |
|
I figured it was transient, so I re-ran the failed job. If it is failing all the time it is probably a GHA bug (in our configs I would guess) |
|
I'm re-running it one more time and pinging @damccorm who is our resident GHA expert :-) |
|
Not sure about the cause of the publishing failures, but the build did succeed and ran the tests (see build scan - https://ge.apache.org/s/e7yiokl4fbixq/tests/overview). Lets see if the publishing issues are transient, if not we can dig in independently from this PR |
|
Noting that the publishing did fail in two consecutive runs on this PR. So the current run would represent 3 consecutive failures so hopefully we can then isolate the problem. I mean hopefully it was transient and all fixed and never happens again, but... :-) |
|
yea same issue |
|
Ah, this is already fixed on master - https://github.com/apache/beam/actions/workflows/beam_PreCommit_Java_GCP_IO_Direct.yml?query=event%3Aschedule - we're just seeing it on retries because retries grab the same merge commit |
|
This is safe to merge |
Thank you! |
Fix error expectations in SpannerChangeStreamErrorTest.
UNAVAILABLE / ABORTED codes are always retried, so they should have the short retry settings only. Otherwise, if you use the default timeout (i.e. 2 hours), the test will timeout.