chore: Control each response for calls to the IAM Mock Server #1455
Conversation
| "36680232662-vrd7ji19qe3nelgchd0ah2csanun6bnr@developer.gserviceaccount.com"; | ||
|
|
||
| @Test | ||
| public void sign_noRetry() throws IOException { |
There was a problem hiding this comment.
Update to include success. Thanks!
| ImpersonatedCredentialsTest.MockIAMCredentialsServiceTransportFactory transportFactory = | ||
| new ImpersonatedCredentialsTest.MockIAMCredentialsServiceTransportFactory(); | ||
| transportFactory.transport.addStatusCodeAndMessage(502, "Bad Gateway"); | ||
| transportFactory.transport.addStatusCodeAndMessage(502, "Bad Gateway"); |
There was a problem hiding this comment.
Should this fail 3 times to test the boundary?
There was a problem hiding this comment.
I updated the description for the following test below sign_retryThreeTimes_exception. I believe that will test the retry boundary as it fails three times before giving up.
There was a problem hiding this comment.
The idea here was to test the other side of the boundary: does the code allow the "last retry" to be successful? This isn't tested now.
There was a problem hiding this comment.
Apologies if I'm misunderstanding your point. The retry is configured to retry three times before giving up and I believe the two sides of the boundaries you are talking about are:
- Retrying while it hasn't reached the number of attempts limit
- Stop retrying after it has reached the number of attempts limit
The first test case sign_retryTwoTimes_success will receive two 5xx errors and should test the first case above.
The second test case sign_retryThreeTimes_exception will receive a four 5xx errors and should test the second case above. It'll retry 3 times and the 4th response will be what is sent back to the user. If it's a 5xx error, it'll throw an exception back, otherwise it'll send the response.
does the code allow the "last retry" to be successful?
If I'm understanding this comment correctly, I'll add another test case where it'll have three 5xx errors before returning a successful response. This should return a success back to the user.
| transportFactory.transport, | ||
| expectedSignature, | ||
| ImmutableMap.of())); | ||
| assertTrue(exception.getMessage().contains("Failed to sign the provided bytes")); |
There was a problem hiding this comment.
Can you assert that the server error is not retried? Or is that implicit in the setup of the transport? -- if so please leave a comment about that.
Otherwise please assert a 2nd call didn't arrive, or have the transport returning a success on 2nd request.
There was a problem hiding this comment.
Sounds good. I added a method in the transport to track the number of requests being made. This tests should include an assertion assertEquals(4, transportFactory.transport.getNumRequests()); which should ensure that a specific number of retry attempts are made and that the final 5xx errors aren't being retried.
| transportFactory.transport.addStatusCodeAndMessage(502, "Bad Gateway"); | ||
| transportFactory.transport.addStatusCodeAndMessage(502, "Bad Gateway"); | ||
| transportFactory.transport.addStatusCodeAndMessage(502, "Bad Gateway"); | ||
| transportFactory.transport.addStatusCodeAndMessage(502, "Bad Gateway"); |
There was a problem hiding this comment.
maybe assign different error code each time, so this test is more explicit on reporting back the third failure?
There was a problem hiding this comment.
Sure, done! There are a few 5xx errors, so I just swapped them to be 500, 502 and 503. It just reports the last status code that came back once all the retries are exhausted. Hopefully this should be clearer.
lqiu96
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
|
This is part 2 that follows up on: #1452
Once Part 1 is merged, I can rebase this PR to show the diffs (mostly testing related changes) to allow us to test for retrying the IAM Sign Blob RPC.