-
Notifications
You must be signed in to change notification settings - Fork 18.8k
fix minor code unreachability error #43844
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
Conversation
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 contributing! I left some suggestions; could you update the PR (amending the commit)? Let me know if you need help doing so!
libnetwork/ipam/parallel_test.go
Outdated
t.Fatalf("Got duplicate IP %s", ip.String()) | ||
t.Errorf("Got duplicate IP %s", ip.String()) | ||
break |
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 contributing (sorry for the delay, I saw your PR, but wanted to have a slightly closer look).
So, there's indeed unreachable code in this function (the break
would never be reached), but I'm not sure if changing t.Fatal
to t.Error
is the correct fix here. i'm not very familiar with this test, but looking at the code;
allocate()
is used by various tests- all of them look to be using it to run these steps in parallel
Changing to t.Error()
effectively would mean;
- All parallel runs continue running
- This
for
loop also continues running - (probably) the code below the loop would still
Fatal
the test
So the "old" code (before this PR) would fail early; based on all of the above, I think that's intentional, because if one of the (parallel) runs fails, the test should fail, so there's not really a reason to continue running the other (parallel) tests as they're not really testing anything else.
I would suggest to instead keep the t.Error()
, and remove the break
;
if there, ok := tctx.ipMap[ip.String()]; ok && there {
t.Fatalf("Got duplicate IP %s", ip.String())
}
While updating, perhaps also change the code outside of the loop, because the assert.Check
is redundant, and;
assert.Check(t, is.Len(tctx.ipList, tctx.maxIP))
if len(tctx.ipList) != tctx.maxIP {
t.Fatal("mismatch number allocation")
}
Can be changed to an assert.Assert()
which would check the length and call t.FailNow()
if it fails;
assert.Assert(t, is.Len(tctx.ipList, tctx.maxIP))
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.
Thank you very much for your patient review and suggestions, the code has been updated.
Signed-off-by: Abirdcfly <fp544037857@gmail.com>
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 updating!
LGTM (assuming CI passes 😅)
Kindly ping @thaJeztah for merge, thanks. |
Signed-off-by: Abirdcfly fp544037857@gmail.com
- What I did
t.Error* will report test failures but continue executing the test.
t.Fatal* will report test failures and stop the test immediately.
So some of the code is actually unreachable
An example:
https://go.dev/play/p/5K3KZ5RzHhR
- How I did it
- How to verify it
- Description for the changelog
fix minor code unreachability error caused by wrong use of t.Fatalf
- A picture of a cute animal (not mandatory but encouraged)