chore(x-goog-spanner-request-id): setup expectations for end-to-end request-id checks #3932
Conversation
product-auto-label
bot
added
size: l
olavloite
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
product-auto-label
bot
added
size: m
odeke-em
force-pushed
the
x-goog-spanner-request-id-setup-e2e-expectations
branch
from
2af5841 to
14190fe
Compare
|
@olavloite @rahul2393 @sakthivelmanii @surbhigarg92 kindly help me run the bots! |
olavloite
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
rahul2393
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
…equest-id checks This change allows us to perform future end-to-end checks to assert on the behaviour of x-goog-spanner-request-id header values. Updates googleapis#3537
odeke-em
force-pushed
the
x-goog-spanner-request-id-setup-e2e-expectations
branch
from
197415f to
abb5f5d
Compare
product-auto-label
bot
added
size: l
olavloite
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
| for (int i = 0; i < gotUnaryValues.length && false; i++) { | ||
| System.out.println("\033[33misUnary: #" + i + ":: " + gotUnaryValues[i] + "\033[00m"); | ||
| } |
There was a problem hiding this comment.
I understand that this is not being printed due to the && false condition in the loop, but can we add a TODO that this should be removed in a follow-up PR.
|
|
||
| public void checkExpectedUnaryXGoogRequestIds(MethodAndRequestId... wantUnaryValues) { | ||
| MethodAndRequestId[] gotUnaryValues = this.accumulatedUnaryValues(); | ||
| sortValues(gotUnaryValues); |
There was a problem hiding this comment.
Is it correct that we are sorting these accumulated values here? Doesn't that mean that if we receive the request-ids out of the expected order on the (mock) server, this sort action hides any potential test failures due to the request-ids having been out of order?
|
|
||
| public void checkAtLeastHasExpectedUnaryXGoogRequestIds(MethodAndRequestId... wantUnaryValues) { | ||
| MethodAndRequestId[] gotUnaryValues = this.accumulatedUnaryValues(); | ||
| sortValues(gotUnaryValues); |
There was a problem hiding this comment.
Same here regarding sorting the accumulated values as above.
| @@ -223,7 +223,7 @@ DatabaseId getDatabaseId() { | |||
| @Override | |||
| public XGoogSpannerRequestId nextRequestId(long channelId, int attempt) { | |||
| return XGoogSpannerRequestId.of( | |||
| this.nthId, this.nthRequest.incrementAndGet(), channelId, attempt); | |||
| this.nthId, channelId, this.nthRequest.incrementAndGet(), attempt); | |||
There was a problem hiding this comment.
The fact that we can make this change without any tests failing (or needing to be changed) seems to indicate that we have a test gap. Will that be covered by the tests that are being added in this PR, but that are currently not verifying anything, as all verifications are in if (false) blocks? If not, can we add tests that cover this in a follow-up PR?
There was a problem hiding this comment.
@olavloite I had made a much larger PR #3915 in which tests were all updated but deemed too large to get in at once hence I carved this one out. Indeed @tharoldD is sending a follow-up PR to remove all the false conditions and then more tests too.
|
Thank you for the code reviews and merge @rahul2393 and @olavloite! @tharoldD, we are now free to send the respective PR updates and tests. |
This change allows us to perform future end-to-end checks to assert on the behaviour of x-goog-spanner-request-id header values.
Updates #3537