-
Notifications
You must be signed in to change notification settings - Fork 619
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
make TestGetHostPortRange unit test deterministic #3587
Conversation
TestGetHostPortRange
unit test deterministic3b58e7d
to
920146c
Compare
920146c
to
064dcf1
Compare
064dcf1
to
aa97e3d
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.
Looks good to me. Can you give an example around why sometimes the specific port is not available?
thanks for reviewing this PR! I am not sure how GH workflows run unit tests and what setup is shared across multiple runs. This PR is just based on observations from previous GH workflow unit test failures, like the one I linked in the description. I suspect if multiple PRs run tests together, they may be running on the same host at the same time. This is just my guess. This PR doesn't change any existing functionality anyways, just mocks the runtime check which it should. |
Sounds good |
26e539d
to
2ddaa56
Compare
2ddaa56
to
ba3d07f
Compare
Summary
make TestGetHostPortRange unit test deterministic
I noticed that the
TestGetHostPortRange
unit test relies on the underlying host's port availability. example failureThe
getHostPortRange
method does anet.Listen
/net.ListenPacket
calls on the host to check if a port is available. In the unit test, we check if the assigned ports matches an expected set of ports. This leads to it occasionally failing on PRs GH workflow, depending on whether a port is available at that point in time.Implementation details
This PR refactors the
getHostPortRange
method and mocks the part where we do the port check on the host, in the unit test.Testing
Existing unit tests succeed.
New tests cover the changes: no
Description for the changelog
make TestGetHostPortRange unit test deterministic
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.