run-tests.sh does not return a non zero status when tests fail with exception

Not clear whether this could lead to un-caught DrupalCI failures, but bumping to critical since it definitely means false negatives running tests locally or on travis.

It doesn't cause issues on DrupalCI because it uses the junit XML output ...

Had the same for D7 #2189345: run-tests.sh should exit with a failure code if any tests failed.

The existing code looks like the following:

        if ($status['exitcode'] === SIMPLETEST_SCRIPT_EXIT_FAILURE) {
          $total_status = max($status['exitcode'], $total_status);
        }
        elseif ($status['exitcode']) {

Together with this addition we can quite improve the code by getting rid of the if and else

We tried to implement #6, but the code is quite fragile, and making it actually working, makes the code not really easier to read.
Given that

Committed/pushed to 8.3.x and 8.2.x thanks!

With 8.1.x I'm going to leave it RTBC for a bit, but should be cherry-picked there too. Might make some tests builds fail, but that's the point.

Had to revert this. Fatal errors in tests were killing DrupalCI runs because it only looks for 0 and 1 status codes.

The correct error statuses for a testing script are 0 - ran and tests passed, 1 - ran and tests failed, and anything > 1 is treated as the test-running software had an abnormal abort. So if there is a Fatal Error in a test, that should not be

max($status['exitcode'], $total_status);

This should be just 1. no matter why a test fails or aborts, 1 is the response we're looking for. if run-tests.sh itself dies, then the return signal should reflect that it aborted.

ok, but that is pretty poor in general.

We should probably mirror what junit/phpunit do, which is exit 2 on an exception:

https://github.com/junit-team/junit4/blob/9e98a85ecf6e857523fcda91507562...
https://github.com/sebastianbergmann/phpunit/blob/1ef3ff15e87f6bd69cb116...

That is correct, I did some more reading and research, and have more information. I would stick to 1 or 0 for now, until drupalci knows what to do with a 2.

That being said, however, Im not entirely sure if/how drupalci should do anything different with exceptions vs failed tests. Should the testing system behave any differently if 'tests failed, because they couldnt run due to an exception' vs 'tests failed, because something's not right?' other than report different messaging?

Amusingly run-tests.sh is already ready with a constant for that too. :) drupalci just needs to be updated.

http://cgit.drupalcode.org/drupal/tree/core/scripts/run-tests.sh#n35

I started to RTBC this but should we make a @todo and a follow up to fix this once drupalci is ready to handle the separate exit code?

Opened a related drupalci issue.

Being as the problem we had was specifically with testbot and to be super sure we don't disrupt anything just confirming that this patch fails well. Think we can RTBC this if it does.

son of a... so... the previous one is the interdiff and I'll where a brown paper bag for the rest of the day.

perdy

Committed/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!