-
Notifications
You must be signed in to change notification settings - Fork 846
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
Add GetFrontendHostPort() method to TestServer #6781
base: main
Are you sure you want to change the base?
Conversation
76b3733
to
28024ed
Compare
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.
Thanks!
I'm wondering what's your use case? You might be better off using https://pkg.go.dev/go.temporal.io/[email protected]/testsuite#StartDevServer instead.
// GetFrontendHostPort returns the host:port for this server. | ||
// | ||
// When constructing a Temporal client from within the same process, | ||
// GetDefaultClient or NewClientWithOptions should be used instead. | ||
func (ts *TestServer) GetFrontendHostPort() string { |
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 Go convention is to not use Get
prefixes for getters. I see we've already violated that in this file so I'd accept your change as-is.
// GetFrontendHostPort returns the host:port for this server. | |
// | |
// When constructing a Temporal client from within the same process, | |
// GetDefaultClient or NewClientWithOptions should be used instead. | |
func (ts *TestServer) GetFrontendHostPort() string { | |
// FrontendHostPort returns the host:port for this server. | |
// | |
// When constructing a Temporal client from within the same process, | |
// GetDefaultClient or NewClientWithOptions should be used instead. | |
func (ts *TestServer) FrontendHostPort() string { |
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.
Yes, I stuck with the Get prefix for consistency within the package. If you prefer no Get prefix so that the interface is more similar to the DevServer, we could do that instead though.
@bergundy thanks for the review. I am using this TestServer for end to end testing, but my project setup involves passing the temporal host:port as a command line argument to the process that will instantiate the temporal client. |
@laouji, why not use the dev server utility I linked from the SDK package instead of taking dependency on the server as a library? |
@bergundy the DevServer won't not work for my use-case, but it seemed a bit clunkier to add a network call to download the binary in my CI, when I could just instantiate a TemporalLite server in my code I can play around with preinstalling the executable, if that's the recommended way to end-to-end test, though! |
What changed?
Added a
GetFrontendHostPort()
method to the TestServerWhy?
For situations where you want to know the port that the test server is running on outside of typical use of client.Client
How did you test it?
Tested locally
Potential risks
None
Documentation
there is currently no documentation for TestServer aside from inline comments
Is hotfix candidate?
no