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.

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

Adjust default port numbers for tests. #6849

Merged
merged 2 commits into from
Jan 15, 2025
Merged

Adjust default port numbers for tests. #6849

merged 2 commits into from
Jan 15, 2025

Conversation

cheatfate
Copy link
Contributor

No description provided.

@tersec tersec enabled auto-merge (squash) January 15, 2025 12:15
Comment on lines +213 to +216
UNIT_TEST_BASE_PORT := 39960
REST_TEST_BASE_PORT := 40990
MINIMAL_TESTNET_BASE_PORT := 35001
MAINNET_TESTNET_BASE_PORT := 36501
Copy link
Contributor

Choose a reason for hiding this comment

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

ephemeral ports could be in use randomly, https://en.wikipedia.org/wiki/Ephemeral_port#Range - unfortunately it's quite platform specific which OS uses what ports.

Copy link
Member

@richard-ramos richard-ramos Jan 15, 2025

Choose a reason for hiding this comment

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

perhaps something like this would work? (after replacing nim by $(ENV_SCRIPT) $(NIMC). That way the port numbers wouldn't have to be hardcoded, and the port number would be obtained by nim automatically regardless of the OS being used.

UNIT_TEST_BASE_PORT := $(shell nim r -d:release --eval:"import net; let s=newSocket(); s.bindAddr(Port(0)); echo s.getLocalAddr()[1]; s.close()")
REST_TEST_BASE_PORT := $(shell nim r -d:release --eval:"import net; let s=newSocket(); s.bindAddr(Port(0)); echo s.getLocalAddr()[1]; s.close()")
MINIMAL_TESTNET_BASE_PORT := $(shell nim r -d:release --eval:"import net; let s=newSocket(); s.bindAddr(Port(0)); echo s.getLocalAddr()[1]; s.close()")
MAINNET_TESTNET_BASE_PORT := $(shell nim r -d:release --eval:"import net; let s=newSocket(); s.bindAddr(Port(0)); echo s.getLocalAddr()[1]; s.close()")

main:
	@echo "Port1: $(UNIT_TEST_BASE_PORT)"
	@echo "Port2: $(REST_TEST_BASE_PORT)"
	@echo "Port3: $(MINIMAL_TESTNET_BASE_PORT)"
	@echo "Port4: $(MAINNET_TESTNET_BASE_PORT)"

Copy link
Contributor Author

@cheatfate cheatfate Jan 15, 2025

Choose a reason for hiding this comment

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

lol, but it will not work because there is a big gap in time between this variables being used and your incredible script being executed.
One more thing, your script does not work when executed many times, it will always provide you with same port number.

Copy link
Contributor

Choose a reason for hiding this comment

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

And also, there are entire ranges of port being used that add on top of the base port xD With the script you could even get the same port multiple times.

Copy link
Member

Choose a reason for hiding this comment

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

Oh well. RIP my script idea
image

@tersec tersec disabled auto-merge January 15, 2025 12:44
Copy link

Unit Test Results

       12 files  ±0    1 826 suites  ±0   1h 0m 39s ⏱️ + 6m 3s
  5 358 tests ±0    5 011 ✔️ ±0  347 💤 ±0  0 ±0 
29 589 runs  ±0  29 145 ✔️ ±0  444 💤 ±0  0 ±0 

Results for commit ed072f9. ± Comparison against base commit ef5cba9.

@tersec tersec merged commit 437bc79 into unstable Jan 15, 2025
12 checks passed
@tersec tersec deleted the change-bind-ports branch January 15, 2025 15:20
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.

4 participants