testsuite.StartDevServer: Respect timeout during dial #1498
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
|
|
abhinav
force-pushed
the
testserver-dial-timeout
branch
from
6afc684 to
57965b9
Compare
|
LGTM, will let @Quinn-With-Two-Ns review |
Quinn-With-Two-Ns
approved these changes
May 31, 2024
|
CI failing looks like an issue with the server |
testsuite.StartDevServer accepts a context.Context meant to indicate how long the caller is willing to wait for the result. This context is used when downloading the Temporal CLI, but is not considered when tryign to dial to the server in `waitServerReady`. Without this, if an attempt to start the server failed (e.g. because of an invalid `--log-format` argument), `waitServerReady` will wait for `600 * 100ms = 1 minute` before returning the failure to the caller. This changes `waitServerReady` to respect the timeout configured in the context--checking in on it after each attempt. This also changes the actual `Dial` attempt to use `DialContext` with the same context so that if the context expires, the dial operation also returns early if possible.
Quinn-With-Two-Ns
force-pushed
the
testserver-dial-timeout
branch
from
57965b9 to
a06cb14
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What was changed
Background:
testsuite.StartDevServer accepts a context.Context
meant to indicate how long the caller is willing to wait for the result.
This context is used when downloading the Temporal CLI,
but is not considered when trying to dial to the server in
waitServerReady.Change:
This changes
waitServerReadyto respect the timeout configured in thecontext--checking in on it after each attempt, and returning early
if the context has expired.
Similarly, the
Dialattempt now usesDialContextso that if the contextexpires, the dial operation also returns early if it can.
Why?
Without this, if an attempt to start the server failed
(e.g. because of an invalid
--log-formatargument),waitServerReadywill block the test for 1 minutebefore giving up and returning an error.
This is a pretty long time to wait for a test to fail,
when the caller is expecting, say, 5 seconds at most.
Checklist
How was this tested:
A unit test was added for the new functionality,
and run with
go test -run WaitServerReady -count 1000to verify that it's not flaky.
Any docs updates needed?
I don't think so.