QuickStartTest: The waiting is the hardest part...

I checked and no other core tests that use Symfony\Component\Process\Process have such potentially infinite loops in them.

Critical really (blocks one, and makes the test un-debuggable).

I was referring to the Process docs, which say:

For long running commands, it is your responsibility to perform the timeout check regularly:

$process->setTimeout(3600);
$process->start();

while ($condition) {
    // ...

    // check if the timeout is reached
    $process->checkTimeout();

    usleep(200000);

I think Process::wait() is for when you have two asynchronously running processes, and you want one to wait on the other. Whereas this waiting within a single process, using a pattern in the docs.

Here's a test patch which should prove that the timeouts are now working as expected. Edit: Crosspost.

I also think the 500 second timeout is really excessive; should we drop it to 30?

That seems risky; I wouldn't want to introduce a bunch of unpredictable random fails due to slow testbots. We should look back at the issue that added it originally and see if there's justification for the choice there.

The 500 timeout was added with the original introduction of the quick-start command.

Yeah, I read over the original issue and people were getting timeouts at 300 seconds, so they increased it to 500. This was probably with Composer 1, mind, so we might not need such a timeout anymore in D10. Still, I think it's best to keep it for now. 500 seconds is much better than the effectively infinite timeout currently. ;)

The fail-only patch in #9 fails as expected and proves that the timeouts are now working.

#8 fails QuickStartTest with:

[ayrton:core | Fri 13:02:53] $ ../vendor/bin/phpunit tests/Drupal/Tests/Core/Command/QuickStartTest.php
PHPUnit 8.5.21 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\Core\Command\QuickStartTest
.SF..                                                               5 / 5 (100%)

Time: 8.87 seconds, Memory: 6.00 MB

There was 1 failure:

1) Drupal\Tests\Core\Command\QuickStartTest::testQuickStartInstallAndServerCommands
Failed asserting that 'Drupal development server started: <http://127.0.0.1:8896>\n
This server is not meant for production use.\n
' contains "127.0.0.1:8896/user/reset/1/".

/Users/xjm/git/drupal-test-site/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:119
/Users/xjm/git/drupal-test-site/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
/Users/xjm/git/drupal-test-site/core/tests/Drupal/Tests/Core/Command/QuickStartTest.php:212
/Users/xjm/git/drupal-test-site/vendor/phpunit/phpunit/src/Framework/TestResult.php:703

FAILURES!
Tests: 5, Assertions: 10, Failures: 1, Skipped: 1.

@ressa :)

I tested #8 locally and confirmed that it does make the processes respect the timeout; shortening the timeout to 1 second fails as expected. I also tested with the original change from #2917655: [9.4.x only] Drop official PHP 7.3 support in Drupal 9.4 on PHP 7.3 and confirmed that the test now times out correctly rather than getting stuck in an infinite loop. So that's probably a better approach; we just need to get that third hunk working.

I moved #8 to a new MR and I think I fixed it.

Patch of this combined with #2917655: [9.4.x only] Drop official PHP 7.3 support in Drupal 9.4 to demonstrate that PHP 7.3 should now time out properly rather than waiting forever.

And, a version with artificially low timeouts to prove the other cases will time out properly as well.

#22 has:

Command.Drupal\Tests\Core\Command\QuickStartTest
✗	
Drupal\Tests\Core\Command\QuickStartTest
exception: [Other] Line 0 of sites/default/files/simpletest/phpunit-105.xml:
PHPUnit Test failed to complete; Error: PHPUnit 8.5.21 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\Core\Command\QuickStartTest
SES..                                                               5 / 5 (100%)

Time: 8.36 minutes, Memory: 4.00 MB

There was 1 error:

1) Drupal\Tests\Core\Command\QuickStartTest::testPhpRequirement
Symfony\Component\Process\Exception\ProcessTimedOutException: The process "'/usr/local/bin/php' 'core/scripts/drupal' 'quick-start' 'standard' '--site-name='\''Test site test32770308'\''' '--suppress-login'" exceeded the timeout of 500 seconds.

/var/www/html/vendor/symfony/process/Process.php:1254
/var/www/html/vendor/symfony/process/Process.php:430
/var/www/html/vendor/symfony/process/Process.php:251
/var/www/html/core/tests/Drupal/Tests/Core/Command/QuickStartTest.php:158
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:703

ERRORS!
Tests: 5, Assertions: 2, Errors: 1, Skipped: 2.
✗	
Unknown

#23 has:

Drupal\Tests\Core\Command\QuickStartTest
exception: [Other] Line 0 of sites/default/files/simpletest/phpunit-105.xml:
PHPUnit Test failed to complete; Error: PHPUnit 9.5.14 by Sebastian Bergmann and contributors.

Warning:       Your XML configuration validates against a deprecated schema.
Suggestion:    Migrate your XML configuration using "--migrate-configuration"!

Testing Drupal\Tests\Core\Command\QuickStartTest
ES...                                                               5 / 5 (100%)

Time: 00:10.072, Memory: 4.00 MB

There was 1 error:

1) Drupal\Tests\Core\Command\QuickStartTest::testQuickStartCommand
Symfony\Component\Process\Exception\ProcessTimedOutException: The process "'/usr/local/bin/php' 'core/scripts/drupal' 'quick-start' 'standard' '--site-name='\''Test site test54067442'\''' '--suppress-login'" exceeded the timeout of 1 seconds.

/var/www/html/vendor/symfony/process/Process.php:1254
/var/www/html/vendor/symfony/process/Process.php:471
/var/www/html/core/tests/Drupal/Tests/Core/Command/QuickStartTest.php:116
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:726

testQuickStartInstallAndServerCommands() doesn't seem to be timing out, though.

Ah, this will prove the timeout works for the last case -- it needs a failing condition.

With a dictionary word.

There we go:

Drupal\Tests\Core\Command\QuickStartTest
exception: [Other] Line 0 of sites/default/files/simpletest/phpunit-105.xml:
PHPUnit Test failed to complete; Error: PHPUnit 9.5.14 by Sebastian Bergmann and contributors.

Warning:       Your XML configuration validates against a deprecated schema.
Suggestion:    Migrate your XML configuration using "--migrate-configuration"!

Testing Drupal\Tests\Core\Command\QuickStartTest
.SE..                                                               5 / 5 (100%)

Time: 00:20.367, Memory: 4.00 MB

There was 1 error:

1) Drupal\Tests\Core\Command\QuickStartTest::testQuickStartInstallAndServerCommands
Symfony\Component\Process\Exception\ProcessTimedOutException: The process "'/usr/local/bin/php' 'core/scripts/drupal' 'server' '--suppress-login'" exceeded the timeout of 1 seconds.

/var/www/html/vendor/symfony/process/Process.php:1254
/var/www/html/vendor/symfony/process/Process.php:471
/var/www/html/core/tests/Drupal/Tests/Core/Command/QuickStartTest.php:211
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:726

This is ready for review again.

Updating the IS with a little more detail.

@neclimdul, not in this case. It's definitely a 100% consistent fail non-failing infinite loop whenever the conditions those three tests are looking for are not met (for example, if the behavior of MINIMUM_SUPPORTED_PHP is changed). See the console output linked in the IS. Edit: Or if it's a race condition, the race is with infinity. ;)

It appears the tests were written with the assumption that built-in Process API for timeouts would terminate the test when needed despite the potentially infinite loops (programming 101...). However, Symfony's docs clearly indicate that the way the test was written before would have required manual checkTimeout() calls. I think @longwave's approach is cleaner though by using the actual API provided, which also works as proved in the test patches.

I think @longwave's approach will also fix #2975644: Random Failure in Drupal\Tests\Core\Command\QuickStartTest FWIW. Different bugs, one fix.

@neclimdul, lol yep, I hacked those 500s timeouts down to 30s and then 1s locally so that it would complete sooner in the failing cases. Sounds like @longwave did too.

The fail in #8 was due to the loop exiting when only part of the output had been sent. I fixed that in 2f7382fab2.

Adding credit for @Spokje, who was the one who figured out that this test was the reason #2917655: [9.4.x only] Drop official PHP 7.3 support in Drupal 9.4 was timing out.