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 network.construction_with_specified_port (uses test network port) #3714

Merged

Conversation

dsiganos
Copy link
Contributor

@dsiganos dsiganos commented Feb 7, 2022

The test case network.construction_with_specified_port uses the test
network port as the test port but that means this test fails if there
is a test network node running on that machine.

Convert the test to use nano::get_available_port() to avoid that
problem.

The test case network.construction_with_specified_port uses the test
network port as the test port but that means this test fails if there
is a test network node running on that machine.

Convert the test to use nano::get_available_port() to avoid that
problem.
@dsiganos dsiganos requested review from clemahieu and theohax February 7, 2022 12:45
@theohax
Copy link
Contributor

theohax commented Feb 7, 2022

nano::get_available_port() is about to be gotten rid of completely, so perhaps use another static fixed port number?

@dsiganos
Copy link
Contributor Author

dsiganos commented Feb 7, 2022

Why are we removing nano::get_available_port? It seems very useful.
What will it be replaced by?

@clemahieu
Copy link
Contributor

clemahieu commented Feb 7, 2022

I don't think we should totally get rid of get_available_port, though most usages should be replaced where we don't need to specify a port; this case would be an exception I think where we do want to specify a port.

@dsiganos dsiganos merged commit d17d2c4 into nanocurrency:develop Feb 7, 2022
@dsiganos dsiganos deleted the fix_construction_with_specified_port branch February 7, 2022 14:47
@theohax
Copy link
Contributor

theohax commented Feb 7, 2022

I don't think we should totally get rid of get_available_port, though most usages should be replaced where we don't need to specify a port; this case would be an exception I think where we do want to specify a port.

This might just leave room for bad usages of it IMHO, but I can see your point. Well so far it's being used everywhere nonetheless so I guess another one usage won't hurt too much.

@zhyatt zhyatt added the unit test Related to a new, changed or fixed unit test label Feb 7, 2022
@zhyatt zhyatt added this to the V24.0 milestone Feb 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
unit test Related to a new, changed or fixed unit test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants