Skip to content

Conversation

@arshia-rgh
Copy link

No description provided.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR attempts to fix flaky tests in the port testsuite by replacing the previous hardcoded port calculation formula (childPID + port) % 60000 with a new getFreePort function that dynamically allocates free ports using the OS's ephemeral port assignment mechanism.

Key changes:

  • Added getFreePort helper function to obtain free ports by binding to port 0 and retrieving the OS-assigned port
  • Modified testProtoWithPID to dynamically allocate parent ports instead of using a PID-based formula
  • Removed the FIXME comment about flaky tests

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Signed-off-by: arshia-rgh <arshiarezagholi1212@gmail.com>
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 142 to 145
pairs := map[int]int{
// FIXME: flaky
80: (childPID + 80) % 60000,
8080: (childPID + 8080) % 60000,
}
if proto == "tcp" {
for _, parentPort := range pairs {
var d net.Dialer
d.Timeout = 50 * time.Millisecond
_, err := d.Dial(proto, fmt.Sprintf("127.0.0.1:%d", parentPort))
if err == nil {
t.Fatalf("port %d is already used?", parentPort)
}
}
80: 0,
8080: 0,
}
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

Setting ParentPort to 0 will cause AddPort() to fail validation. The validation in pkg/port/portutil/portutil.go:135 explicitly rejects port values <= 0:

if spec.ParentPort <= 0 || spec.ParentPort > 65535 {
    return fmt.Errorf("invalid ParentPort: %q", spec.ParentPort)
}

The test will fail at line 221 in testProtoRoutine when d.AddPort() returns this validation error.

To fix the flaky test, you need a different approach that ensures the port is free before using it, such as:

  1. Finding an available port dynamically (e.g., binding to port 0 temporarily to get an OS-assigned port, then closing and using that port number)
  2. Using a port allocation helper that checks availability
  3. Using a wider range or better randomization in the previous formula to reduce collision probability

Copilot uses AI. Check for mistakes.
@arshia-rgh arshia-rgh closed this Dec 3, 2025
@arshia-rgh arshia-rgh deleted the fix/flaky branch December 3, 2025 14:59
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.

1 participant