Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#46815 closed task (blessed) (fixed)

Add PHPUnit setup to composer.json

Reported by: welcher's profile welcher Owned by: ocean90's profile ocean90
Milestone: 5.5 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch has-dev-note
Focuses: Cc:

Description

Setting up the correct version of PHPUnit to run tests locally can be difficult and confusing. It is also different depending on what is being used as the web server ( MAMP, VVV, etc ). This can be simplified to some extent by leveraging composer to install the correct, compatible PHPUnit version and provide a script to run the tests.

The attached patch adds PHPunit as a dev dependency and introduces a unit-tests composer command.

Attachments (6)

46815.diff (53.9 KB) - added by welcher 5 years ago.
Initial patch
46815.2.patch (9.8 KB) - added by ayeshrajans 5 years ago.
46815.2.diff (54.0 KB) - added by welcher 5 years ago.
46815.3.diff (56.9 KB) - added by welcher 5 years ago.
Refreshed patch
46815.4.diff (839 bytes) - added by welcher 5 years ago.
46815.5.diff (678 bytes) - added by welcher 5 years ago.

Download all attachments as: .zip

Change History (36)

@welcher
5 years ago

Initial patch

#1 @johnbillion
5 years ago

I'd love to do this, but the problem it introduces is that the CI tests on older versions of PHP won't be able to install PHPUnit 7 as the PHP requirements won't be met.

It might be possible to use the config.platform.php setting in composer.json to set the PHP version to 7.3, and then allow the existing PHPUnit installation in .travis.yml to do its thing.

Last edited 5 years ago by johnbillion (previous) (diff)

#3 @welcher
5 years ago

@johnbillion as per our slack chat. It looks like composer install is only being run for 7.2 which is compatible with PHPUnit 7. Are their any other concerns?

#5 @johnbillion
5 years ago

We'll need to consider the impact on projects that specify WordPress as a dependency via one of the unofficial packages, eg johnpbloch/wordpress, roots/wordpress, or wordplate/wordplate, but use a version of PHPUnit less than 7.

CCing a few people for their feedback: @rarst @austinpray @johnpbloch @vinkla

#6 @johnbillion
5 years ago

"phpunit/phpunit": "<8" could work.

#7 @vinkla
5 years ago

Specify version ^5.0 || ^6.0 || ^7.0 || ^8.0 for phpunit/phpunit and Composer will figure out which version to use.

Last edited 5 years ago by vinkla (previous) (diff)

#8 @Rarst
5 years ago

require-dev is root–only, it’s not processed for dependencies so it can't conflict.

#9 @johnbillion
5 years ago

  • Milestone changed from Awaiting Review to 5.3

#10 follow-up: @ayeshrajans
5 years ago

I always wondered why we don't use the composer.json file to install PHPUnit, but have a complex switch/case matrix in .travis.yml with composer install phpunit/phpunit calls.

  • Because we use different phpunit versions for each PHP version, we cannot use the same composer.lock file. When composer install is run, composer will install the exact list of packages regardless of the platform PHP version. As a solution, we can remove shipping our composer.lock file. PHPUnit promptly follows server, and we shouldn't be afraid of minor updates.
  • With the .travis.yml, PHPUnit is installed globally. If we were to regular composer require-dev, we will now need to update all PHPUnit invocations to use the vendor/bin/phpunit path because IIRC, Travis images already have PHPUnit installed.
  • This will require us to update the Gruntfile.js to use the new vendor/bin/phpunit executable instead of the global one.

I understand why whoever decided to use composer install in the Travis to install phpunit, but with WordPress dropping PHP 5.2 (so we will always have composer)` I think it's a good time to finally clean up and fix.

#11 @ayeshrajans
5 years ago

I have made a more elaborate list of changes (https://github.com/Ayesh/wordpress-develop/commits/composer-phpunit), and tests pass (https://travis-ci.org/Ayesh/wordpress-develop/builds/516626733 , save for some formatting issues in a build job).

@ayeshrajans
5 years ago

#12 follow-up: @vinkla
5 years ago

Out of curiosity; is there any reason why PHPUnit 8.0 is excluded?

#13 in reply to: ↑ 12 @ayeshrajans
5 years ago

Replying to vinkla:

Out of curiosity; is there any reason why PHPUnit 8.0 is excluded?

PHPUnit 8 requires PHP versions 7.2 or higher. Class methid signatures are not compatible with the tests we currently have (lack of void return types for example), so we can't use PHPUnit 8 yet.

#14 @welcher
5 years ago

@ayeshrajans thanks for the thoughtful and thorough patch.

I am wondering if we should split it out into a separate ticket that is focused on the Travis changes/conversation so we don't risk turning this ticket into an epic.

@johnbillion any objections?

@welcher
5 years ago

#15 @welcher
5 years ago

@johnbillion I've uploaded a new patch refreshed against master and using the approach @vinkla mentioned

My Travis tests are passing: https://travis-ci.org/ryanwelcher/wordpress-develop/builds/525030607

Is there anything further I can add or test?

#16 in reply to: ↑ 10 @netweb
5 years ago

Replying to ayeshrajans:

... As a solution, we can remove shipping our composer.lock file.

A ticket for this has now been created:

Related: #47381 - Remove the Composer lock file from version control

@welcher
5 years ago

Refreshed patch

#17 follow-up: @welcher
5 years ago

@johnbillion @netweb patch refreshed. Hoping this can still get in 5.3

#18 in reply to: ↑ 17 @netweb
5 years ago

  • Keywords needs-refresh added

Replying to welcher:

@johnbillion @netweb patch refreshed. Hoping this can still get in 5.3

Thanks @welcher, your patch is a little off, it has a couple of of changes relating to WPCS, you've changed it from 2.1.0 to 2.0.0, and added it as a script, I suspect this is because of recent commits, would you mind refreshing 46815.3.diff please.

@welcher
5 years ago

@welcher
5 years ago

#19 @welcher
5 years ago

  • Keywords needs-refresh removed

@netweb thanks for the feedback. I must have resolved merge conflicts badly at some point. My latest patch looks correct to me.

This ticket was mentioned in Slack in #core by welcher. View the logs.


5 years ago

#21 @johnbillion
5 years ago

  • Keywords dev-feedback removed
  • Owner set to johnbillion
  • Status changed from new to reviewing
  • Type changed from enhancement to task (blessed)

#22 @desrosj
5 years ago

  • Milestone changed from 5.3 to Future Release

I'm punting this to Future Release. When this is ready, it can be moved into a numbered milestone.

#23 @johnbillion
5 years ago

  • Milestone changed from Future Release to 5.4

#24 @ayeshrajans
5 years ago

Also see #47767. PHPUnit setup is currently done at docker level unless I'm reading that ticket wrong.

#25 @johnbillion
5 years ago

  • Milestone changed from 5.4 to Future Release

I don't have time to look at this before beta so let's try again in 5.5 early.

#26 @ocean90
4 years ago

  • Milestone changed from Future Release to 5.5
  • Owner changed from johnbillion to ocean90
  • Status changed from reviewing to accepted

Let's give 46815.5.diff a try.

#28 @ocean90
4 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 47881:

Build/Test Tools: Add PHPUnit to Composer dev dependencies.

This helps setting up the correct version of PHPUnit to run WordPress core tests locally and for using it as source for code autocompletion in tests. Also introduces a new Composer script test to run the PHPUnit tests.

Props welcher, ayeshrajans, vinkla, johnbillion, Rarst, netweb, ocean90.
Fixes #46815.

#29 @desrosj
4 years ago

  • Keywords needs-dev-note added

Going to include this in a Build/Test Tools & General PHP improvements dev note.

#30 @desrosj
4 years ago

  • Keywords has-dev-note added; needs-dev-note removed
Note: See TracTickets for help on using tickets.