googleapis / gax-nodejs

Google API Extensions for Node.js
Apache License 2.0
117 stars 88 forks source link

fix: Counter does increment first retry #1601

Closed danieljbruce closed 4 months ago

danieljbruce commented 4 months ago

Source code changes

One line of code is added to increment the retry counter for the first error that is encountered as that is not currently done.

Test application code changes

A test called testShouldFailOnThirdError is added that sends three retriable errors from the server and then sends an OK status code with some data. On the third error, since maxRetries is set to 2, an error should bubble up to the test application that says the maximum number of retries have been exceeded. The test ensures that this error does in fact bubble up and get received by the user.

The source code change breaks test testServerStreamingRetrieswithRetryRequestOptions, but this test should not be passing as is. In this test, maxRetries in streaming.ts gets set to 1, but the server in this test emits 2 errors so an error should bubble up to the test application. The test however, expects no error to bubble up so this test is changed so that maxRetries does not get set to 1.

Unit test change

One of the existing unit tests expects the retry function to be called 3 times. The logic in the comment mentions that even though max retries is 2 the retry function will always be called maxRetries+1 the final call is where the failure happens. But note that on the final error, retry never actually gets called so really we should be only expecting retry to be called twice in the test.

This unit test is also wrapped with a try/catch block so that when the assert statements fail, mocha reports a failing test with the error instead of timing out.

leahecole commented 4 months ago

cc @sofisl just for visibility 🙂