Fatal PHPUnit tests are not always reported as failure

How feasible would it be to run the phpunit tests via the phpunit executable at that point? I guess jenkins would already have support for all that.

Tried to start with that, but the problem is that PHPUnit opens up that file and overwrites it in http://cgit.drupalcode.org/drupal/tree/core/vendor/phpunit/phpunit/src/U...

+++ b/core/modules/simpletest/simpletest.module
@@ -178,8 +178,25 @@ function simpletest_run_tests($test_list) {
+  if ($ret == ".") {

MHH doesn,'t "." mean something broke on the first test, but there might be more. Can't you look at the return value 0, 1, etc?

The exec() command in simpletest_phpunit_run_command() needs to have the the optional 3rd argument which has the execution status. Then that var can be checked for 1/0 as @dawehner suggests in #8.

I am not sure what the behavior of return_var is on non-unix systems.

Please do not credit me, this is a trivial change. Also, since this blocks DrupalCI, it blocks Drupal 8 so it's a critical.

Can we add test coverage for this? This seems like the very definition of things we do not want to ever break again.

I doubt we can/should test this at this level ... ideally for example we would use phpunit directly in which case the test failure would need to be caught on the level of DrupalCI, as this would execute that script.
Its hard to test the testing system, this is for sure.

I am not sure if simpletest_phpunit_run_command is testable. Maybe passing in garbage as $phpunit_file, and hope the command crashes, and assert that ret is non-zero?

Well to be clear, there might be a chance to test it now, but on the longrun we really should move away from having a simpletest(phpunit()) wrapping construct,
as it makes things problematic. Once we have that not any longer, the test we will introduce here, won't work any longer.

@webchick your wish is my command. Here's a test.

Oh bother, a missing @group.

Edit: preg_match('/(.(?!b))*/', str_repeat("a", 10000)); is from http://ilia.ws/archives/5_Top_10_ways_to_crash_PHP.html -- it causes a segfault immediately. If a standalone PCRE library is chosen AND the --disable-stack-for-recursion option is given at compilation THEN this will not segfault. However, this is extremely unpopular (and no distributions ship with it) as it slows down PCRE for the sake of what is simply known as degenerate regexps. You can test by running pcretest -C | grep 'Match recursion' it will say (if you have a standalone PCRE install, that is) Match recursion uses stack.

Edit2: this is better than using bananas() to cause a fatal error because shutdown functions still might run after a fatal error. Nothing runs after the process itself is aborted this way. We can't abort reliably normally as exit/die will also run destructors and shutdown functions. Alternatively we could use $pid = getmypid(); exec("kill $pid"); exec("TSKILL $pid"); to get the OS to terminate the process immediately. Hopefully PHPunit won't add signal handlers... (it doesn't have one now).

1. +++ b/core/modules/simpletest/tests/fixtures/simpletest_phpunit_run_command_test.php
   @@ -0,0 +1,15 @@
   +
   +class SimpletestPhpunitRunCommandTestWillDie extends UnitTestCase {
   ...
   +      preg_match('/(.(?!b))*/', str_repeat("a", 10000));
   

   Can we have a quick one line comment what we do here?

2. +++ b/core/modules/simpletest/tests/fixtures/simpletest_phpunit_run_command_test.php
   @@ -0,0 +1,15 @@
   +  function testWillDie() {
   

   public ...

3. +++ b/core/modules/simpletest/tests/src/Unit/SimpletestPhpunitRunCommandTest.php
   @@ -0,0 +1,38 @@
   +    include_once __DIR__ .'/../../fixtures/simpletest_phpunit_run_command_test.php';
   +    $app_root = __DIR__ . '/../../../../../..';
   

   This one loads code so we need additional annotations, see IRC

Thank you. Otherwise we would have had another major, at least. You really don't want to break a workflow committers use.

Ran the phpunit tests manually:

phpunit testsuite unit
............................................................. 8540 / 8569 ( 99%)
.............................

Time: 42.1 seconds, Memory: 275.25Mb

OK (8569 tests, 17001 assertions)

1. +++ b/core/modules/simpletest/tests/fixtures/simpletest_phpunit_run_command_test.php
   @@ -4,10 +4,18 @@
   + *
   + * To avoid accidentally running, it is not in a normal PSR-4 directory, the
   + * file name does not adhere to PSR-4 and an environment variable also needs to
   + * be set for the crash to happen.
   + */
   

   Perfect, thank you!

2. +++ b/core/modules/simpletest/tests/src/Unit/SimpletestPhpunitRunCommandTest.php
   @@ -11,7 +11,12 @@
   + * @runTestsInSeparateProcesses
   + * @preserveGlobalState disabled
   

   This is needed in order to avoid #2565971: failures running phpunit from command line.

Unfollowing the issue: I had a green patch then came phpunit annotations and I can't reproduce the fail locally from running the test alone with phpunit, running the simpletest group with phpunit or running the test with simpletest. It always passes. I can't and won't fight phpunit, I let others do that. I was against shoveling in PHPunit without documentation, education and proper integration and now this is going to block the RC There was no comparison between modern testing framework done, we just added PHPunit just because. Well done.

Edit: I reported two years ago that the integration is broken #2096615: Testbot does not return PHPunit fails properly. PHPunit is still unmaintained and the integration never got fixed.

With Drupal CI we can run these outside simpletest right?

Past is past and time is limited.

I asked for this additional annotations because otherwise state will leak into the tests running. Test failures like #2565971: failures running phpunit from command line. are hard to debug and should be prevented
in beforehand. We have more criticals to solve, no reason to freak out :( It burns everyone out, who is involved in the process!

Edit: I reported two years ago that the integration is broken #2096615: Testbot does not return PHPunit fails properly. PHPunit is still unmaintained and the integration never got fixed.

You might have the right idea, get rid of the integration and rely on running phpunit directly. Its IMHO the only thing we can and should do!
#2566767: Don't allow running phpunit-based tests via the UI is the first issue of that kind. @Mixologic how difficult is it to run the phpunit tests separate?

I tried to use the drupalci_testrunner, but it turns out, I can't install it at the moment.
Feel free to commit #21 + the comments and maybe create a follow up?

This is the fail (getting it locally)

There was 1 error:

1) Drupal\Tests\simpletest\Unit\SimpletestPhpunitRunCommandTest::testSimpletestPhpUnitRunCommand
PHPUnit_Framework_Exception: Segmentation fault

Code I used to run it locally

./core/vendor/bin/phpunit -c ./core/ ./core/modules/simpletest/tests/src/Unit/SimpletestPhpunitRunCommandTest.php

The issue is in \PHPUnit_Util_PHP::processChildResult

Moar isolation

This passes locally

Let's hope it passes. This would be nice!

larowlan++

Worked on the issue summary / beta eval, just to be sure.

Well since I run all the phpunit tests before I commit most patches we don't actually have any broken tests but it is certainly critical that our test infrastructure does not pick up all test fails. Nice that this comes with a test. However the test does not work for me or @dawehner since the changes in #35. In fact it works as expected without those changes. I don't get why those change matter one jot - this is a totally separate invocation of phpunit so it is already isolated BUT it should not matter if it is. My recommendation here is to remove the tests and add a major followup to add them in when we can get them to work consistently for everyone.

#2566963: Follow up of 2560643 Add test coverage is the needed follow up

There we go, removed the tests

Since this is the exact same patch that already passed above, moving to RTBC.

Committed c27852c and pushed to 8.0.x. Thanks!