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

Parallel Tests: #66 #149

Merged
merged 2 commits into from
Dec 9, 2024
Merged

Parallel Tests: #66 #149

merged 2 commits into from
Dec 9, 2024

Conversation

MaxMaeder
Copy link
Contributor

Rewrote Accept to send assigned port to portChan, a member of MockServer, also modified tests in stream_test.go to make use of this so that they can be run in parallel.

@MaxMaeder MaxMaeder requested a review from sergiu128 as a code owner November 29, 2024 19:34
@sergiu128
Copy link
Collaborator

sergiu128 commented Dec 3, 2024

Hey, thank you! Sorry for the late reply, had very busy days at work.

I think this looks good, but maybe we can make it shorter? Some suggestions:

  • I think we can get rid of wsURI in most places:
ws.AsyncHandshake(fmt.Sprintf("ws://localhost:%d", <-portChan), ...)
  • I think the mock server can just own the port channel
srv := NewMockServer()
go func() { srv.Accept(...) }
ws.AsyncHandshake(fmt.Sprintf("ws:/localhost:%d", <-srv.portChan), ...
  • With the above changes, the server then owns the port assignment. Can get rid of GetFreePort and just have the server listener bind to "localhost:0" if addr == "". Something similar to this:
const MockServerRandomAddr = "" // or some better name
func (s *MockServer) Accept(addr string) (err error) {
         if addr == MockServerRandomAddr {
	    s.ln, err = net.Listen("tcp", "localhost:0")
	 } else {
	     s.ln, err = net.Listen("tcp", addr)
	 }
	 if err != nil { return err }
	 s.portChan <- ln.Addr().(*net.TCPAddr).Port

This will make for less code in the tests and less setup in each instance. After you do this part, I'll take the rest of the tests.

@sergiu128
Copy link
Collaborator

If you're still interested, here are some more exciting problems to solve in no particular order:

Ping me if you're up for any of these!

@MaxMaeder
Copy link
Contributor Author

Thanks for the feedback, the way you're suggesting makes a lot more sense!

I refactored the MockServer and the tests, let me know what you think.

Copy link
Collaborator

@sergiu128 sergiu128 left a comment

Choose a reason for hiding this comment

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

Awesome work, thank you!

@sergiu128 sergiu128 merged commit 3ebd145 into talostrading:master Dec 9, 2024
2 checks passed
@sergiu128 sergiu128 mentioned this pull request Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants