58f9a0a Use --failfast when running functional tests on Travis (James O'Beirne)
bf720c1 Add --failfast option to functional test runner (James O'Beirne)
Pull request description:
Add the option (`--failfast`) to stop the functional test runner's execution when it encounters the first failure. Also cleans up run_test's arguments list ([no more mutable default for `args`](http://docs.python-guide.org/en/latest/writing/gotchas/#mutable-default-arguments)) and call site.
Tree-SHA512: e854b1b1634bf613ae8ae88e715df1460982fa68db9d785aafeb5eccf5bf324c7f20dded2ca6840ebf18a28347ecac2138d6c7592507b34939b02609ef55e1b3
Partial backport of Core PR13105
This backport only includes some of the --failfast behavior since our test_runner has diverged so much.
This patch implements --failfast in a way that does not kill currently running tests like the original PR does.
This is due to a limitation of the thread-level parallelization that we implemented in test_runner.
Though the documentation on these matters is extremely scattered, I will attempt to sum up my conclusions:
Threading implementation (current) + modifications that match this backport:
- Attempting to kill running tests either fails completely or leaves child bitcoind processes running. My understanding is this is due to a limitation in the way Python handles threads that spawn processes.
Multiprocessing implementation:
- I attempted a full migration from Threading to Multiprocessing to alleviate the above.
- Despite documentation indicating that killing a Multiprocess will also kill it's children, this behavior does not appear to work as intended. Some example issues that have persisted even in recent Python versions that might be related: https://bugs.python.org/issue9205 https://bugs.python.org/issue22393 While it's difficult to say exactly which of these issues is related to the root cause, it appears that we will not find a stable version of Python that supports our Multiprocessing needs in the near future. It also looks like I'm not the only one with such frustration: https://stackoverflow.com/questions/1408356/keyboard-interrupts-with-pythons-multiprocessing-pool#comment101399084_1408476 (note the timeline of the entire conversation: 2010 - 2019)
- Despite getting a mostly-working implementation using various workarounds, I ran into all sorts of signal passing issues where SIGKILL, SIGTERM, and SIGINT would not behave as expected when these signals originated from worker processes. This was especially annoying, as it effectively had the same behavior as the Threading implementation when you attempt to Ctrl+C test_runner: leaving dangling bitcoind processes for an unknown amount of time after test_runner had quit.
After much experimentation, it appears there is no stable way to kill the currently running tests with widely
available Python versions (my current is 3.7.3). Even if we could get something to work, it is clear to me
that the maintenance cost of doing so just to pull in --failfast is not worth it. We should expect the
default to be that the test suite passes, so I'm not concerned with a small performance hit on --failfast.
It also appears that Core's test_runner runs tests in parallel now, so doing a migration to a more up-to-date
version of their test_runner is likely our best course of action in the long run.
Pulling in this patch sooner rather than later will help improve UX of our future land bot when test failures
occur by reducing patch queuing time.