Use Popen instead of check_call in collect_tests

[TheRealFalcon]

check_call is blocking, and if the list of collected tests exceeds the system's pipe-max-size, pushing to the pipe will also block until data is pulled off of it. This means that the check_call call will never return and hang forever.

Using Popen instead allows the code after the subprocess call to execute immediately, and it will block reading until the pipe gets closed from the other end.

[maciej-gol]

it will block reading until the pipe gets closed from the other end.

It won't block reading, though, no? The calling process will be able to start reading ASAP, until the pipe gets closed in the called process.

Also, shouldn't we wait for the process? To handle process failures, etc. Otherwise, if the process fails without closing the pipe, we hang again.

[TheRealFalcon]

It won't block reading, though, no? The calling process will be able to start reading ASAP, until the pipe gets closed in the called process.

In this case, the calling process does the writing, and reading can't begin until check_call completes.

As a reproducer on Linux, first I take a look at my max pipe size:

$ cat /proc/sys/fs/pipe-max-size
1048576

and then create a number of tests such that collection will exceed that size. AFAIK, 1M is the default pipe-max-size on Linux these days, but if yours is larger, adjust accordingly:

import string
import random

def generate_test_name():
    # Generate a random string of fixed length
    return ''.join(random.choices(string.ascii_lowercase + string.ascii_uppercase + string.digits, k=200))

with open('test_file.py', 'w') as f:
    for i in range(1000):
        test_name = generate_test_name()
        f.write(f"def test_{test_name}():\n    pass\n\n")

Run the script. E.g.:

$ python script.py

I then edit the resulting file to include your project example at the top. E.g.:

$ head -10 test_file.py
a = 1

def test_faulty() -> None:
    global a
    a = 2

def test_failing() -> None:
    assert a == 1

def test_Y8P1IonXkdAt02H9FjXyZYxfWMBZH7mKBnPSrBxNKsRzW3F6lY40mRp4hXNoFiXfOI6RJPZLn3waRuLBQuijSclZ8vCitGNOB6A6NvahyRF2JB3m0egLCwnNsyIfMggYKz6s69el0yKX0W0Cjw1z11aHancY09HvHmhVwrz94rn1H39C8wiJMLZ2DgVLUHP14MCbsseT():

Then run,

pytest-bisect-tests --failing-test "test_file.py::test_failing"

It should hang forever.

Also, shouldn't we wait for the process? To handle process failures, etc.

Yeah, good point. I added an update to do that. Let me know if you want me to change the exception type or text within it.

Otherwise, if the process fails without closing the pipe, we hang again.

AFAIK, the process can't fail without closing the pipe, but even if it does, there's nothing we can do about it. The child process (i.e., the pytest --collect-only --bisect-tests-ids-to-fd call) has it's own copy of the FD and there's nothing we can do from the parent process to affect it. Closing the write side from the parent process won't affect the child's writing capability.

[maciej-gol]

Thanks for the fix! Will publish it later today!

[maciej-gol]

Released with 0.2.0. Again, thanks a lot for your contribution!