drud / quicksprint

A basic toolkit to get people started with ddev and a Drupal codebase on macOS, Linux, or WIndows
Other
24 stars 16 forks source link

Version of phpunit is too old #53

Closed torenware closed 6 years ago

torenware commented 6 years ago

The sprint oriented setup uses PHP 7.2 and Drupal 8.6.x-dev, but the version of phpunit bundled with it is only PHPUnit 4.8.36. This version of phpunit will not run in this configuration.

I'm a little behind on changes in how composer builds things currently, but we need to goose this.

torenware commented 6 years ago

I'm looking for documentation as to how composer to force the installation of phpunit ^6.1. Looking at core/composer.json, it looks like there is nothing there that would do that if we are running PHP 7.2.

Do we really need to run PHP 7.2 for the sprint? It appears to be simple to knock the PHP version down to 7.1 (it's a configuration line in sprint/.ddev/config.yaml), which would make the install compatible with phpunit 4.3.

Obviously, if there's a way to coerce ddev to run composer in such a way to get PHP 7.2 and phpunit ^6.1, this would be better. I lack the composer fu needed to make this happen however. But since the ddev installer is a sort of lowest common denominator for sprinters, people who actually need to test against PHP 7.2 can likely manually install phpunit 6 if they needed.

torenware commented 6 years ago

Looks like reducing PHP version will not help here. Is there any way we can configure drud so that it calls:

composer run-script drupal-phpunit-upgrade

This appears to be required if you want to use PHP 7 and phpunit. Which, of course, we do.

rfay commented 6 years ago

When is drupal-phpunit-upgrade available? Where does it come from? just a dev build of drupal8 composer install, or what? We run composer twice, once during the build, and a finish-up during bringing up the project. So a ddev exec composer run-script drupal-phpunit-upgrade would do this in the latter situation (just grep for "ddev exec" to see the other things that are being done that way.

rfay commented 6 years ago

I want to draw your attention to the phantomjs install referenced in https://github.com/drud/ddev/issues/723#issuecomment-374044739 - The last time I tried the script referenced there I had no trouble at all, would be interested to know what is important there and what can be improved.

torenware commented 6 years ago

I did a bit of digging in the source. The script drupal-phpunit-upgrade is defined in core/composer.json, and maps to "@composer update phpunit/phpunit --with-dependencies --no-progress”. The version test is also defined there; the code is in Drupal\Core\Composer\Composer::upgradePHPUnit, and is just a version test of PHP and phpunit.

The error messages are posted here:

drupal8/core/tests/bootstrap.php:163:  $message = "PHPUnit testing framework version 6 or greater is required when running on PHP 7.0 or greater. Run the command 'composer run-script drupal-phpunit-upgrade' in order to fix this.";
drupal8/core/scripts/run-tests.sh:143:  simpletest_script_print_error("PHPUnit testing framework version 6 or greater is required when running

My guess is that the ddev script could invoke this stuff once the vendor directory is populated and phpunit is actually on disk to be upgraded.

On Apr 10, 2018, at 5:28 AM, Randy Fay notifications@github.com wrote:

When is drupal-phpunit-upgrade available? Where does it come from? just a dev build of drupal8 composer install, or what? We run composer twice, once during the build, and a finish-up during bringing up the project. So a ddev exec composer run-script drupal-phpunit-upgrade would do this in the latter situation (just grep for "ddev exec" to see the other things that are being done that way.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/drud/quicksprint/issues/53#issuecomment-380080422, or mute the thread https://github.com/notifications/unsubscribe-auth/AATqZVvRrsL8UiYNMsOWaM4VfCvCuCC5ks5tnKVUgaJpZM4TNoBg.

torenware commented 6 years ago

I’ll give it a look.

On Apr 10, 2018, at 8:39 AM, Randy Fay notifications@github.com wrote:

I want to draw your attention to the phantomjs install referenced in drud/ddev#723 (comment) https://github.com/drud/ddev/issues/723#issuecomment-374044739 - The last time I tried the script referenced there I had no trouble at all, would be interested to know what is important there and what can be improved.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/drud/quicksprint/issues/53#issuecomment-380147094, or mute the thread https://github.com/notifications/unsubscribe-auth/AATqZUQMDsR-4VJ4LMWU_3Mw0U6TrBM5ks5tnNJLgaJpZM4TNoBg.

rfay commented 6 years ago

So nothing like drupal-phpunit-upgrade exists anywhere in the codebase (d8 with a composer install). However, it's possible to do the composer update phpunit/phpunit --with-dependencies --no-progress But it changes loads and loads of things, by major version changes. - Updating phpunit/phpunit (4.8.36 => 6.5.8): Downloading (100%)

I guess it's important to figure out the authoritative answer to this - I would never recommend to anybody at a sprint to do more than a composer install because that's what D8 should be running with right?

There's no reason I know of that we can't use php7.0 or php7.1 everywhere if that's the issue?

mradcliffe commented 6 years ago

8.6.x has the following in composer.json, @rfay

    "scripts": {
        "pre-autoload-dump": "Drupal\\Core\\Composer\\Composer::preAutoloadDump",
        "post-autoload-dump": "Drupal\\Core\\Composer\\Composer::ensureHtaccess",
        "post-package-install": "Drupal\\Core\\Composer\\Composer::vendorTestCodeCleanup",
        "post-package-update": "Drupal\\Core\\Composer\\Composer::vendorTestCodeCleanup",
        "drupal-phpunit-upgrade-check": "Drupal\\Core\\Composer\\Composer::upgradePHPUnit",
        "drupal-phpunit-upgrade": "@composer update phpunit/phpunit --with-dependencies --no-progress",
        "phpcs": "phpcs --standard=core/phpcs.xml.dist --runtime-set installed_paths $($COMPOSER_BINARY config vendor-dir)/drupal/coder/coder_sniffer --",
        "phpcbf": "phpcbf --standard=core/phpcs.xml.dist --runtime-set installed_paths $($COMPOSER_BINARY config vendor-dir)/drupal/coder/coder_sniffer --"
    },
rfay commented 6 years ago

I'm fine with composer.json having that... but that's not what we have on a composer install:

rfay@rfay-mbp-15:~/Sites/sprint/sprint-20180410-0953/drupal8/vendor/bin$ \ls
phpcbf      phpcs       phpunit     simple-phpunit
mradcliffe commented 6 years ago

Right, it's not a default. It requires that extra composer drupal-phpunit-upgrade command.

torenware commented 6 years ago

On Apr 10, 2018, at 1:09 PM, Randy Fay notifications@github.com wrote:

So nothing like drupal-phpunit-upgrade exists anywhere in the codebase

Yeah, it’s just an alias for a native composer command. The version test probably could be done w/o reference to the Drupal source, although I don’t see why we’d need to do that.

(d8 with a composer install). However, it's possible to do the composer update phpunit/phpunit --with-dependencies --no-progress

Certainly. But it changes loads and loads of things, by major version changes. - Updating phpunit/phpunit (4.8.36 => 6.5.8): Downloading (100%)

I guess it's important to figure out the authoritative answer to this - I would never recommend to anybody at a sprint to do more than a composer install because that's what D8 should be running with right?

Although any developer who’s using PHP 7+ and doing testing needs to have PHPUnit v. 6 available, and any dependencies tied to v. 6 would also be installed on those systems. PHP 5.x is still (theoretically) supported in 8.6.x (it’s out in 8.7.x), but I don’t think we want much of anyone at a sprint running it, unless it’s a PHP 5.6 compatibility issue they’re working on.

I’m not sure there’s a way to get composer to simply install THE RIGHT DAMN VERSION OF PHPUNIT based upon the running PHP version; I’m guessing the current conditions in core/composer.json are the best that can done w/o a really fugly hack.

Are you saying that we’d prefer not to have the ddev clean setup script running drupal-phpunit-upgrade (say, due to increased traffic on limited internet access at a sprint), or that you want to avoid running the command during the sprint to reduce the burden on new sprinters?

There's no reason I know of that we can't use php7.0 or php7.1 everywhere if that's the issue?

Damned if I know. And I haven’t done much with PhantomJS either, since I don’t test much Drupal-core JS dependent code.

Rob

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/drud/quicksprint/issues/53#issuecomment-380230456, or mute the thread https://github.com/notifications/unsubscribe-auth/AATqZVBlR3yaSDPtViPRWuBeGqAiL7NPks5tnRFggaJpZM4TNoBg.

rfay commented 6 years ago

I think the right answer to this: If people are going to run phpunit, they need to run the command. They would just do ddev exec composer update phpunit/phpunit --with-dependencies --no-progress - Or optionally they could ddev ssh and do it.

torenware commented 6 years ago

Don’t think so. That command is simply an alias to a command from phpunit itself. So composer will find the needed command based upon phpunit being in vendor/, w/o any need to reference drupal. We just run

composer update phpunit/phpunit --with-dependencies --no-progress

directly.

On Apr 10, 2018, at 1:19 PM, Matthew Radcliffe notifications@github.com wrote:

Right, it's not a default. It requires that extra composer drupal-phpunit-upgrade command.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/drud/quicksprint/issues/53#issuecomment-380233396, or mute the thread https://github.com/notifications/unsubscribe-auth/AATqZSAu3_QyXZ1HhZoxizA3xs3JZuqoks5tnRPZgaJpZM4TNoBg.

torenware commented 6 years ago

This is going to make the Ghost of Werner Heisenberg weep.

The perversity here is that nothing gets into core w/o tests passing, and the modifications made by upgrading phpunit are in the version of Drupal that pass those tests.

It’s also true that the version of Drupal that is deployed in the wild mostly does not have those modification.

I’m not suggesting we must necessarily preinstall phpunit 6.5. But it’s also worth remembering that the uncertainty here is baked into our testing system :-)

On Apr 10, 2018, at 1:31 PM, Randy Fay notifications@github.com wrote:

I think the right answer to this: If people are going to run phpunit, they need to run the command. They would just do ddev exec composer update phpunit/phpunit --with-dependencies --no-progress - Or optionally they could ddev ssh and do it.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/drud/quicksprint/issues/53#issuecomment-380236659, or mute the thread https://github.com/notifications/unsubscribe-auth/AATqZXpcGEGZLGcTGrUTOwV89vMjsigYks5tnRaLgaJpZM4TNoBg.

mradcliffe commented 6 years ago

The correct version of phpunit will be installed on whatever system you run composer install on so if we run composer install/drupal-phpunit-upgrade on a code base outside of the container, then most likely MacOS' php 5.6 will run and we'll get phpunit 4.8. If we run composer install/drupal-phpunit-upgrade manually in the IDE container, for instance, then it should update to phpunit 6 because it's running php 7.2.

rfay commented 6 years ago

OK, I'll take a look at that. Should be easy enough. We'll have to either require a php 7.2 on the build system (for the composer install) or build in a container. Probably we should build in a container.

torenware commented 6 years ago

I do not believe this to be correct. I’m doing this on a Mac with php version 7.1.4, and it still installs phpunit 4.8. I don’t think it’s a characteristic of the running copy of composer. I suspect it’s a result of how the dependency is defined:

"phpunit/phpunit": "^4.8.35 || ^6.1”

The problem is that phpunit 4.8.6 has the dependency:

"php": ">=5.3.3”

and phpunit 6.3 has the dependency

"php": "^7.0”

I have the suspicion that we’re specifying the dependency wrong in core/composer.json. If we want to enforce a version of phpunit that’s above 6.1 when running php ^7.0, we likely really want to specify this as:

"phpunit/phpunit": “^6.1 || ^4.8.35”

This will check the requirement for php ^7.0, and fail over to ^4.8.35 if we’re running PHP 5.

Rob

On Apr 10, 2018, at 6:16 PM, Matthew Radcliffe notifications@github.com wrote:

The correct version of phpunit will be installed on whatever system you run composer install on so if we run composer install/drupal-phpunit-upgrade on a code base outside of the container, then most likely MacOS' php 5.6 will run and we'll get phpunit 4.8. If we run composer install/drupal-phpunit-upgrade manually in the IDE container, for instance, then it should update to phpunit 6 because it's running php 7.2.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/drud/quicksprint/issues/53#issuecomment-380295415, or mute the thread https://github.com/notifications/unsubscribe-auth/AATqZemFVSfmkQnm1-E6HZi3uW318vGXks5tnVligaJpZM4TNoBg.

rfay commented 6 years ago

It's not just a matter of running php7.2, that's my result as well

I tried this with the official composer container, latest version (php 7.2):

docker run --rm --interactive --tty --volume $PWD:/app --volume $SSH_AUTH_SOCK:/ssh-auth.sock --volume /etc/passwd:/etc/passwd:ro --volume /etc/group:/etc/group:ro --user $(id -u):$(id -g) --env SSH_AUTH_SOCK=/ssh-auth.sock composer:1.6.3 install --ignore-platform-reqs --no-scripts

You end up with phpunit 4.6.

torenware commented 6 years ago

I've confirmed that my conditions hack does NOT work. You still get phpunit 4.x. Looks like running the upgrader is the only thing that works.

rfay commented 6 years ago

The d8 core issue seems to be https://www.drupal.org/project/drupal/issues/2932606

rfay commented 6 years ago

None of this seems to have had to do with quicksprint, so closing.