run-tests.sh should exit with a failure code if any tests failed

Changing the exit code to always be 1 in case of any test failures and errors sounds fine to me.

That said, the script already does so, but only in case of fatal errors (and uncaught exceptions if they manage to bubble up into test runner).

Therefore, only two adjustments should be necessary:

1. The case of regular case of test failures and exceptions: This can be retrieved from the test results array.
2. A fatal error in a spawned sub-process might not be properly caught and added as a failure to the test results array of the parent process.

Maybe this is a better approach? We dont want to see the fatal error message whenever a test is failing or has a exception.

1. +++ b/core/scripts/run-tests.sh
   @@ -98,7 +98,12 @@
   +if ($has_fails_or_exceptions) {
   +  exit(1);
   +}
   +else {
   +  exit;
   +}
   

   This can be written in a single line (conditional argument value instead of conditional code paths).

2. +++ b/core/scripts/run-tests.sh
   @@ -549,7 +557,7 @@ function simpletest_script_execute_batch($test_classes) {
   -        if ($status['exitcode']) {
   +        if ($status['exitcode'] == 1) {
              echo 'FATAL ' . $child['class'] . ': test runner returned a non-zero error code (' . $status['exitcode'] . ').' . "\n";
   

   The PHP exit code can be anything between 0 and 255.

   Unless I'm terribly mistaken, a Fatal error yields 255.

   In any case, a child process that does not exit with 0 (success) must always trigger a verbose error on the command line, because something/anything blew up in very unexpected ways (not caught by the testing framework's built-in error handling).

3. +++ b/core/scripts/run-tests.sh
   @@ -570,6 +581,7 @@ function simpletest_script_execute_batch($test_classes) {
   +  return $has_fails_or_exceptions;
   
   @@ -608,9 +620,15 @@ function simpletest_script_run_phpunit($test_id, $class) {
   +  return $has_fails;
   

   Let's consistently return exit codes (ideally constants) instead of Booleans, so that we don't translate them in multiple spots.

4. +++ b/core/scripts/run-tests.sh
   @@ -627,6 +645,10 @@ function simpletest_script_run_one_test($test_id, $test_class) {
   +    if ($test->results['#fail'] || $test->results['#exception']) {
   +      exit(2);
   +    }
   

   This appears to implement what I mentioned in my earlier comment; good start.

   However, there is a standard for I/O process exit status codes. We should use the closest matching one.

   (unless you verified that 2 is a compatible code already?)

I've tried to fix most of the things you said or do things a little differently, I'm just not sure what exit code to use, suggestions are welcome! I wasn't able to find a complete list anywhere.

+++ b/core/scripts/run-tests.sh
@@ -608,9 +613,15 @@ function simpletest_script_run_phpunit($test_id, $class) {
+  exit($exit_code);

This would never work of course since it would exit the function after the first PHPUnit tests. I could return the exit code but I think its a little messy. What would a better approach?

@benjy I'll think about interdiffs on next patches.

Alright, this is the best I can come up with at this moment.

Base libc definition of process exist status codes:
http://www.gnu.org/software/libc/manual/html_node/Exit-Status.html

POSIX process exit status codes are defined by sysexits.h in glibc:
- Canonical source: http://osxr.org/glibc/source/misc/sysexits.h
- More elaborate interpretation: http://www.freebsd.org/cgi/man.cgi?query=sysexits&sektion=3

Bash process exit status codes:
http://tldp.org/LDP/abs/html/exitcodes.html

PHPUnit process exit status codes:
https://github.com/sebastianbergmann/phpunit/blob/master/src/TextUI/Test...

Note that EXCEPTION_EXIT (2) is used both for (internal) test runner execution errors as well as (user-space) errors in tests. Simpletest makes no differentation between errors and exceptions, so the latter is always FAILURE_EXIT (1).

Let's simply be consistent with PHPUnit.

In case this happens, the change should be backported to D7 (+ possibly to the D6 simpletest contrib module), so the test runners are consistent.

Needs to be updated for #2310415: run-tests.sh does not handle the error when invalid test groups/classes are specified

Has this been coordinated with the testbot maintainers? IIRC the testbot relies on exit statuses, but I could be wrong here.

+++ b/core/scripts/run-tests.sh
@@ -518,7 +524,10 @@ function simpletest_script_execute_batch($test_classes) {
+        if ($phpunit_status > $total_status) {
+          $total_status = $phpunit_status;
+        }

Why not use max()?

Testbot is definitely fine. This patch only changes failure conditions anyway. PIFR reports failures despite the lack of proper exit codes already.

Next to #18, @Xano you mentioned in IRC that you encountered an exit code 1 for a passing test (without this patch); any new clues on that, or is that obsolete?

No new clues on that. Note that Travis says the run-tests.sh command exited with 0, but that storing the status in a variable and using it later results in a status of 1. See https://travis-ci.org/bartfeenstra/currency_test/jobs/33989295. Line 3618 contains Travis' result of the run-tests.sh command and lines 3626 and 3627 are the result of storing the exit status in a variable and printing it in the terminal.

@Xano that is a travis problem, we solved that by moving the exit status parsing to the same line, has nothing to do wit this issue. See https://github.com/md-systems/redirect/commit/f582109358c64e7cf20d26b356...

Reroll.

For some reason I can't create an interdiff?

josh@dev:~/web/runtestsfull$ interdiff 2189345-runtests-exitcode_16.patch 2189345-runtests-exitcode_27.patch
1 out of 9 hunks FAILED -- saving rejects to file /tmp/interdiff-1.XpScJO.rej
interdiff: Error applying patch1 to reconstructed file

Straight re-roll, no interdiff.

Straight re-roll, no interdiff.

Patch looks good and applies fine.
It returns 0 on success, 1 on failure/exception and 255 on fatal error.

As per #10 I changed all calls to exit, exit(), exit(1) to exit(SIMPLETEST_SCRIPT_EXIT_SUCCESS) or exit(SIMPLETEST_SCRIPT_EXIT_FAILURE) respectively – just to be more verbose and consistent.
Patch and interdiff attached.

Otherwise this looks ready to go!

D7 backport attached.

1. Changed that to distinguish between --help and missing args
2. Nice catch, changed that too.
And changed some more occurences of SIMPLETEST_SCRIPT_EXIT_SUCCESS after simpletest_script_print_error to SIMPLETEST_SCRIPT_EXIT_FAILURE
3. As multiple tests are run in a batch, we always want to return the most serious status code on exit, not just the last.
4. I don't think so. Exceptions are taken care of in the else statement following 5 lines further down
5. Same as in three.

Attached is a new patch, interdiff and backport updated.

As per #2189345: run-tests.sh should exit with a failure code if any tests failed this is actually not an API change but rather a correction.
Also, looking at workarounds like
https://github.com/md-systems/redirect/blob/f582109358c64e7cf20d26b35667...
https://github.com/bartfeenstra/currency_test/blob/8.x-3.x/.travis.yml#L83
https://github.com/larowlan/token/blob/8.x-1.x/.travis.yml#L84
from my point of view this should be considered a bugfix.

Yeah I agree this is bugfix but also testing infrastructure changes are not prohibited during beta. This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 1edfae7 and pushed to 8.0.x. Thanks!

This is a straight port just missing the phpunit part not available in D7, so it should be just fine.
But I'm not going to RTBC my own patch.

Follow-up #2376039: Undefined property ContainerAwareEventDispatcherTest::results in run-tests.sh.

@benjy it is not causing the problem, but it currently makes test-results from qa.drupal.org unusable (if there are other errors). For example #2324055-66: Split up the module manager into runtime information and extension information. has only one exceptions and zero test-fails, still the status of the patch is "Failed to run tests: failed during invocation of run-tests.sh.". As a result, the detailed table of test-results is also not displayed on the qa-page, rather only the test log is shown.

This is causes issues with testbot reporting... rolling back.

See https://qa.drupal.org/pifr/test/910608

This patch is causing a random issue with the testbot infrastructure. If there is a fail during a test run, the fails are not reported properly. See https://qa.drupal.org/pifr/test/910608 an example. This test tun had #2376039: Undefined property ContainerAwareEventDispatcherTest::results in run-tests.sh applied so that did not actually fix the issue which is why I reverted this patch.

I guess that qa.drupal.org does not expect non-zero exit status unless there is a fatal error. This patch changes the exit status for fatal errors from 1 to 2 and for soft-failures (failed tests) from 0 to 1.

Therefore this needs to be coordinated with the pifr team (probably rfay or jthorson). Maybe it would be possible to commit this in two pieces:

1. Change the exit status for fatal errors from 1 to 2 (for D8 and D7). Basically this would be the current patch but with SIMPLETEST_SCRIPT_EXIT_FAILURE set to 0.
2. Let the qa-team roll out changes to the test-bot, such that only exit codes > 1 are considered fatal errors.
3. Commit a trivial patch which changes SIMPLETEST_SCRIPT_EXIT_FAILURE to 1.

reroll, no further changes.

needs test

Reroll of the patch in #51.

klausi opened a new pull request for this issue.

Just a reroll, no other changes.

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

Another reroll, resolved a merge conflict.

Reroll for latest HEAD.

Attached is a straight reroll, no changes.

I was working around this issue by using Drush to run the tests, since Drush was correctly returning an error code on failure. The test runner now has been removed from Drush: https://github.com/drush-ops/drush/issues/1362

The problem is that we first need to change pifr (or pift?) to detect soft-failure ("Detect a failing test") vs. hard failure ("Detect a test run failure"). See #46ff.

I have started using this but hit a case where tests failed but the script still returned 0: after all tests ran the following exception occurred when the script tries to compile the test results:

PDOException: SQLSTATE[HY000]: General error: 2006 MySQL server has gone away: SELECT last_prefix FROM {simpletest_test_id} WHERE test_id = :test_id LIMIT 0, 1; Array
(
    [:test_id] => 1
)
 in simpletest_last_test_get() (line 238 of /home/travis/build/pfrenssen/drupal/build/modules/simpletest/simpletest.module).

This is caused by a too low wait_timeout setting in MySQL. I'm using Travis CI and it defaults to only 180 seconds, if a test suite runs longer than this then MySQl closed the connection and this exception is thrown.

What would be the best way of solving this? We cannot use set_exception_handler() since there is already one being set in the bootstrap. Shall we wrap the entire test runner in a try-catch block?

Added try-catch blocks around all the function calls that do database calls, and also when creating the request since this can also throw a multitude of exceptions.

This does not work for PHPUnit tests. I made some modifications to the core BlockFormTest (Unit) and BlockCacheTest (requiring database), so that they both fail, and here is the output on Ubuntu:

The web test gives the exit code 1, as expected:

root@0b353bdfb134:/srv/drupal/www# php ./core/scripts/run-tests.sh --class Drupal\\block\\Tests\\BlockCacheTest

Drupal test run
---------------

Tests to be run:
  - Drupal\block\Tests\BlockCacheTest

Test run started:
  Thursday, May 14, 2015 - 21:27

Test summary
------------

Drupal\block\Tests\BlockCacheTest                             80 passes  81 fails  32 exceptions             
- Found database prefix 'simpletest708195' for test ID 2.

Test run duration: 1 min 13 sec

root@0b353bdfb134:/srv/drupal/www# echo $?
1

However, when running a unit test, the exit code is still 0, despite the fact that there is a failture (I inserted $this->assertTrue(FALSE); in the code):

root@0b353bdfb134:/srv/drupal/www# php ./core/scripts/run-tests.sh --class Drupal\\Tests\\block\\Unit\\BlockFormTest  

Drupal test run
---------------

Tests to be run:
  - Drupal\Tests\block\Unit\BlockFormTest

Test run started:
  Thursday, May 14, 2015 - 21:32

Test summary
------------

Drupal\Tests\block\Unit\BlockFormTest                          0 passes   1 fails                            
PHP Notice:  Undefined property: Drupal\Tests\block\Unit\BlockFormTest::$results in /srv/drupal/www/core/scripts/run-tests.sh on line 707

Notice: Undefined property: Drupal\Tests\block\Unit\BlockFormTest::$results in /srv/drupal/www/core/scripts/run-tests.sh on line 707
PHP Notice:  Undefined property: Drupal\Tests\block\Unit\BlockFormTest::$results in /srv/drupal/www/core/scripts/run-tests.sh on line 707

Notice: Undefined property: Drupal\Tests\block\Unit\BlockFormTest::$results in /srv/drupal/www/core/scripts/run-tests.sh on line 707

Test run duration: 0 sec

root@0b353bdfb134:/srv/drupal/www# echo $?
0

FYI, as a workaround if you do need to have a failure when running PHPUnit tests, until this is corrected, you can run PHPUnit directly like so (this uses the same example as above with a modified version of BlockFormTest which fails):

root@0b353bdfb134:/srv/drupal/www# core/vendor/phpunit/phpunit/phpunit --bootstrap ./core/tests/bootstrap.php core/modules/block/tests/src/Unit/BlockFormTest.php
PHPUnit 4.4.2 by Sebastian Bergmann.

F

Time: 182 ms, Memory: 4.50Mb

There was 1 failure:

1) Drupal\Tests\block\Unit\BlockFormTest::testGetUniqueMachineName
Failed asserting that false is true.

/srv/drupal/www/core/modules/block/tests/src/Unit/BlockFormTest.php:89

FAILURES!
Tests: 1, Assertions: 1, Failures: 1.
root@0b353bdfb134:/srv/drupal/www# echo $?
1

1. +++ b/core/scripts/run-tests.sh
   @@ -560,7 +610,13 @@ function simpletest_script_execute_batch($test_classes) {
   +        if ($status['exitcode'] == SIMPLETEST_SCRIPT_EXIT_FAILURE) {
   

   nit ===

2. +++ b/core/scripts/run-tests.sh
   @@ -560,7 +610,13 @@ function simpletest_script_execute_batch($test_classes) {
   +          if ($status['exitcode'] > $total_status) {
   +            $total_status = $status['exitcode'];
   +          }
   

   can we use max() here?

Otherwise this looks good to me

Thank you @larowlan for helpful review. Here is a patch with changes you've suggested.

Time to fix the codestyle a bit.

> phpcs --standard=Drupal core/scripts/run-tests.sh 

FILE: /home/sauditor/workspace/drupal/drupal/core/scripts/run-tests.sh
----------------------------------------------------------------------
FOUND 83 ERRORS AND 7 WARNINGS AFFECTING 67 LINES
----------------------------------------------------------------------
edit: <--!snip-->

I'm not entirely sure that we should fix codestyle here. I would rather opt for follow-up issue.

I marked issue RTBC, so committers can look at it and say if we should fix it now or in separate issue.

@sanduhrs
I think the idea with codestyle issues is to have issues per codestyle rule so we minimize the complexity of reviews.

I'm perfectly fine doing followups, then.

Just a heads-up that if you search the issues for the name run-tests.sh you get zero result. I had to use google to get here.

+1 on proper return values from run-tests.sh.

Here's your coding standards follow-up: #2605290: Improve docs, coding standards for run-tests.sh

Testbot was in a bad mood again - back to RTBC.

@benjy rc eligible is the correct tag for tests only changes.

So have we confirmed that this does not cause any problems for DrupalCI - when this was originally committed we rolled it back because it caused pifr problems. It would be great to have some failing patches that should this doesn't break DrupalCI reporting.

I'd make a failing test here to demo, but...

The patch in 81 didn't apply, so I tried to reroll.

When I did, there ended up being no diff.

Did this get committed somewhere?

@Mile23 all sorted now

So here are two patches.

One is only a module added to modules/ with two failing tests in it: One each SimpleTest and PHPUnit.

The other is the patch from 81 with the same extra module.

Here's what it looks like when I run it locally:

$ php ./core/scripts/run-tests.sh --module some_module

Drupal test run
---------------

Tests to be run:
  - Drupal\Tests\some_module\Unit\FailingUnitTest
  - Drupal\some_module\Tests\FailingSimpleTest

Test run started:
  Tuesday, November 3, 2015 - 21:04

Test summary
------------

Drupal\Tests\some_module\Unit\FailingUnitTest                  0 passes   1 fails                            
Drupal\some_module\Tests\FailingSimpleTest                     1 passes   1 fails                            
- Found database prefix 'simpletest678751' for test ID 8.

Test run duration: 27 sec

$

Not sure why it shows me that it found the prefix.

At the moment the tests-only patch is still running but the 81+tests one says this in the CI log https://dispatcher.drupalci.org/job/default/31559/console :

20:33:29 Drupal\system\Tests\Module\InstallUninstallTest              2583 passes   3 fails                            
20:34:27 Drupal\views_ui\Tests\ViewEditTest                           224 passes                            1 messages
20:34:27 Drupal\views_ui\Tests\DisplayTest                            313 passes                                      
20:34:27 
20:34:27 Test run duration: 29 min 43 sec
20:34:27 
20:34:27 HTTP/1.1 200 OK
20:34:27 Content-Type: application/vnd.docker.raw-stream
20:34:27 
20:34:27 
20:34:27 Error
20:34:27 Received a non-zero return code from the last command executed on the container.  (Return status: 1)
20:34:27 Execution Error
20:34:27 Error encountered while executing job build step execute:command
20:34:27 Checking console output
20:34:27 Recording test results
20:34:27 ERROR: Publisher 'Publish JUnit test result report' failed: No test report files were found. Configuration error?
20:34:27 Finished: FAILURE

#2378713: Results broken when run-tests exits with appropriate error code waiting for review.

#2378713: Results broken when run-tests exits with appropriate error code is now in production, and I manually requeued the patches in #106. Could somebody more familiar with the changes here evaluate the impact of #2580293: Patch having test with "PHP Fatal error" is marked as PASSED https://www.drupal.org/node/2580293#comment-10558844 and determine if we need to handle anything additional here as a result?

Here's a reroll after #2580293: Patch having test with "PHP Fatal error" is marked as PASSED

It's a reroll of #104 which is a reroll of #81. :-)

Patch, patch with added failing tests, failing tests only.

Dunno. I queued it again to see if it was testbot related. Did seem strange that it was a database error like that.

Both Database errors seem to be totally unrelated to this patch.
No idea where the error in LocalePluralFormatTest came from, but seems to be unrelated, too.
Everything else working as expected, so:
+1 RTBC

Disabled the by intention failing tests to calm testbot.

More files to hide.

#112 passes and is RTBC :)

Personally I don;t think this can be backported as anyone using run-tests.sh for continuous integration has probably already got their own solutions.

I'm committing this work now because I think it makes sense to have in for Drupal 8.0.0 and represents very little risk now that we've proved the Drupal CI is not broken by this change. Committed 37705f2 and pushed to 8.0.x. Thanks!

Also desperately needed for D7, updating issue status.

Re-uploading the patch form #39 for visibility.

+1

+++ b/scripts/run-tests.sh
@@ -8,12 +8,16 @@ define('SIMPLETEST_SCRIPT_COLOR_PASS', 32); // Green.
+  exit(($count == 0) ? SIMPLETEST_SCRIPT_EXIT_FAILURE : SIMPLETEST_SCRIPT_EXIT_SUCCESS);

I didn't notice this until now, and it's in the other patch, but sending a "There were failed Tests" signal when you do not provide arguments to the script is unusual. It should probably throw the SIMPLETEST_SCRIPT_EXIT_EXCEPTION signal instead, but otherwise LGTM.

+1 on getting this into d7 as is.

This looks good to me and I agree backporting it to Drupal 7 is reasonable (with a release notes mention so people know the exit code is changing).

However based on the above discussion it seems worth double-checking that this doesn't cause any problems with the Drupal 7 testbot actually failing patches that it's supposed to fail, so here is a quick test of that.

Hm, it looks like there is a problem after all (similar to the ones reported earlier in this issue for the old testbot).

Instead of reporting the failures it's reporting a "CI error" with this in the logs:

01:17:33 Error
01:17:33 Received a non-zero return code from the last command executed on the container.  (Return status: 1)
01:17:33 Execution Error
01:17:33 Error encountered while executing job build step execute:command

I'm setting this back to "needs review" because I don't think there's anything wrong with this patch; however, it may need to wait for changes to the testbot before it can be committed.

Been using #39 for at least 6 month now. Various CI providers handle this properly.

Has the Drupal testbot been updated to be ready for this patch?

#39 works for me in Travis CI.

Re-testing #173.

+1 to the approach, I worked around this in drupal_ti by parsing the output.

This needs to be fixed in DrupalCI first.

This is a pretty simple fix in drupalci, and I cant see a whole lot of risk in putting it in now: please watch #2756485: Update drupal7 to use the testcommand for handling signals for updates.

I've deployed the change to drupalci that will no longer treat signal > 0 coming back from the testrunner as abnormal, now it treats signal > 1 as abnormal, and treats a 1 as "failed tests"

I also requeued the tests in #170, and the middle one no longer says CI Error, it says 1 Fail, like it should.

Awesome, back to RTBC then.

Marking for commit later.

Committed to 7.x - thanks!

An interesting side effect of this patch is that if a test fails early (for example, a PDOException in the setUp() method), previously the testbot would report that particular test case as failing. Now it just reports it as "CI error". You can see the difference by comparing the first two tests at https://www.drupal.org/node/1970534#comment-11363869

In some ways this is a good thing - a lot of times when a test fails that early it's because of one of the random testbot failures we sometimes see (i.e. it has nothing to do with the patch that was being tested, so "CI error" is better than a message about a specific test failure).

But for others e.g. PostgreSQL and SQLite tests at https://www.drupal.org/node/2656548#comment-11363217 it would be better to see the actual failures. (Although you still can get them in some form by clicking through to the console output.)

Not sure which we want: A followup to the core patch here to deal with that, or a followup on the testbot to be able to handle this situation?

Because what the current code is doing is this:

function simpletest_script_run_one_test($test_id, $test_class) {
  try {
    // Bootstrap Drupal.
    drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL);

    simpletest_classloader_register();

    $test = new $test_class($test_id);
    $test->run();
    $info = $test->getInfo();

    $had_fails = (isset($test->results['#fail']) && $test->results['#fail'] > 0);
    $had_exceptions = (isset($test->results['#exception']) && $test->results['#exception'] > 0);
.....
    if ($had_fails || $had_exceptions) {
      exit(SIMPLETEST_SCRIPT_EXIT_FAILURE);
    }
    exit(SIMPLETEST_SCRIPT_EXIT_SUCCESS);
  }
  catch (Exception $e) {
    echo (string) $e;
    exit(SIMPLETEST_SCRIPT_EXIT_EXCEPTION);
  }
}

And I think the problem comes when it exits with SIMPLETEST_SCRIPT_EXIT_EXCEPTION.

Given that this exception happens either as a result of a failure boostrapping Drupal, or an exception that happens while initializing the test, do we really want to treat it differently than a test which has an exception inside the test results (and which exits with SIMPLETEST_SCRIPT_EXIT_FAILURE instead)?

What are we doing for Drupal 8 in the same situation? (e.g. on testbot)

I think that is what is important to check first as parity should remove headaches in the future.

@Fabianx: See #37.

From the code it looks like it should have the same problem (unless there is a difference on the testbot). I just posted https://www.drupal.org/node/1970534#comment-11383991 to find out.

I think the difference here is that Drupal 8 does not install a Drupal site in order to run tests. I don't think we backported that change.

I think it actually might just be because Drupal 8 does a try/catch around test setUp() methods and Drupal 7 doesn't. That was added in #2170023: Use exceptions when something goes wrong in test setup.

I've created an issue for Drupal 7 to try to at least backport some of that, which hopefully will take care of the regression noted above: #2763435: Exceptions during the setUp() or tearDown() method of a test are not handled

However it's interesting that if you look at my above test results for Drupal 8 (https://dispatcher.drupalci.org/job/default/169015/consoleFull), Drupal 8 is still getting a fatal error about the test runner returning with a non-zero error code (which suggests the script is still exiting with SIMPLETEST_SCRIPT_EXIT_EXCEPTION).... But in Drupal 8 it doesn't prevent it from reporting the issues as test failures. Not quite sure what's going on with that:

04:48:48 Drupal\Tests\Core\UrlTest                                      0 passes  91 fails                            
04:48:48 FATAL Drupal\Tests\Core\UrlTest: test runner returned a non-zero error code (2).
04:48:48 Drupal\Tests\Core\UrlTest                                      0 passes   1 fails