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

[tests][dask] make find-open-port test more reliable #3993

Merged
merged 3 commits into from
Feb 18, 2021

Conversation

jameslamb
Copy link
Collaborator

Working on a proposal to address #3823 (review), I found that sometimes the "find an open port" test in test_dask.py failed with an error.

assert new_port == 12401
E assert 12404 == 12401

I think that test is too strict. Right now it checks "if port 12400 is unavailable, _find_open_port() returns 12401". If 12401 is busy for some unrelated reason, this test fails.

This PR proposes loosening the condition a bit, to reduce the risk of test failures like that. It isn't important that that method finds 12401 specifically. Just some port that is > the port number you passed in.

worker_ip = '127.0.0.1'
with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s:
s.bind((worker_ip, 12400))
s.bind((worker_ip, listen_port))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that test is too strict.

I think now test is too soft 😃 . What about checking some meaningful intervals? Something like

worker_ip = '127.0.0.1'
port = listen_port
with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s:
    s.bind((worker_ip, port))
    ...
assert port < new_port < port + 10

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok I can change it to port < new_port < port + 1000, since that is the range that the function covers:

Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

Thanks!

@StrikerRUS StrikerRUS changed the title [dask] make find-open-port test more reliable [tests][dask] make find-open-port test more reliable Feb 18, 2021
@StrikerRUS StrikerRUS merged commit a3f4831 into microsoft:master Feb 18, 2021
@jameslamb jameslamb deleted the misc/open-port-test branch June 1, 2021 04:05
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants