Opened 3 years ago
Closed 3 years ago
#53945 closed task (blessed) (fixed)
Local test workflow changes for installing Composer dependencies
Reported by: | hellofromTonya | Owned by: | johnbillion |
---|---|---|---|
Milestone: | 5.9 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Build/Test Tools | Keywords: | has-patch has-testing-instructions |
Focuses: | Cc: |
Description (last modified by )
With the adoption of PHPUnit Polyfills (see ticket #46149 and changeset [51559]), its dependencies are installed via and tests run using Composer (see ticket #46149 and changeset [51545]).
For the CI, this makes sense. What about the instead build the Composer extra steps `npm` / Docker workflow when testing locally?
The Problem
The npm workflow for running PHPUnit tests locally:
npm install npm run build:dev npm run env:start npm run env:install npm run test:php
- This works on the WP 5.8 branch ✅
- Does not work/run on trunk/master branch ❌
Error: The PHPUnit Polyfills library is a requirement for running the WP test suite. You need to run `composer update` before running the tests. Once the dependencies are installed, you can run the tests using the Composer-installed version of PHPUnit or using a PHPUnit phar file, but the dependencies do need to be installed whichever way the tests are run.
Extra steps currently needed are:
- Step 1: Install and setup Composer locally
- Step 2: Install dependencies:
composer install npm install npm run build:dev npm run env:start npm run env:install npm run test:php
These extra steps increase the complexity, barrier, and effort for contributors.
Proposal
This ticket proposes to move the extra steps into the Docker images as part of the existing npm workflow. In other words, build it into the tooling.
Why?
To avoid passing these steps on to contributors. With this change, contributors can continue using the same npm commands to locally run tests.
What about alternative workflows?
Composer is and has been an alternative workflow. More advanced testing scenarios may need it to enable/disable different PHP extensions. For contributors who wish to use Composer locally, this alternative workflow is available for them.
For those contributors who do not wish to use Composer or npm/Docker, they can manually install PHPUnit, PHPUnit Polyfills, etc. and use the WP_TESTS_PHPUNIT_POLYFILLS_PATH
constant to specify where the polyfills exist on their machine.
Change History (43)
#2
@
3 years ago
- Keywords needs-patch added
- Milestone changed from Awaiting Review to 5.9
- Version set to trunk
Hehe, not sure "Proposal" is the right word here, it should be "accepted" by default. This is a part of making the WP tests depend on third-party software. If the Polyfills are installed globally in the Docker image (hopefully with only the needed version of PHPUnit as the PHP version is "fixed" there), testing will work again as it used to.
This is by far the easiest/most user friendly way to handle the changes in PHP testing.
#3
@
3 years ago
I'm not able to get this working on either the /trunk
or /branches/5.8
branches
It seems to hang when running the npm script npm run env:install
I've added the wp_cli_info()
script here only for debugging so that an additional script is run after install_composer_dependancies()
as this was where the scripts appeared to be hung
-
tools/local-env/scripts/install.js
22 22 23 23 install_wp_importer(); 24 24 25 install_composer_dependancies(); 26 27 wp_cli_info(); 28 25 29 // Read in wp-tests-config-sample.php, edit it to work with our config, then write it to wp-tests-config.php. 26 30 const testConfig = readFileSync( 'wp-tests-config-sample.php', 'utf8' ) 27 31 .replace( 'youremptytestdbnamehere', 'wordpress_develop_tests' ) … … 57 61 execSync( `docker-compose exec -T php rm -rf ${testPluginDirectory}`, { stdio: 'inherit' } ); 58 62 execSync( `docker-compose exec -T php git clone https://github.com/WordPress/wordpress-importer.git ${testPluginDirectory} --depth=1`, { stdio: 'inherit' } ); 59 63 } 64 65 /** 66 * Installs the Composer package dependancies. 67 */ 68 function install_composer_dependancies() { 69 execSync( `docker-compose exec -T php composer install`, { stdio: 'inherit' } ); 70 } 71 72 /** 73 * Get the environment information. 74 */ 75 function wp_cli_info() { 76 wp_cli( `cli info` ); 77 78 }
With the above patch, composer install
is run and the yoast/phpunit-polyfills
package is installed in the PHP Docker container:
No composer.lock file present. Updating dependencies to latest instead of installing from lock file. See https://getcomposer.org/install for more information. Loading composer repositories with package information Updating dependencies Lock file operations: 41 installs, 0 updates, 0 removals - Locking dealerdirect/phpcodesniffer-composer-installer (v0.7.1) - Locking doctrine/instantiator (1.4.0) - Locking myclabs/deep-copy (1.10.2) - Locking nikic/php-parser (v4.12.0) - Locking phar-io/manifest (2.0.3) - Locking phar-io/version (3.1.0) - Locking phpcompatibility/php-compatibility (9.3.5) - Locking phpcompatibility/phpcompatibility-paragonie (1.3.1) - Locking phpcompatibility/phpcompatibility-wp (2.1.2) - Locking phpdocumentor/reflection-common (2.2.0) - Locking phpdocumentor/reflection-docblock (5.2.2) - Locking phpdocumentor/type-resolver (1.4.0) - Locking phpspec/prophecy (1.13.0) - Locking phpunit/php-code-coverage (9.2.6) - Locking phpunit/php-file-iterator (3.0.5) - Locking phpunit/php-invoker (3.1.1) - Locking phpunit/php-text-template (2.0.4) - Locking phpunit/php-timer (5.0.3) - Locking phpunit/phpunit (9.5.8) - Locking sebastian/cli-parser (1.0.1) - Locking sebastian/code-unit (1.0.8) - Locking sebastian/code-unit-reverse-lookup (2.0.3) - Locking sebastian/comparator (4.0.6) - Locking sebastian/complexity (2.0.2) - Locking sebastian/diff (4.0.4) - Locking sebastian/environment (5.1.3) - Locking sebastian/exporter (4.0.3) - Locking sebastian/global-state (5.0.3) - Locking sebastian/lines-of-code (1.0.3) - Locking sebastian/object-enumerator (4.0.4) - Locking sebastian/object-reflector (2.0.4) - Locking sebastian/recursion-context (4.0.4) - Locking sebastian/resource-operations (3.0.3) - Locking sebastian/type (2.3.4) - Locking sebastian/version (3.0.2) - Locking squizlabs/php_codesniffer (3.6.0) - Locking symfony/polyfill-ctype (v1.23.0) - Locking theseer/tokenizer (1.2.1) - Locking webmozart/assert (1.10.0) - Locking wp-coding-standards/wpcs (2.3.0) - Locking yoast/phpunit-polyfills (1.0.1) Writing lock file Installing dependencies from lock file (including require-dev) Package operations: 41 installs, 0 updates, 0 removals - Downloading squizlabs/php_codesniffer (3.6.0) - Downloading dealerdirect/phpcodesniffer-composer-installer (v0.7.1) - Downloading myclabs/deep-copy (1.10.2) - Downloading phar-io/version (3.1.0) - Downloading phar-io/manifest (2.0.3) - Downloading phpcompatibility/php-compatibility (9.3.5) - Downloading phpcompatibility/phpcompatibility-paragonie (1.3.1) - Downloading phpcompatibility/phpcompatibility-wp (2.1.2) - Downloading phpdocumentor/reflection-common (2.2.0) - Downloading phpdocumentor/type-resolver (1.4.0) - Downloading sebastian/recursion-context (4.0.4) - Downloading sebastian/exporter (4.0.3) - Downloading sebastian/diff (4.0.4) - Downloading sebastian/comparator (4.0.6) - Downloading symfony/polyfill-ctype (v1.23.0) - Downloading webmozart/assert (1.10.0) - Downloading phpdocumentor/reflection-docblock (5.2.2) - Downloading doctrine/instantiator (1.4.0) - Downloading phpspec/prophecy (1.13.0) - Downloading theseer/tokenizer (1.2.1) - Downloading sebastian/version (3.0.2) - Downloading nikic/php-parser (v4.12.0) - Downloading sebastian/lines-of-code (1.0.3) - Downloading sebastian/environment (5.1.3) - Downloading sebastian/complexity (2.0.2) - Downloading sebastian/code-unit-reverse-lookup (2.0.3) - Downloading phpunit/php-text-template (2.0.4) - Downloading phpunit/php-file-iterator (3.0.5) - Downloading phpunit/php-code-coverage (9.2.6) - Downloading phpunit/php-invoker (3.1.1) - Downloading phpunit/php-timer (5.0.3) - Downloading sebastian/cli-parser (1.0.1) - Downloading sebastian/code-unit (1.0.8) - Downloading sebastian/object-reflector (2.0.4) - Downloading sebastian/global-state (5.0.3) - Downloading sebastian/object-enumerator (4.0.4) - Downloading sebastian/resource-operations (3.0.3) - Downloading sebastian/type (2.3.4) - Downloading wp-coding-standards/wpcs (2.3.0) - Downloading phpunit/phpunit (9.5.8) - Downloading yoast/phpunit-polyfills (1.0.1) 0/41 [>---------------------------] 0% 8/41 [=====>----------------------] 19% 9/41 [======>---------------------] 21% 10/41 [======>---------------------] 24% 15/41 [==========>-----------------] 36% 18/41 [============>---------------] 43% 19/41 [============>---------------] 46% 20/41 [=============>--------------] 48% 25/41 [=================>----------] 60% 27/41 [==================>---------] 65% 29/41 [===================>--------] 70% 34/41 [=======================>----] 82% 38/41 [=========================>--] 92% 39/41 [==========================>-] 95% 40/41 [===========================>] 97% 41/41 [============================] 100% - Installing squizlabs/php_codesniffer (3.6.0): Extracting archive - Installing dealerdirect/phpcodesniffer-composer-installer (v0.7.1): Extracting archive - Installing myclabs/deep-copy (1.10.2): Extracting archive - Installing phar-io/version (3.1.0): Extracting archive - Installing phar-io/manifest (2.0.3): Extracting archive - Installing phpcompatibility/php-compatibility (9.3.5): Extracting archive - Installing phpcompatibility/phpcompatibility-paragonie (1.3.1): Extracting archive - Installing phpcompatibility/phpcompatibility-wp (2.1.2): Extracting archive - Installing phpdocumentor/reflection-common (2.2.0): Extracting archive - Installing phpdocumentor/type-resolver (1.4.0): Extracting archive - Installing sebastian/recursion-context (4.0.4): Extracting archive - Installing sebastian/exporter (4.0.3): Extracting archive - Installing sebastian/diff (4.0.4): Extracting archive - Installing sebastian/comparator (4.0.6): Extracting archive - Installing symfony/polyfill-ctype (v1.23.0): Extracting archive - Installing webmozart/assert (1.10.0): Extracting archive - Installing phpdocumentor/reflection-docblock (5.2.2): Extracting archive - Installing doctrine/instantiator (1.4.0): Extracting archive - Installing phpspec/prophecy (1.13.0): Extracting archive - Installing theseer/tokenizer (1.2.1): Extracting archive - Installing sebastian/version (3.0.2): Extracting archive - Installing nikic/php-parser (v4.12.0): Extracting archive - Installing sebastian/lines-of-code (1.0.3): Extracting archive - Installing sebastian/environment (5.1.3): Extracting archive - Installing sebastian/complexity (2.0.2): Extracting archive - Installing sebastian/code-unit-reverse-lookup (2.0.3): Extracting archive - Installing phpunit/php-text-template (2.0.4): Extracting archive - Installing phpunit/php-file-iterator (3.0.5): Extracting archive - Installing phpunit/php-code-coverage (9.2.6): Extracting archive - Installing phpunit/php-invoker (3.1.1): Extracting archive - Installing phpunit/php-timer (5.0.3): Extracting archive - Installing sebastian/cli-parser (1.0.1): Extracting archive - Installing sebastian/code-unit (1.0.8): Extracting archive - Installing sebastian/object-reflector (2.0.4): Extracting archive - Installing sebastian/global-state (5.0.3): Extracting archive - Installing sebastian/object-enumerator (4.0.4): Extracting archive - Installing sebastian/resource-operations (3.0.3): Extracting archive - Installing sebastian/type (2.3.4): Extracting archive - Installing wp-coding-standards/wpcs (2.3.0): Extracting archive - Installing phpunit/phpunit (9.5.8): Extracting archive - Installing yoast/phpunit-polyfills (1.0.1): Extracting archive 0/39 [>---------------------------] 0% 9/39 [======>---------------------] 23% 10/39 [=======>--------------------] 25% 17/39 [============>---------------] 43% 19/39 [=============>--------------] 48% 25/39 [=================>----------] 64% 29/39 [====================>-------] 74% 35/39 [=========================>--] 89% 37/39 [==========================>-] 94% 38/39 [===========================>] 97% 39/39 [============================] 100% 9 package suggestions were added by new dependencies, use `composer suggest` to see details. Generating autoload files 26 packages you are using are looking for funding. Use the `composer fund` command to find out more!
This is as far as I can get, but I'd be interested to here any feedback from others who are able to run this in either the /trunk
or /branches/5.8
branches
This ticket was mentioned in Slack in #core-test by netweb. View the logs.
3 years ago
This ticket was mentioned in Slack in #core-test by hellofromtonya. View the logs.
3 years ago
This ticket was mentioned in Slack in #core-test by hellofromtonya. View the logs.
3 years ago
This ticket was mentioned in Slack in #core-php by hellofromtonya. View the logs.
3 years ago
#8
@
3 years ago
During a live mob working session with @netweb @johnbillion @desrosj @jrf and me, achieved consensus on how to make the Composer install invisible to contributors when doing local npm/Docker testing.
- In the
install.js
file, adding the following function and command:/** * Installs the Composer package dependencies within the Docker environment. */ function install_composer_dependencies() { execSync( `docker-compose run -T php composer update -W`, { stdio: 'inherit' } ); }
- What about if someone deletes the
vendor
folder and/orcomposer.lock
file? Add a cascade command with thetest:php
andtest:php-composer
commands to first runcomposer update
. Johnathan proposed topackage.json
"scripts"
:"test:php-composer": "node ./tools/local-env/scripts/docker.js run -rm composer update -W && node ./tools/local-env/scripts/docker.js run --rm phpunit php ./vendor/bin/phpunit",
This ticket was mentioned in PR #1627 on WordPress/wordpress-develop by hellofromtonya.
3 years ago
#9
- Keywords has-patch added; needs-patch removed
As agreed to during a live mob working session, this PR achieves the following:
- Installs the Composer package dependencies within the Docker environment as part of the installation process.
- Ensures these deps are installed when running
npm run test:php
ornpm run test:php-composer
, i.e. runscomposer update -W
to guard for scenario where contributor removes thevendor
and/orcomposer.lock
file.
## Notes
For cascading composer update
command within the npm run test:php-composer
and npm run test:php
script commands, the proposed run -rm composer update -W
worked but outputs the following messages in the command-line:
Run a one-off command on a service. For example: $ docker-compose run web python manage.py shell By default, linked services will be started, unless they are already running. If you do not want to start linked services, use `docker-compose run --no-deps SERVICE COMMAND [ARGS...]`. Usage: run [options] [-v VOLUME...] [-p PORT...] [-e KEY=VAL...] [-l KEY=VALUE...] [--] SERVICE [COMMAND] [ARGS...]
As this message could confuse contributors, this PR switches to using the same command as in the install.js
script:
run -T php composer update -W
Using this command also works but without the above Run a one-off command on a service.
notification.
## Testing Scenarios
Are available in the Trac ticket.
Trac ticket: https://core.trac.wordpress.org/ticket/53945
#10
@
3 years ago
- Keywords has-testing-instructions added
Testing Scenarios
In these examples:
- the
@group formatting
is used to run a smaller sample of tests (i.e. to speed up testing):
npm run test:php -- --group formatting
or
npm run test:php-composer -- --group formatting
- the
...
(in the command-line output example)s means more packages areLocking
orDownloading
. Using this notation for brevity.
Scenario 1: Docker not running (or container destroyed) and npm and composer packages not installed.
Assumes Docker is installed locally.
Expectations:
- Composer package dependencies are installed
composer.lock
file is created- Tests run
Instructions:
Contributor types the following commands into their command-line tool:
npm install npm run build:dev npm run env:start npm run env:install
When running npm run env:install
, the expected command-line output (or similar):
Creating wordpress-develop_cli_run ... done Success: Generated 'wp-config.php' file. Creating wordpress-develop_cli_run ... done Success: Added the constant 'WP_DEBUG' to the 'wp-config.php' file with the raw value 'true'. Creating wordpress-develop_cli_run ... done Success: Added the constant 'WP_DEBUG_LOG' to the 'wp-config.php' file with the raw value 'true'. Creating wordpress-develop_cli_run ... done Success: Added the constant 'WP_DEBUG_DISPLAY' to the 'wp-config.php' file with the raw value 'true'. Creating wordpress-develop_cli_run ... done Success: Added the constant 'SCRIPT_DEBUG' to the 'wp-config.php' file with the raw value 'true'. Creating wordpress-develop_cli_run ... done Success: Added the constant 'WP_ENVIRONMENT_TYPE' to the 'wp-config.php' file with the value 'local'. Cloning into 'tests/phpunit/data/plugins/wordpress-importer'... Cloning into 'tests/phpunit/data/plugins/wordpress-importer'... Creating wordpress-develop_php_run ... done Loading composer repositories with package information Updating dependencies Lock file operations: 41 installs, 0 updates, 0 removals - Locking dealerdirect/phpcodesniffer-composer-installer (v0.7.1) ... - Locking yoast/phpunit-polyfills (1.0.1) Writing lock file Installing dependencies from lock file (including require-dev) Package operations: 41 installs, 0 updates, 0 removals - Downloading squizlabs/php_codesniffer (3.6.0) ... - Downloading phpunit/phpunit (9.5.8) - Downloading yoast/phpunit-polyfills (1.0.1) 0/41 [>---------------------------] 0% 3/41 [==>-------------------------] 7% 8/41 [=====>----------------------] 19% 11/41 [=======>--------------------] 26% 16/41 [==========>-----------------] 39% 19/41 [============>---------------] 46% 22/41 [===============>------------] 53% 27/41 [==================>---------] 65% 31/41 [=====================>------] 75% 35/41 [=======================>----] 85% 39/41 [==========================>-] 95% 40/41 [===========================>] 97% 41/41 [============================] 100% - Installing squizlabs/php_codesniffer (3.6.0): Extracting archive ... - Installing phpunit/phpunit (9.5.8): Extracting archive - Installing yoast/phpunit-polyfills (1.0.1): Extracting archive 0/39 [>---------------------------] 0% 6/39 [====>-----------------------] 15% 9/39 [======>---------------------] 23% 11/39 [=======>--------------------] 28% 14/39 [==========>-----------------] 35% 17/39 [============>---------------] 43% 20/39 [==============>-------------] 51% 23/39 [================>-----------] 58% 26/39 [==================>---------] 66% 29/39 [====================>-------] 74% 31/39 [======================>-----] 79% 34/39 [========================>---] 87% 36/39 [=========================>--] 92% 37/39 [==========================>-] 94% 38/39 [===========================>] 97% 39/39 [============================] 100% 9 package suggestions were added by new dependencies, use `composer suggest` to see details. Generating autoload files 26 packages you are using are looking for funding. Use the `composer fund` command to find out more! PHP CodeSniffer Config installed_paths set to ../../phpcompatibility/php-compatibility,../../phpcompatibility/phpcompatibility-paragonie,../../phpcompatibility/phpcompatibility-wp,../../wp-coding-standards/wpcs Creating wordpress-develop_cli_run ... done Success: Database reset. Creating wordpress-develop_cli_run ... done Success: WordPress installed successfully.
Once completed, the contributor runs the tests:
npm run test:php -- --group formatting
or
npm run test:php-composer -- --group formatting
Scenario 2: composer.lock
file is deleted
The contributor has already run the above commands to setup the local testing environment but then deletes the composer.lock
file.
Expectations:
composer.lock
file is created- Tests run
Instructions:
Notes for testers: Run scenario 1 first. Then delete the composer.lock
file.
Contributor types the following in their command-line tool:
npm run test:php -- --group formatting
or
npm run test:php-composer -- --group formatting
Expected command-line output (or similar):
Creating wordpress-develop_php_run ... done Loading composer repositories with package information Updating dependencies Lock file operations: 41 installs, 0 updates, 0 removals - Locking dealerdirect/phpcodesniffer-composer-installer (v0.7.1) ... - Locking yoast/phpunit-polyfills (1.0.1) Writing lock file Installing dependencies from lock file (including require-dev) Nothing to install, update or remove 9 package suggestions were added by new dependencies, use `composer suggest` to see details. Generating autoload files 26 packages you are using are looking for funding. Use the `composer fund` command to find out more! Creating wordpress-develop_phpunit_run ... done Installing... Running as single site... To run multisite, use -c tests/phpunit/multisite.xml Not running ajax tests. To execute these, use --group ajax. Not running ms-files tests. To execute these, use --group ms-files. Not running external-http tests. To execute these, use --group external-http. PHPUnit 7.5.20 by Sebastian Bergmann and contributors. ............................................................. 61 / 1473 ( 4%) ... ......... 1473 / 1473 (100%) Time: 7.07 seconds, Memory: 128.00 MB OK (1473 tests, 2741 assertions)
Notes for testers: The PHPUnit version will depend upon your PHP version. The Time
and Memory
values may be different on your machine.
Scenario 3: vendor
folder is deleted
The contributor has already run the above commands to setup the local testing environment but then deletes the vendor
folder.
Expectations:
- Composer package dependencies are installed
- No changes to the
composer.lock
file - Tests run
Instructions
Notes for testers: Same as Scenario 2 but this time delete the vendor
folder.
Contributor types the following in their command-line tool:
npm run test:php -- --group formatting
or
npm run test:php-composer -- --group formatting
Expected command-line output (or similar):
Creating wordpress-develop_php_run ... done Loading composer repositories with package information Updating dependencies Nothing to modify in lock file Installing dependencies from lock file (including require-dev) Package operations: 41 installs, 0 updates, 0 removals - Downloading squizlabs/php_codesniffer (3.6.0) ... - Downloading yoast/phpunit-polyfills (1.0.1) 0/41 [>---------------------------] 0% 7/41 [====>-----------------------] 17% 8/41 [=====>----------------------] 19% 12/41 [========>-------------------] 29% 16/41 [==========>-----------------] 39% 23/41 [===============>------------] 56% 27/41 [==================>---------] 65% 32/41 [=====================>------] 78% 37/41 [=========================>--] 90% 40/41 [===========================>] 97% 41/41 [============================] 100% - Installing squizlabs/php_codesniffer (3.6.0): Extracting archive ... - Installing yoast/phpunit-polyfills (1.0.1): Extracting archive 0/39 [>---------------------------] 0% 6/39 [====>-----------------------] 15% 9/39 [======>---------------------] 23% 10/39 [=======>--------------------] 25% 11/39 [=======>--------------------] 28% 14/39 [==========>-----------------] 35% 16/39 [===========>----------------] 41% 19/39 [=============>--------------] 48% 22/39 [===============>------------] 56% 24/39 [=================>----------] 61% 28/39 [====================>-------] 71% 32/39 [======================>-----] 82% 33/39 [=======================>----] 84% 34/39 [========================>---] 87% 35/39 [=========================>--] 89% 36/39 [=========================>--] 92% 37/39 [==========================>-] 94% 38/39 [===========================>] 97% 39/39 [============================] 100% Generating autoload files 26 packages you are using are looking for funding. Use the `composer fund` command to find out more! PHP CodeSniffer Config installed_paths set to ../../phpcompatibility/php-compatibility,../../phpcompatibility/phpcompatibility-paragonie,../../phpcompatibility/phpcompatibility-wp,../../wp-coding-standards/wpcs Creating wordpress-develop_phpunit_run ... done Installing... Running as single site... To run multisite, use -c tests/phpunit/multisite.xml Not running ajax tests. To execute these, use --group ajax. Not running ms-files tests. To execute these, use --group ms-files. Not running external-http tests. To execute these, use --group external-http. PHPUnit 7.5.20 by Sebastian Bergmann and contributors. ............................................................. 61 / 1473 ( 4%) ... ......... 1473 / 1473 (100%) Time: 6.64 seconds, Memory: 128.00 MB OK (1473 tests, 2741 assertions)
Notes for testers: The PHPUnit version will depend upon your PHP version. The Time
and Memory
values may be different on your machine.
Scenario 4: vendor
folder and composer.lock
are deleted
The contributor has already run the above commands to setup the local testing environment but then deletes the vendor
folder and composer.lock
file.
Notes for Testers: Delete both the vendor
folder and composer.lock
file.
Expectations:
- Composer package dependencies are installed
composer.lock
file is created- Tests run
Instructions:
Notes for Testers: Delete both the vendor
folder and composer.lock
file.
Contributor types the following in their command-line tool:
npm run test:php -- --group formatting
or
npm run test:php-composer -- --group formatting
Expected command-line output is similar to Scenario 3.
#11
@
3 years ago
Testing Report
Env
I tested on 3 different Macs:
Mac 1:
- OS: macOS Big Sur 11.4
- Docker Desktop: 3.3.3 (64133)
- npm: 6.14.13
- Command-line tool: iterm
Mac 2:
- OS: macOS Big Sur 11.5.1
- Docker Desktop: 3.5.2 (3.5.2.18)
- npm: 7.6.0
- Command-line tool: iterm
Mac 3:
- OS: macOS Big Sur 11.5.2
- Docker Desktop: 3.5.2 (3.5.2.18)
- npm: 6.14.13
- Command-line tool: iterm
Results
For all 3 of my machines.
Before applying PR 1627:
- All 4 scenarios failed as expected ✅
The following message displays in the command-line tool:
Error: The PHPUnit Polyfills library is a requirement for running the WP test suite. You need to run `composer update` before running the tests. Once the dependencies are installed, you can run the tests using the Composer-installed version of PHPUnit or using PHPUnit phar file, but the dependencies do need to be installed whichever way the tests are run.
After applying PR 1627:
- Scenario 1: worked as expected ✅
- Scenario 2: worked as expected ✅
- Scenario 3: worked as expected ✅
- Scenario 4: worked as expected ✅
This ticket was mentioned in Slack in #core-test by hellofromtonya. View the logs.
3 years ago
#13
@
3 years ago
- Keywords commit added
- Owner set to johnbillion
- Status changed from new to accepted
- Version trunk deleted
This also covers the scenario where an out of date composer.lock file is in place, which can be the case for existing contributors who have previously installed Composer dependencies but try to re-run the tests without running composer update
first.
johnbillion commented on PR #1627:
3 years ago
#15
#16
follow-up:
↓ 17
@
3 years ago
Uh oh... commit [51685] has broken the tests from running against PHP 8.1....
The reason for this is quite simple: not all PHPUnit dependencies have released a version yet which has declared compatibility with PHP 8.1, so on PHP 8.1 the composer update
needs to run with --ignore-platform-requirements
as otherwise PHPUnit 4.8.x will be installed (the last PHPUnit version which didn't have version constraints declared).
Previously in GH Actions, this was (and still is) handled by special casing PHP 8.1 in the "Composer install" step, but as the "Install WordPress" step is run after that step and now runs composer update
as well, the "Install WordPress" step removes the previously installed PHPUnit 9.x version and replaces it with PHPUnit 4.x.
And:
https://github.com/WordPress/wordpress-develop/runs/3452011214?check_suite_focus=true#step:16:83
I can currently think of two potential solution directions, though am not sure which one is best/feasible:
- Presuming the
install.js
script has access to the PHP version on which the script is being run (does it ?), special case the command for PHP 8.1 to use--ignore-platform-requirements
. - Presuming the
install.js
script has access to an environment variable indicating that it is being run in the context of a GitHub actions run, skip the call toinstall_composer_dependencies()
.
We may even need a combination of the two as with solution (1), it would be best to remove the "Composer install" step from the GitHub Actions unit test and code coverage workflows as that step is now handled via the Docker "Install WordPress" step, but on the other hand, if we do that, this may break the caching of the Composer downloads in GitHub Actions, slowing down the workflows.
@johnbillion @desrosj @netweb What do think would be the best way to solve this ?
#17
in reply to:
↑ 16
@
3 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
Replying to jrf:
Uh oh... commit [51685] has broken the tests from running against PHP 8.1....
Re-opening this issue...
- Presuming the
install.js
script has access to the PHP version on which the script is being run (does it ?), special case the command for PHP 8.1 to use--ignore-platform-requirements
.
We should be able to use wp-cli and the wp-cli()
helper function to check for that PHP version:
wp_cli( `eval 'echo PHP_VERSION;'` );
- Presuming the
install.js
script has access to an environment variable indicating that it is being run in the context of a GitHub actions run, skip the call toinstall_composer_dependencies()
.
Unure of what GitHub Actions environment variables are available to us here
#18
@
3 years ago
Just noting that as a quick-fix for the time being, while we figure out the Docker side of things, we could move the "Install Composer" step to after the "Install WordPress" step (or duplicate it) to make sure that PHP 8.1 will get the correct PHPUnit version again.
This ticket was mentioned in Slack in #core-test by sergey. View the logs.
3 years ago
#20
@
3 years ago
What if the installer is removed from install.js
? Instead of installing the dependencies during the npm installation process, install them when they are needed, i.e. when running the automated tests npm run test
and/or running phpcs.
Why?
Yesterday when setting up an environment for manually testing a patch, I noticed that the Composer dependencies were installed. For testers doing manual testing, this extra step adds more set up time and is unnecessary.
This ticket was mentioned in PR #1647 on WordPress/wordpress-develop by hellofromtonya.
3 years ago
#21
In [51685] added install_composer_dependencies()
to tools/local-env/scripts/install.js
to handle installing the
Composer dependencies during the npm installation workflow.
However, testers doing manual testing do not need these dependencies installed. This addition added extra and
unnecessary step up time for contributors.
By removing the installation of the Composer dependencies from the npm installation process, the dependencies will be installed when needed, i.e. when running npm run test:php
.
This also fixes the introduced problem that broke running tests on PHP 8.1.
Trac ticket: https://core.trac.wordpress.org/ticket/53945
hellofromtonya commented on PR #1647:
3 years ago
#22
Bummer, didn't resolve the PHP 8.1 issue as it still installs4.8.36
when running the tests:
- Downloading phpunit/phpunit (4.8.36)
https://github.com/WordPress/wordpress-develop/pull/1647/checks?check_run_id=3486341755
hellofromtonya commented on PR #1647:
3 years ago
#23
https://github.com/hellofromtonya/wordpress-develop/commit/8a7d2f720be37e5e6c493144cd7fd2c1466b0970 removes the composer update
from the test:php-composer
script as this is the script the CI/GHA uses whereas contributors use npm run test:php
.
Results:
The right version of PHPUnit is installed for the PHP 8.1 tests in the PHP 8.1 job:
PHPUnit 9.5.9 by Sebastian Bergmann and contributors.
#24
@
3 years ago
- Keywords commit removed
PR 1647 proposes a viable solution
- Removes the installation logic from
tools/local-env/scripts/install.js
- Removes the
composer update
cascading command from thetest:php-composer
script in thepackage.json
file
Both of these changes fixes the broken tests on PHP 8.1 in the CI GHA. Why? The phpunit-tests.yml
workflow includes logic to:
- run the right command for installing the Composer dependencies (see it here)
- uses
npm run test:php-composer
to run the tests (starts here)
What about contributors testing locally?
- Documentation in the handbook and the repo's README.md state to use
npm run test:php
. - Removing the composer install from the
npm run env:install
process solves the problem I noted above where folks doing local testing don't need these dependencies.
IMO the npm run test:php-composer
can exist for the CI's automated test runs while the local testing recommendation remains npm run test:php
.
cc @johnbillion @netweb @desrosj What do you all think?
Note: I'm removing commit
keyword until there's consensus.
This ticket was mentioned in Slack in #core-test by hellofromtonya. View the logs.
3 years ago
#27
follow-up:
↓ 28
@
3 years ago
@hellofromTonya Just looked at your PR, but correct me if I'm wrong, that looks like it doesn't solve the original problem ? nor the PHP 8.1 issue (not really).
It solves the current GH build problem only, but brings the rest of this ticket back to square 1.
As far as I can see, the changes currently proposed:
- Would work only if someone uses the PHPUnit PHAR /
test:php
command to run the tests. In that case,composer update
is run as part of thetest:php
command and would install/update the PHPUnit Polyfills (if necessary). The PHPUnit version installed is not (well, barely) relevant in that case as the Docker image will use the appropriate PHAR for the chosen PHP image. - Running
test:php-composer
would not work unless someone has already run acomposer update/install
prior to choosing that command (then again: that was also the case prior to this change). - Running
test:php-composer
may still show the "you need to update the PHPUnit Polyfills" message. - Running the
test:php-composer
command may be broken depending in the PHP version used when running the originalcomposer update/install
command, if that version does not match the PHP version of the Docker image used to run the tests. Think: local default PHP is PHP 5.6 and the Docker 8.0 image is used. - Would still necessitate instructing people to manually run
composer update -W --ignore-platform-reqs
for running the tests on PHP 8.1 withtest:php-composer
.
There also seems to be a misconception: "Running the composer update/install
command would slow down the general install".
While this is true in a situation when an install/update is actually needed (mismatch between the versions installed and the current PHP version), if there is no version mismatch, the command will just say "nothing to update" and move on, so for most users, the slowdown would barely be noticeable.
I have a feeling I may be missing something, so what am I missing ?
#28
in reply to:
↑ 27
;
follow-up:
↓ 31
@
3 years ago
Replying to jrf:
- Would work only if someone uses the PHPUnit PHAR /
test:php
command to run the tests. In that case,composer update
is run as part of thetest:php
command and would install/update the PHPUnit Polyfills (if necessary). The PHPUnit version installed is not (well, barely) relevant in that case as the Docker image will use the appropriate PHAR for the chosen PHP image.
I think the disconnect is this: the recommended command for running tests locally is npm run test:php
, not npm run test:php-composer
. This works for those using npm/Docker workflow too.
Here is where it's recommended:
- In the testing section of the `README.md`:
To run the tests
These commands run the PHP and end-to-end test suites, respectively:
npm run test:php
npm run test:e2e
Running the tests
Once the docker container is set up and provisioned, you can run the tests from the command-line:
npm run test:php
.
There is a note in this post about the intended differences between these 2 commands:
// Run within the local Docker container using version of // PHPUnit installed within the container. npm run test:php // Run within the local Docker container using version of // PHPUnit installed via Composer. npm run test:php-composer
However, we've changed this as the PHPUnit phar is no longer loaded in the container and instead is installed via Composer.
So I'm thinking test:php-composer
command is for the CI or for those contributors who don't want to install Composer or have it installed globally somewhere.
Replying to jrf:
There also seems to be a misconception: "Running the
composer update/install
command would slow down the general install".
While this is true in a situation when an install/update is actually needed (mismatch between the versions installed and the current PHP version), if there is no version mismatch, the command will just say "nothing to update" and move on, so for most users, the slowdown would barely be noticeable.
The time it takes depends upon a contributor's internet download speed as well as if there are any blips in the connectivity chain. While for many, it may be super duper fast, there is a potential for some contributors that it slows them down.
#29
@
3 years ago
Looking at the differences between the scripts:
npm run test:php
:
node ./tools/local-env/scripts/docker.js run -T php composer update -W && node ./tools/local-env/scripts/docker.js run --rm phpunit phpunit
whereas npm run test:php-composer
:
node ./tools/local-env/scripts/docker.js run -T php composer update -W && node ./tools/local-env/scripts/docker.js run --rm phpunit php ./vendor/bin/phpunit
the subtle difference of run phpunit phpunit
vs run phpunit php ./vendor/bin/phpunit
.
Running npm run test:php
on one of my machines that doesn't have PHPUnit phar installed nor PHPUnit installed via composer globally, it works. The only instance of PHPUnit is in the vendor bin.
Why does it work? Hmm, I'll do more investigation and testing to discover why.
Update:
Okay I see. The phar is installed in the Docker container and that's used when running npm run test:php
. I mistakingly thought this phar file wasn't installed in the container but instead only Composer version was used. My mistake.
So yes, PR 1647 is invalid.
Also, we'll need to update the handbook and README.md for when to use test:php
vs. test:php-composer
.
hellofromtonya commented on PR #1647:
3 years ago
#30
See this comment in the Trac ticket.
I mistakenly thought the phar was no longer installed in the container, i.e. thinking it also switched to using the Composer installed version. This means running npm run test:php
uses the phar file in the container and not the Composer installed version.
This PR then is invalid as contributors need to use npm run test:php-composer
when doing local testing.
#31
in reply to:
↑ 28
;
follow-up:
↓ 32
@
3 years ago
Replying to hellofromTonya:
I think the disconnect is this: the recommended command for running tests locally is
npm run test:php
, notnpm run test:php-composer
. This works for those using npm/Docker workflow too.
...
However, we've changed this as the PHPUnit phar is no longer loaded in the container and instead is installed via Composer.
As far as I see PHPUnit is always installed in /vendor when installing the (other) dependencies, either with local Composer or by npm run env:install
. If that's correct, for simplicity's sake perhaps better to always run it from there? Then the test:php-composer
command won't be needed.
Replying to jrf:
There also seems to be a misconception: "Running the
composer update/install
command would slow down the general install".
While this is true in a situation when an install/update is actually needed (mismatch between the versions installed and the current PHP version), if there is no version mismatch, the command will just say "nothing to update" and move on, so for most users, the slowdown would barely be noticeable.
The time it takes depends upon a contributor's internet download speed as well as if there are any blips in the connectivity chain. While for many, it may be super duper fast, there is a potential for some contributors that it slows them down.
Right. I'm also noticing a delay of about 2.5-3 min every time the tests are run with npm run test:php
or npm run test:php-composer
. It happens after Use the 'composer fund' command to find out more!
so presuming after composer has checked for updates. However skipping composer update -W
and running only % node ./tools/local-env/scripts/docker.js run --rm phpunit phpunit "--group" "formatting"
doesn't trigger that delay.
That ~3 min delay is pretty annoying especially when writing or fixing tests and running the same test 10-20 times in a row. Even if it's fixed, perhaps Composer updates should not be running every time PHPUnit is run, still takes 5-10 sec to check :)
> WordPress@5.9.0 test:php /Users/andrew/dev/wp/trunk > node ./tools/local-env/scripts/docker.js run -T php composer update -W && node ./tools/local-env/scripts/docker.js run --rm phpunit phpunit "--group" "formatting" Starting trunk-wpenv_mysql_1 ... done Loading composer repositories with package information Updating dependencies Nothing to modify in lock file Installing dependencies from lock file (including require-dev) Nothing to install, update or remove Generating autoload files 26 packages you are using are looking for funding. Use the `composer fund` command to find out more! ----------> delay happens here <--------- Starting trunk-wpenv_mysql_1 ... done Installing... Running as single site... To run multisite, use -c tests/phpunit/multisite.xml Not running ajax tests. To execute these, use --group ajax. Not running ms-files tests. To execute these, use --group ms-files. Not running external-http tests. To execute these, use --group external-http. PHPUnit 7.5.20 by Sebastian Bergmann and contributors. ............................................................. 61 / 1473 ( 4%) ...
#32
in reply to:
↑ 31
;
follow-up:
↓ 34
@
3 years ago
Replying to azaozz:
Right. I'm also noticing a delay of about 2.5-3 min every time the tests are run with
npm run test:php
ornpm run test:php-composer
. It happens afterUse the 'composer fund' command to find out more!
so presuming after composer has checked for updates. However skippingcomposer update -W
and running only% node ./tools/local-env/scripts/docker.js run --rm phpunit phpunit "--group" "formatting"
doesn't trigger that delay.
That ~3 min delay is pretty annoying especially when writing or fixing tests and running the same test 10-20 times in a row. Even if it's fixed, perhaps Composer updates should not be running every time PHPUnit is run, still takes 5-10 sec to check :)
The delay happening after the "composer fund" to me indicates that this has nothing to do with Composer as Composer is already finished by then.
If I remember correctly, I believe @johnbillion also mentioned something about running the tests via Docker being significantly slower (independently of this change and before this change was even made). He also mentioned a time difference of ~3 minutes.
So, yes, that must be really annoying, but I somehow have a feeling that that delay is unrelated to what we're doing in this ticket.
The ~3 minute delay sounds like a 180 second process timeout, so I wonder whether there is something going on in Docker where either a process is not passing an exit code and the script is waiting for one or where the exit code is not being checked correctly. Just guessing here, but might be a starting point for debugging this.
Either way, the delay sounds like something which most likely has nothing to do with Composer and is outside of the scope of this ticket.
I think a separate ticket needs to be opened to look into debugging that.
This ticket was mentioned in PR #1656 on WordPress/wordpress-develop by hellofromtonya.
3 years ago
#33
Changeset [51685] broke the PHP 8.1 tests as it used composer update
without --ignore-platform-reqs
which caused the wrong version of PHPUnit to be installed/updated on PHP 8.1 runs. This happened due to 2 changes:
composer update
is run duringenv:install
processcomposer update
is run again each timetest:php-composer
run
Both of the changes were for local testing and not the CI.
This PR fixes the CI while retaining the needed improvements for local testing when running npm run test:php-composer
by:
- Switches the CI to use the native
docker-compose run
instead of the npm scripttest:php-composer
- Removes the
composer update
command from theenv:install
process
Trac ticket: https://core.trac.wordpress.org/ticket/53945
#34
in reply to:
↑ 32
@
3 years ago
Replying to jrf:
The delay happening after the "composer fund" to me indicates that this has nothing to do with Composer as Composer is already finished by then.
Right, but it is caused by running composer update -W
that was added in [51685]. If that is not run, there is no delay.
If I remember correctly, I believe @johnbillion also mentioned something about running the tests via Docker being significantly slower (independently of this change and before this change was even made). He also mentioned a time difference of ~3 minutes.
Right, it is slower, by about 1.5 min here. As far as I understand this is caused by running from a Docker container vs. directly in the OS. All of WP runs a bit slower. That's not the same as the delay after composer update
finishes and before phpunit --group 12345
starts.
The ~3 minute delay sounds like a 180 second process timeout, so I wonder whether there is something going on in Docker
Yeah, could be. I'm not that good at debugging Docker, but will try looking more there.
My other point was that it doesn't make sense to run composer update -W
every time when doing npm run test:php
. When working on tests the same test case is usually run repeatedly, so checking for Composer updates 20-30 times per hour is not good.
For that reason I think that part of [51685] should be reverted. Perhaps another method can be found to ensure Composer updates are run from time to time, or that can be a documented step like with npm updates.
#35
@
3 years ago
PR 1656 solves the CI problem introduced in [51685] by:
- Removing the
composer update
logic from theinstall.js
script (as not needed for the CI or for those who are not running the PHPUnit or phpcs tasks) - Switching to use the original (pre-51685) node full command instead of the
npm run test:php-composer
The PR solves the CI problem while retaining the objectives of this ticket to hide the composer update
commands when working locally.
johnbillion commented on PR #1656:
3 years ago
#37
#39
@
3 years ago
Test Report
Env
- node: 14.17.0
- npm: 6.14.13
- Docker Desktop: 3.5.2
- WordPress:
trunk
- OS: macOS Big Sur 11.5.2
- Command-Line tool: iTerm2 3.4.8
First run steps
Set up a clean first run:
- Delete the container in the Docker Desktop app
Clean/Purge data
in the Docker Desktop app- Delete the
node_modules
andvendor
folders - Delete the
composer.lock
file - Clear the local cache:
npm cache clean --force
composer clearcache
- Fetch and merge the latest from
trunk
:git fetch upstream
git merge upstream/master
- Start up the npm Docker environment:
npm install
npm run build:dev
npm run env:start
npm run env:install
- Test
composer update
and clock the time delay once the message appears
- Repeat all the above steps to test
npm run test:php -- --group cron
- Repeat all the above steps to test
npm run test:php-composer -- --group cron
Repeat run steps
After the first run (see above), clock the time delay once the message appears but don't do the clean up step.
Repeatedly run:
composer update
and then note the time delay once the message appears. Repeat.- Repeat for
npm run test:php -- --group cron
- Repeat for
npm run test:php-composer -- --group cron
Results
The noted time delay after the Use the 'composer fund' command to find out more!
message appears:
composer update
- First run: 2.36 seconds
- Rerunning: 1.02 sec, 0.96 sec, 1.13 sec, 1.15 sec
npm run test:php -- --group cron
- First run: 18.85 secs
- Rerunning: 2.52 sec, 1.35 sec, 1.60 sec, 1.59 sec, 1.73 sec
npm run test:php-composer -- --group cron
- First run: 19.22 secs
- Rerunning: 1.14 sec, 1.93 sec, 1.55 sec, 2.06 sec, 1.83 sec
#40
follow-up:
↓ 41
@
3 years ago
Replying to azaozz:
I'm also noticing a delay of about 2.5-3 min every time the tests are run with
npm run test:php
ornpm run test:php-composer
. It happens afterUse the 'composer fund' command to find out more!
With the latest trunk
update (that include both changesets in this ticket), I ran a series of tests to clock the delay that happens after that composer fund
message appears. See Test Report.
I wasn't able to reproduce the 2-3 minute delay after the composer fund
message appeared. Rather, on the first run it was about 18-20 seconds and on average about 1.5 seconds for rerunning tests over and over again.
@azaozz can you check again on your machine with the latest trunk
? Please try it before and after the Composer deps are installed.
#41
in reply to:
↑ 40
@
3 years ago
Replying to hellofromTonya:
I wasn't able to reproduce the 2-3 minute delay after the
composer fund
message appeared.
Right, after updating and resetting Docker I only see a delay on the first run. Seems the problem was there. In consequent runs the delay is only about a second.
on the first run it was about 18-20 seconds and on average about 1.5 seconds for rerunning tests over and over again.
Yeah, the numbers here are similar: 20-25 sec. on the first run, then the longer delay after Composer and before PHPUnit (more like 50-60 seconds now). Running the tests again, the delays are a lot smaller.
Tried to figure out what's going on and it looks like npm is running some update checks (perhaps) as it is making few hundreds requests during that first-run delay. Seems that's from the Creating trunk-wpenv_phpunit_run ... done
and Installing...
:
... 26 packages you are using are looking for funding. Use the `composer fund` command to find out more! Creating trunk-wpenv_phpunit_run ... done Installing... Running as single site... To run multisite, use -c tests/phpunit/multisite.xml Not running ajax tests. To execute these, use --group ajax. ...
Another thing I've noticed is that npm run test:php
will fail if the network connection is shaky/faulty and Composer is not able to check for updates. It also fails on subsequent runs when there will be no updates available. The PHP tests (mostly) run with no internet connection (although some fail instead of being skipped). Not a big deal but may be good to fix.
Whoopsie copy/paste issue with the link to running tests in the README.md.