Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Counter does increment first retry #1601

Merged
merged 24 commits into from
May 22, 2024

Conversation

danieljbruce
Copy link
Contributor

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.

@danieljbruce danieljbruce requested review from a team as code owners May 21, 2024 18:53
@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label May 21, 2024
@danieljbruce danieljbruce changed the title Counter does increment first retry fix: Counter does increment first retry May 21, 2024
@leahecole
Copy link
Contributor

cc @sofisl just for visibility 🙂

Copy link
Contributor

@leahecole leahecole left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small change!

gax/test/test-application/src/index.ts Show resolved Hide resolved
gax/test/unit/streaming.ts Show resolved Hide resolved
Co-authored-by: Leah E. Cole <6719667+leahecole@users.noreply.github.com>
Copy link
Contributor

@leahecole leahecole left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants