Skip to content
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.

Sign up for GitHub

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

chore(e2e): better port selection #60

Merged
merged 5 commits into from
Sep 20, 2024
Merged

chore(e2e): better port selection #60

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2491fec
better port selection
Lazar955 Sep 19, 2024
87b3ea5
shared port selection
Lazar955 Sep 20, 2024
90238d1
count 1
Lazar955 Sep 20, 2024
cc3acd8
cleanup
Lazar955 Sep 20, 2024
42214b9
mrand2
Lazar955 Sep 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ test:
go test -race ./...

test-e2e:
go test -mod=readonly --failfast -timeout=25m -v $(PACKAGES_E2E) -count=1 --parallel 12 --tags=e2e
go test -mod=readonly -failfast -timeout=15m -v $(PACKAGES_E2E) -count=1 --parallel 12 --tags=e2e

build-docker:
$(DOCKER) build --tag babylonlabs-io/vigilante -f Dockerfile \
Expand Down
10 changes: 5 additions & 5 deletions e2etest/container/container.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
"fmt"
bbn "github.com/babylonlabs-io/babylon/types"
"github.com/btcsuite/btcd/btcec/v2"
"github.com/cometbft/cometbft/libs/rand"
"net"
"regexp"
"strconv"
Expand Down Expand Up @@ -263,19 +262,20 @@ func randomAvailablePort(t *testing.T) int {
// Base port and spread range for port selection
const (
basePort = 20000
portRange = 20000
portRange = 30000
)

// Seed the random number generator to ensure randomness
rand.Seed(time.Now().UnixNano())

// Try up to 10 times to find an available port
for i := 0; i < 10; i++ {
port := randPort(basePort, portRange)
listener, err := net.Listen("tcp", fmt.Sprintf("127.0.0.1:%d", port))
if err != nil {
continue
}

// listen on a port for a bit before closing, this should be just enough time for when other tests start
// when they try to net.Listen they should get an err and try a different port
time.Sleep(200 * time.Millisecond)
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems hacky...
I would feel better by setting a specific port range for each test, but also doesn't seem too good
Any other idea to solve this? @babylonlabs-io/core-dev

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we assume there is no other processes using our test ports we could probably:

  1. have global map of used ports
  2. change semantics of this function to be more like acquirePort i.e each call generates port, checks if it is not used (stored in map), and if it is not used it stores it in this map. (of course this operations must be behind mutex)
  3. at the end of the test port is released (removed from map) . This can by accomplished by t.Cleanup(..) and of course it also must be protected by mutex

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, but tests do not share state, so having a mutex or a map here doesn't help. One way would be to have a test which would be ran first (e.g called 01Setup and without parallel flag), which would create a file and a lockfile, then all other parallel tests startup and read/write port that they are acquiring to this shared file (locking and unlocking).
I feel like this is a bit much, but if we start having collisions I would implement it.
Thoughts @KonradStaniec @RafilxTenfen

Copy link
Member Author

Choose a reason for hiding this comment

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

Also reason for needing to have a test called 01Setup (or something like that), is that because all tests start in parallel so they would also be having race condition for the initial file creation

Copy link
Member Author

Choose a reason for hiding this comment

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

Not super hard to implement, I'll try it, see if it works and is worth the extra code. Thorugh this was clever and a nice hack :D

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, but tests do not share state, so having a mutex or a map here doesn't help.

They do not share it now but we could introduce it. You could create file port_map.go in our e2etest package have there var port_map = map[int]bool and protect access to it. It seems simpler that having file with lock file.

Or do I miss something about how go shares state between tests ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, you're right, pushed a new version

if err := listener.Close(); err != nil {
continue
}
Expand Down
2 changes: 1 addition & 1 deletion e2etest/reporter_e2e_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ func (tm *TestManager) GenerateAndSubmitBlockNBlockStartingFromDepth(t *testing.
}

func TestReporter_BoostrapUnderFrequentBTCHeaders(t *testing.T) {
t.Parallel()
//t.Parallel() // todo(lazar): this test when run in parallel is very flaky, investigate why
// no need to much mature outputs, we are not going to submit transactions in this test
numMatureOutputs := uint32(150)

Expand Down
Loading