Automatic upgrade to PHPUnit 6 is dangerous

Can't we get rid of PHPUnit 4.8? Can someone describe me why this old, unsupported PHPUnit version is the minimum required version in Drupal 8.5.x still? (I'd be thankful for a link to an issue/discussion about this. I have not found anything.) Because PHP 5.5 support :(

Ultimately the issue here is that we need to eliminate the lockfile for core.

If you do not have control over the production platform that your software is going to run in, you should not ship a lock file.

The vast majority of users who are using composer to build sites are ignoring that lock file anyhow, because they're using scaffold templates like https://github.com/drupal-composer/drupal-project. The only time the lockfile comes into play is if they want the added safety of ensuring they're on a tested version and use https://packagist.org/packages/webflo/drupal-core-strict.

So, what, beyond minimum and maximum dependency testing, do we need to do in order to not ship with a lock file? Anything else? I can prioritize that min/max testing if it would help us get to where there is no lock file.

+++ b/core/scripts/run-tests.sh
@@ -463,6 +465,21 @@ function simpletest_script_init() {
+  // Detect if we're in the top-level process using the private 'execute-test'
+  // argument. Determine if being run on drupal.org's testing infrastructure
+  // using the presence of 'drupaltestbot' in the database url.
+  if (!$args['execute-test'] && preg_match('/drupaltestbot/', $args['dburl'])) {
+    // Update PHPUnit if needed and possible. There is a later check once the
+    // autoloader is in place to ensure we're on the correct version. We need to
+    // do this before the autoloader is in place to ensure that it is correct.
+    $composer = ($composer = rtrim('\\' === DIRECTORY_SEPARATOR ? preg_replace('/[\r\n].*/', '', `where.exe composer.phar`) : `which composer.phar`))
+      ? $php . ' ' . escapeshellarg($composer)
+      : 'composer';
+    passthru("$composer run-script drupal-phpunit-upgrade-check");
+  }

Whats the purpose of this block? i.e. is this really a drupalci feature request?

I'd definitely want to avoid guessing about whether or not we're on the testbot, as even that url has changed before. (also, I dont think drupaltestbot is in the dburl for sqlite)

As we've talked about before core needs a way to reliably run set up commands like this prior to a test run. And we need to be able to ship changes to these commands with different releases.

You have that now, albeit "reliably" is arguable. run-tests.sh is the build script for running core's tests, and the testbots will happily execute anything thats in there. This is definitely the rightest place to put these sorts of things, despite run-tests.sh jankyness. The one caveat is that if its something that would conflict with contrib testing, then it should be configurable, such that it'd only happen for core.

My question above was more of a why are you running the drupal-phpunit-upgrade-check exclusively on the testbot, and not in all environments?

I confirmed that the sqlite url doesnt have the u:p in it: --dburl "sqlite://localhost/sites/default/files/db.sqlite"

If you definitely need a reliable indicator that you're running in a testbot environment, I can definitely provide that directly, much like I added for the nightwatch testing: http://cgit.drupalcode.org/drupalci_testbot/tree/src/DrupalCI/Plugin/Bui...

(I should probably have that env be something like "BUILD_ENV" and use it universally)

This is why CI's like travis and circle CI work so well

The primary difference *there* is the poverty of being in a patch based workflow. Its hard to modify the build in a patch workflow as you almost have to double build, depending on what kinds of requirements changed. But theres nothing stopping us from doing whatever we want to the build, its just that it's always seemed like run-tests.sh was a thing we didnt want to touch.

Make PHPUnit 4.8 the special case. Specify that core needs PHPUnit >=4.8 <7 (allow 4, 5, and 6). Put PHPUnit 6 in the lock file. Look for PHP versions < 7 and say composer update for those.

If we had a defined behavior for composer.json in modules, we'd be able to tell contrib to specify PHPUnit 4.8 if their tests aren't ready. But we can fake it because (IIRC) the testbot merges the facade's composer.json for the project, so it's almost the same. Contrib already has to deal with PHP 7.2+ so those projects should be updating their tests anyway.

I don't think we need to modify run-test.sh or anything, just flip the behavior of the composer script and then wait until we drop PHP <7. Then we just remove the special case from the composer script.

See also: https://github.com/sebastianbergmann/phpunit/releases/tag/7.0.0

Make PHPUnit 4.8 the special case. Specify that core needs PHPUnit >=4.8 <7 (allow 4, 5, and 6). Put PHPUnit 6 in the lock file. Look for PHP versions < 7 and say composer update for those.

If we had a defined behavior for composer.json in modules, we'd be able to tell contrib to specify PHPUnit 4.8 if their tests aren't ready. But we can fake it because (IIRC) the testbot merges the facade's composer.json for the project, so it's almost the same. Contrib already has to deal with PHP 7.2+ so those projects should be updating their tests anyway.

I don't think we need to modify run-test.sh or anything, just flip the behavior of the composer script and then wait until we drop PHP <7. Then we just remove the special case from the composer script.

See also: https://github.com/sebastianbergmann/phpunit/releases/tag/7.0.0

#19 looks viable but I think we should add a @todo and a follow-up to remove it for when we drop PHP 5.5 support.

We definitely should avoid changing the results of composer.lock to avoid surprises. This is a much better way to accomplish this oddly required workaround until we can get a better system in place, and it works with the testbots as is.

LGTM.

Committed/pushed to 8.6.x and cherry-picked to 8.5.x. Thanks!