-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 unit test for LoadOrStore #2649
Add unit test for LoadOrStore #2649
Conversation
Codecov Report
|
@@ -101,6 +101,14 @@ func TestForwardedPorts(t *testing.T) { | |||
t.Fatal("didn't load port 9000 correctly") | |||
} | |||
|
|||
if _, ok := pf.LoadOrStore(4000, struct{}{}); ok { |
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.
I know this isn't directly related to this change, but it got me thinking. do we really need the signature of this method to take a struct{}{}
at all? aren't we just using this as an atomic set to track which ports are in use? maybe we could just change the signature to LoadOrStore(value int)
and have the implementation throw the struct{}{}
in there. WDYT?
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.
So this is necessary because the forwardedPorts
type has to implement the ForwardedPorts interface so that it can use GetAvailablePort.
I think the event API uses that function as well, except with a sync.Map
, so we need LoadOrStore
as written to work for both!
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 for clarifying!
Adds unit test to LoadOrStore which had a bug in it previously
Part of #2596