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

fix(tests): do not specify port number #1552

Merged
merged 1 commit into from
Feb 13, 2023
Merged

fix(tests): do not specify port number #1552

merged 1 commit into from
Feb 13, 2023

Conversation

LNSD
Copy link
Contributor

@LNSD LNSD commented Feb 13, 2023

This PR should help mitigate the "LPError: Address already in use" issues reported in #1357.

From this conversation at discord:

FWIW. The issue could be solved by allowing the Waku node/switch to be configured with a /ip4/0.0.0.0/tcp/0 (then it would look for a free TCP/UDP port and bind to it). Then each test case would use its own TCP port, avoiding the "port reuse".

@LNSD LNSD self-assigned this Feb 13, 2023
Copy link
Contributor

@alrevuelta alrevuelta left a comment

Choose a reason for hiding this comment

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

lgtm! thanks!

(ingnore this if CI is ok)
found in the past some cases where the supplied port was 0 and some tests were failing because the assert was (==0) but in the multiaddress what was being stored was a "real" available port (eg 2000) so 0!=2000.

Copy link
Contributor

@rymnc rymnc left a comment

Choose a reason for hiding this comment

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

Thanks for this! LGTM if ci passes in the first go :)

@LNSD
Copy link
Contributor Author

LNSD commented Feb 13, 2023

found in the past some cases where the supplied port was 0 and some tests were failing because the assert was (==0) but in the multiaddress what was being stored was a "real" available port (eg 2000) so 0!=2000.

I explicitly decided to not modify those test cases exactly for that reason. Those test cases will have to be fixed in a different piece of work.

@LNSD
Copy link
Contributor Author

LNSD commented Feb 13, 2023

Passing ✅ 🎸

@LNSD LNSD merged commit 6eb13b9 into master Feb 13, 2023
@LNSD LNSD deleted the fix-tests-ports branch February 13, 2023 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants