test_get_first_free_port fails randomly

[ichorid]

On main. See this test run, for instance

Error Message

assert 50001 == 50000   +50001   -50000

Stacktrace

def test_get_first_free_port():
        # target port is free
>       assert get_first_free_port(start=50000) == 50000
E       assert 50001 == 50000
E         +50001
E         -50000

[qstokkink]

This was encountered again in #6056.

[qstokkink]

Just had a look at the test. My advice would be a major refactoring:

• This unit test is claiming actual physical OS resources (sockets), allowing outside communication into our unit test suite.
• This unit test depends on lucky allocation by the OS.
• The test lives in tribler_core/tests/, testing functionality from tribler_common/.

In detail, this test consists of three steps that are all deficient:

# target port is free
port = random.randint(50000, 60000)
assert get_first_free_port(start=port) == port

This assumes a random port on the machine is free. This is not true, any port can be occupied by the OS at any time.

# target port is locked
with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as sock:
    sock.bind(('', port))
    assert get_first_free_port(start=port) == port + 1

This opens the port, which may fail, and checks if get_first_free_port finds the following port to not be in use. This following port may also already be in use by the OS. Nitpick here would be that socket.AF_INET, socket.SOCK_STREAM may clash with a change in the defaults of get_first_free_port, which is avoidable.

# no free ports found
with pytest.raises(FreePortNotFoundError):
    get_first_free_port(start=port, limit=0)

This checks if no range leads to no port. This should be a separate unit test.

---

Suggestions

• Split this test into get_first_free_port and get_first_free_port_none_available (testing more edge cases would be even better).
• Move the tests into the right folder.
• Refactor the function in network_utils.py into a NetworkUtils-type class, so you can easily mock out the internals that claim physical resources (either through a factory pattern or by overwriting methods, like _test_port, in a subclass used for testing). This gives deterministic behavior, instead of depending on happy-flow allocation by the OS.

[drew2a]

@qstokkink yes, you are right. Thank you for the suggestions, I will implement them soon.