Replace usage of math/rand with math/rand/v2 #6817
Conversation
Signed-off-by: Ravjot Singh <[email protected]>
|
Hey, @antoninbas
Please review the changes at your earliest convenience and let me know if there are any further adjustments needed. |
| @@ -17,7 +17,7 @@ package e2e | |||
| import ( | |||
| "encoding/json" | |||
| "fmt" | |||
| "math/rand" | |||
| "math/rand/v2" // Updated import path to use math/rand/v2 | |||
There was a problem hiding this comment.
this is not a helpful comment to have in the code
| @@ -33,6 +33,13 @@ import ( | |||
| e2euttils "antrea.io/antrea/test/e2e/utils" | |||
| ) | |||
|
|
|||
| const ( | |||
There was a problem hiding this comment.
what are these new constants? I'm really confused because they seem unused and completely unrelated to the contents of this file
| @@ -458,7 +464,7 @@ func (data *MCTestData) testStretchedNetworkPolicyUpdatePolicy(t *testing.T) { | |||
| // Update the policy to select the eastRegularClient. | |||
| acnpBuilder.AddStretchedIngressRule(map[string]string{"antrea-e2e": eastRegularClientName}, nil, "", nil, crdv1beta1.RuleActionDrop) | |||
| if _, err := data.createOrUpdateACNP(westCluster, acnpBuilder.Get()); err != nil { | |||
| t.Fatalf("Error updateing ACNP %s: %v", acnpBuilder.Name, err) | |||
| t.Fatalf("Error updating ACNP %s: %v", acnpBuilder.Name, err) | |||
There was a problem hiding this comment.
while these changes are legit (fixing typos), they are unrelated to the main PR
I would recommend making a separate PR for them
| mainRand := rand.New(rand.NewPCG(1, 0)) // Initialize with fixed seeds for deterministic behavior | ||
| gwIdx := mainRand.IntN(len(nodes)) // #nosec G404: for test only |
There was a problem hiding this comment.
I don't know if deterministic behavior is really important / desired here. Maybe try with the main random generator provided by the package.
Overall I think maybe avoiding deterministic behavior is a good thing for e2e tests.
| @@ -40,7 +40,7 @@ import ( | |||
| ) | |||
|
|
|||
| // #nosec G404: random number generator not used for security purposes. | |||
| var icmpEchoID = rand.Int31n(1 << 16) | |||
| var icmpEchoID = rand.IntN(1 << 16) | |||
There was a problem hiding this comment.
After this change, you should remove the explicit casts to int in the rest of the code. Look for occurrences of int(icmpEchoID) in this file.
| @@ -28,7 +28,7 @@ import ( | |||
| ) | |||
|
|
|||
| // #nosec G404: random number generator not used for security purposes | |||
| var pktRand = rand.New(rand.NewSource(time.Now().UnixNano())) | |||
| var pktRand = rand.New(rand.NewPCG(uint64(time.Now().UnixNano()), 0)) | |||
There was a problem hiding this comment.
maybe we could use rand.New(rand.NewPCG(rand.Uint64(), rand.Uint64())) here. No need to use the time package anymore to seed?
| // Initialize random number generator with a seed | ||
| ran = rand.New(rand.NewPCG(uint64(time.Now().UnixNano()), 0)) |
There was a problem hiding this comment.
why is this needed? why can't we use the main random number generator which is randomly seeded and is also thread-safe?
| // #nosec G404: random number generator not used for security purposes | ||
| var pktRandInstance = rand.New(rand.NewPCG(1, 0)) // Fixed seeds: 1 and 0 | ||
|
|
||
| // Override pktRand in the main package with pktRandInstance during tests. |
There was a problem hiding this comment.
"main package" means something else. We are not in the main package here.
| // Initialize the PCG generator | ||
| pcg := rand.PCG{} | ||
| // Provide both 'state' and 'sequence' seeds | ||
| pcg.Seed(uint64(seed), uint64(1)) | ||
| rnd := rand.New(&pcg) |
There was a problem hiding this comment.
why are we using this verbose code here instead of rand.New(rand.NewPCG(seed, 0)) like in the rest of the PR?
| for _, ok := existingCIDRs[cidr]; ok; { | ||
| for { | ||
| if _, ok := existingCIDRs[cidr]; !ok { | ||
| break | ||
| } |
There was a problem hiding this comment.
I am not sure why this is an improvement to the existing code?
antoninbas
changed the title
Contribution towards #6648