-
Notifications
You must be signed in to change notification settings - Fork 87
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
feat: more information for exceed timeout error #839
Conversation
Codecov Report
@@ Coverage Diff @@
## master #839 +/- ##
==========================================
- Coverage 89.74% 89.70% -0.04%
==========================================
Files 46 46
Lines 7432 7443 +11
Branches 514 517 +3
==========================================
+ Hits 6670 6677 +7
- Misses 759 763 +4
Partials 3 3
Continue to review full report at Codecov.
|
You know I'm deeply afraid of code changes that have no test coverage :) Can you make sure this code is covered by a unit test? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's test it please.
Sure, @alexander-fenster added a unit test to verify the error message. Based on the information from #839, add function name, total time it takes, and retry settings to the message. |
test/unit/apiCallable.ts
Outdated
assert.ok(err instanceof GoogleError); | ||
assert.strictEqual( | ||
err.message, | ||
'Function takes 100 milliseconds which exceeds retry total timeout 100 milliseconds before any response was received. The retry settings is {"initialRetryDelayMillis":0,"retryDelayMultiplier":0,"maxRetryDelayMillis":0,"initialRpcTimeoutMillis":0,"rpcTimeoutMultiplier":0,"maxRpcTimeoutMillis":0,"totalTimeoutMillis":100}.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function name is missing here - is it a test setup problem, or a problem with func.name
in the code? Will it actually work with the real function call?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, this is a user facing error message, and I'm not sure we need to dump the whole JSON object here - maybe just the total timeout (the one that is compared to)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, the name of function name is empty here, actually not sure the function name is helpful because users might just want to know which API they are calling.
re: timeout setting: I will just print out the total timeout value.
Should it say "fix #741" instead of "fix #839"? :) If it's for #741 then in its current form it does not help unfortunately, because what the user specifically asked for is
We still don't print which API is being called... |
pass the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK this looks really good now!
fix #741