Skip to content

Conversation

@crwood
Copy link
Member

@crwood crwood commented Jun 6, 2022

The Supervisor test test_supervisor_restarts_process_when_killed has been failing intermittently on Windows, for some time -- perhaps because 3 seconds isn't enough time for a process to be killed on GitHub Actions' Windows runners, or perhaps because filesystem operations are extra slow on GHA, or perhaps because Windows re-uses PIDs more eagerly than other operating systems... In any case, it happens frequently enough to be a source of developer-frustration and, Windows-specific issues aside, it isn't a very good test anyway:

================================== FAILURES ===================================
________________ test_supervisor_restarts_process_when_killed _________________
tmp_path = WindowsPath('C:/Users/runneradmin/AppData/Local/Temp/pytest-of-runneradmin/pytest-0/test_supervisor_restarts_proce0')
    @inlineCallbacks
    def test_supervisor_restarts_process_when_killed(tmp_path):
        pidfile = tmp_path / "python.pid"
        supervisor = Supervisor(pidfile=pidfile, restart_delay=0)
        pid_1, _ = yield supervisor.start(PROCESS_ARGS, started_trigger="OK")
        Process(pid_1).kill()
        yield deferLater(reactor, 3, lambda: None)
        pid_2 = int(pidfile.read_text().split()[0])
>       assert pid_1 != pid_2
E       assert 3140 != 3140
tests\test_supervisor.py:82: AssertionError

This PR improves the intermittently-failing test by verifying (and awaiting on, for a more generous 10 seconds) the running and not-running states of the supervised process (by PID) in order to determine whether the Supervisor "restart" functionality is working correctly. The result of these changes is much closer to the intended behavior of the component being tested; it avoids the filesystem, accommodates for slower systems, and will waste less time on actually-faster ones.

@crwood crwood changed the title Fix Supervisor "restart" test Fix intermittently-failing Supervisor "restart" test Jun 6, 2022
@crwood crwood added the Windows label Jun 6, 2022
@crwood crwood merged commit 1a59778 into master Jun 6, 2022
@crwood crwood deleted the fix-supervisor-test branch June 6, 2022 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants