Criterion icon indicating copy to clipboard operation
Criterion copied to clipboard

The runner ignores exit statuses after the test is done executing

Open Snaipe opened this issue 8 years ago • 2 comments

After the discussion with @cbatten on #204 about address sanitizer, I went on to test my allegations since it's been a few releases since I haven't checked that particular synergy.

It turns out that the introduction of the new client/server code broke it. In particular, this bit reports the test success (or failure), but this happens while the worker is still alive -- and while an exit called in the test is going to trigger the death event, and fail the test, Address Sanitizer and Valgrind on the other hand exit with a non-zero exit status after the main has done executing, which is well after the success or failure of the test has been reported and printed.

The incorrect condition came from the change of semantics in the runner's protocol: before the refactor, the worker process was the test itself. Now, the runner conceptually supports workers that may run more than one test.

I think the best fix for that is to simply restrict the procotol to only accept one test per process, and then handle the reporting after the worker exits. Additional tests can always be run as sub-tests of the process "root" test.

Snaipe avatar Oct 09 '17 08:10 Snaipe

Interesting. Thanks for looking into this! I also noticed this when you mentioned that a valgrind error would cause the test to fail. I tried this:

#include <criterion/criterion.h>
#include <stdlib.h>

Test( leak, leak )
{
  int* ptr = malloc( sizeof(int) );
}

And then ran it like this:

 % valgrind --leak-check=full --trace-children=yes --error-exitcode=1 ./leak

And valgrind reports a memory leak error but the test still passes.

cbatten avatar Oct 09 '17 12:10 cbatten

Yes, this is the same issue. It used to work before the changes to the i/o layer, as the final exit status was used as a final override on the success failure of the test. Moving the actual test status report after the process exit should restore that behaviour.

Snaipe avatar Oct 09 '17 12:10 Snaipe