More gracefully handle crashes in pytest.

[clalancette]

When the pytest executable is executing, there is some possibility that it can crash (due to a bug in C extensions, for instance). In the current code, this circumstance causes the main colcon executable to return a non-zero exit number.

But that doesn't seem correct; colcon didn't fail, one of the tests did. The test should be marked as failing, but colcon itself should return a success code. Additionally, we notice that if a e.g. ctest test crashes, colcon returns 0.

The reason this happens only with pytest is because pytest doesn't have a mechanism to suppress these kinds of errors. Thus if the python interpreter in the pytest process crashes, pytest returns the segmentation fault number (-11 on Linux, 0xc0000005 on Windows).

Handle this by explicitly capturing this fault number, and setting the test to failed. Unfortunately there are no convenient constants in Python that I could find for these numbers, so they are hardcoded here.

This does change the user-visible functioning of colcon, but I believe it does so for the better, and it is a bit of a corner case.

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.

Project coverage is 87.22%. Comparing base (a74fa97) to head (b6e1f79).

Files with missing lines	Patch %	Lines
colcon_core/task/python/test/pytest.py	0.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #682      +/-   ##
==========================================
- Coverage   87.28%   87.22%   -0.07%     
==========================================
  Files          68       68              
  Lines        3949     3952       +3     
  Branches      760      761       +1     
==========================================
  Hits         3447     3447              
- Misses        396      399       +3     
  Partials      106      106              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mikepurvis]

This looks good to me. Gracefully handling tests that crash rather than just fail has always been a bit tricky, but this seems reasonable enough.

[cottsay]

I have to say that I disagree that this is the right way to handle this.

colcon didn't fail, one of the tests did. The test should be marked as failing, but colcon itself should return a success code. Additionally, we notice that if a e.g. ctest test crashes, colcon returns 0.

We don't know that one of the tests failed. We don't know if any of the tests even ran. We don't treat invocation of a test framework as a test itself anywhere else in colcon. Frameworks like ctest operate on a process model in which the executor itself is isolated from the running tests, but pytest's default behavior is to run tests in the same process as the executor. A crash in the executor process doesn't constitute a "test failure" and is rather a "failure to test". In effect, there was an error running the tests. Information is lost, and we may not be able to draw certain conclusions from the test results anymore. Maybe we ran all of the tests, but maybe we didn't run any of them, we simply don't know. Maybe the failure to test is the result of a missing dependency, or maybe the framework itself is broken.

While we may be able to improve the console output when pytest crashes like this, dumping the package name into a test failure event like this is a hack. There are other ways this can be mitigated without losing information.

If you have a package that's specifically struggling here, perhaps working around the problem in colcon isn't the right approach. Perhaps leveraging an extension like pytest-isolate is a better way to prevent the test executor from being affected by a crash.

Is your motivation to improve how this is handled on ci.ros2.org?

[clalancette]

Is your motivation to improve how this is handled on ci.ros2.org?

Yes, this is the primary motivation. Right now if a pytest invocation crashes, it causes colcon to return a non-zero exit code, which ends up making the entire job a "failure". But that isn't correct; the job didn't fail, one test just crashed, which should be reported as a failed test.

We don't know that one of the tests failed. We don't know if any of the tests even ran. We don't treat invocation of a test framework as a test itself anywhere else in colcon. Frameworks like ctest operate on a process model in which the executor itself is isolated from the running tests, but pytest's default behavior is to run tests in the same process as the executor. A crash in the executor process doesn't constitute a "test failure" and is rather a "failure to test". In effect, there was an error running the tests. Information is lost, and we may not be able to draw certain conclusions from the test results anymore. Maybe we ran all of the tests, but maybe we didn't run any of them, we simply don't know. Maybe the failure to test is the result of a missing dependency, or maybe the framework itself is broken.

I'm not sure I totally agree with this, given the nature of the fix. It is true that there are other reasons for the test "runner" to fail, but I don't think that those reasons would show up as a SIGSEGV (in other words, I don't think that pytest would return -11 on Linux, though I have not verified that). While I can of course construct scenarios where pytest would return SIGSEGV due to something other than a test crash, those would be really weird events (like the Python interpreter itself crashing).

The other thing that I'll argue is that ctest behaves in exactly this way. If a ctest test fails, then the ctest runner continues on and marks the test as failed. I guess this is more fine-grained reporting than what pytest does, but I consider the situation to be very analogous.

If you have a package that's specifically struggling here, perhaps working around the problem in colcon isn't the right approach. Perhaps leveraging an extension like pytest-isolate is a better way to prevent the test executor from being affected by a crash.

I didn't know about pytest-isolate, thanks. That is something to consider for ci.ros2.org. That said, I still think that this fix is viable, because it fixes a very specific case for pytest runners that local users (or github workflow users) could run into.