[flaky] Fix flaky tests caused by port binding#149
Merged
Conversation
cendhu
reviewed
Oct 30, 2025
|
|
||
| var err error | ||
| var listener net.Listener | ||
| if ctx == nil || c.Endpoint.Port == 0 { |
Contributor
There was a problem hiding this comment.
It is not intuitive that when the ctx is nil, retry till successful binding is not executed. I notice all callers to pass the ctx in the current code. Do we need this check?
Contributor
Author
There was a problem hiding this comment.
Leftover code. Good catch!
| // It stores the listener object internally to be reused on subsequent calls to Listener(). | ||
| func (c *ServerConfig) PreAllocateListener() (net.Listener, error) { | ||
| listener, err := c.Listener() | ||
| listener, err := c.Listener(context.Background()) |
Contributor
There was a problem hiding this comment.
Do we plan to pass the ctx as an argument to this PreallocateListener() in the future?
Contributor
Author
There was a problem hiding this comment.
No. PreallocateListener should only be used with port=0. If such attempt fails, there is no reason to retry.
| t.Log("One server down") | ||
| servers[2].Servers[0].Stop() | ||
| waitUntilGrpcServerIsDown(ctx, t, &servers[2].Configs[0].Endpoint) | ||
| listener1 := holdPort(ctx, t, servers[2].Configs[0]) |
Contributor
There was a problem hiding this comment.
Earlier, we only checked whether the server was down or not. Now, we create another fake listener and explicitly close it. Can you please explain the reason as I might be missing the core issue?
Contributor
Author
There was a problem hiding this comment.
I added the following comment to holdPort to clarify its use case
// holdPort attempts to bind to the specified server port and holds it until the listener is closed.
// It serves two purposes:
// 1. A successful bind indicates the port is free, meaning the server previously using it is down.
// 2. It prevents other tests from binding to the same port, ensuring this test correctly detects the server as
// unavailable.
Signed-off-by: Liran Funaro <liran.funaro@gmail.com>
Signed-off-by: Liran Funaro <liran.funaro@gmail.com>
liran-funaro
force-pushed
the
fix-flaky-test-ports
branch
from
d6a3392 to
4b0e58e
Compare
Effi-S
pushed a commit
to Effi-S/fabric-x-committer
that referenced
this pull request
Nov 3, 2025
#### Type of change - Test update #### Description - [ServerConfig] Add retry mechanism when binding to a predefined port (`c.Listener()`). - [TestBroadcastDeliver] Use the above retry mechanism to ensure the port is not used by other tests. #### Related issues - resolves hyperledger#109 - resolves hyperledger#137 --------- Signed-off-by: Liran Funaro <liran.funaro@gmail.com>
Effi-S
pushed a commit
to Effi-S/fabric-x-committer
that referenced
this pull request
Nov 3, 2025
#### Type of change - Test update #### Description - [ServerConfig] Add retry mechanism when binding to a predefined port (`c.Listener()`). - [TestBroadcastDeliver] Use the above retry mechanism to ensure the port is not used by other tests. #### Related issues - resolves hyperledger#109 - resolves hyperledger#137 --------- Signed-off-by: Liran Funaro <liran.funaro@gmail.com> Signed-off-by: Effi-S <effi.szt@gmail.com>
Effi-S
pushed a commit
to Effi-S/fabric-x-committer
that referenced
this pull request
Nov 3, 2025
#### Type of change - Test update #### Description - [ServerConfig] Add retry mechanism when binding to a predefined port (`c.Listener()`). - [TestBroadcastDeliver] Use the above retry mechanism to ensure the port is not used by other tests. #### Related issues - resolves hyperledger#109 - resolves hyperledger#137 --------- Signed-off-by: Liran Funaro <liran.funaro@gmail.com> Signed-off-by: Effi-S <effi.szt@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Type of change
Description
c.Listener()).Related issues