Skip to content

Build/Test Tools: Improve dev environment's CLI in speed, non-interactive usage, and argument handling #8969

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 15 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -155,7 +155,7 @@ jobs:
run: npm run env:restart

- name: Test a CLI command
run: npm run env:cli wp option get siteurl
run: npm run env:cli option get siteurl
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wp is removed here because previously the entrypoint would inject it if it was missing: https://github.com/WordPress/wpdev-docker-images/blob/becc6740684d6ff6414d6539df040633b5ac5794/entrypoint/entrypoint-cli.sh#L16-L17

With this PR, the wp is always assumed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is technically a breaking change, I searched github for npm run env:cli wp and there is only one reference which seems to be from a fork of a demo rather than actual production code, but it still might be worth a PR there after this lands and a quick note in the core room so folks are aware. I don't think it needs a dev note though since this isn't a public API.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


- name: Test logs command
run: npm run env:logs
3 changes: 3 additions & 0 deletions docker-compose.yml
Original file line number Diff line number Diff line change
@@ -111,6 +111,9 @@ services:
volumes:
- ./:/var/www

# Keeps the service alive.
command: 'sleep infinity'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a true idling container I would rather use
command: 'tail -f /dev/null'

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose this for consistency with wp-env.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems there were no big explanations
WordPress/gutenberg@c11312a

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If Gutenberg switches back to tail we can switch it back here too.


# The init directive ensures the command runs with a PID > 1, so Ctrl+C works correctly.
init: true

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -184,7 +184,7 @@
"env:clean": "node ./tools/local-env/scripts/docker.js down -v --remove-orphans",
"env:reset": "node ./tools/local-env/scripts/docker.js down --rmi all -v --remove-orphans",
"env:install": "node ./tools/local-env/scripts/install.js",
"env:cli": "node ./tools/local-env/scripts/docker.js run --rm cli",
"env:cli": "node ./tools/local-env/scripts/docker.js exec cli wp --allow-root",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"env:logs": "node ./tools/local-env/scripts/docker.js logs",
"env:pull": "node ./tools/local-env/scripts/docker.js pull",
"test:performance": "wp-scripts test-playwright --config tests/performance/playwright.config.js",
33 changes: 24 additions & 9 deletions tools/local-env/scripts/docker.js
Original file line number Diff line number Diff line change
@@ -1,21 +1,36 @@
const dotenv = require( 'dotenv' );
/* jshint node:true */

const dotenv = require( 'dotenv' );
const dotenvExpand = require( 'dotenv-expand' );
const { execSync } = require( 'child_process' );
const { spawnSync } = require( 'child_process' );
const local_env_utils = require( './utils' );

dotenvExpand.expand( dotenv.config() );

const composeFiles = local_env_utils.get_compose_files();

if (process.argv.includes('--coverage-html')) {
if ( process.argv.includes( '--coverage-html' ) ) {
process.env.LOCAL_PHP_XDEBUG = 'true';
process.env.LOCAL_PHP_XDEBUG_MODE = 'coverage';
}

// This try-catch prevents the superfluous Node.js debugging information from being shown if the command fails.
try {
// Execute any Docker compose command passed to this script.
execSync( 'docker compose ' + composeFiles + ' ' + process.argv.slice( 2 ).join( ' ' ), { stdio: 'inherit' } );
} catch ( error ) {
process.exit( 1 );
// Add --no-TTY (-T) arg after exec and run commands when STDIN is not a TTY.
const dockerCommand = process.argv.slice( 2 );
if ( [ 'exec', 'run' ].includes( dockerCommand[0] ) && ! process.stdin.isTTY ) {
dockerCommand.splice( 1, 0, '--no-TTY' );
}

// Execute any Docker compose command passed to this script.
const returns = spawnSync(
'docker',
[
'compose',
...composeFiles
.map( ( composeFile ) => [ '-f', composeFile ] )
.flat(),
...dockerCommand,
],
{ stdio: 'inherit' }
);

process.exit( returns.status );
9 changes: 5 additions & 4 deletions tools/local-env/scripts/install.js
Original file line number Diff line number Diff line change
@@ -13,7 +13,10 @@ dotenvExpand.expand( dotenv.config() );
local_env_utils.determine_auth_option();

// Create wp-config.php.
wp_cli( `config create --dbname=wordpress_develop --dbuser=root --dbpass=password --dbhost=mysql --force --config-file=${process.env.LOCAL_DIR}/../wp-config.php` );
wp_cli( `config create --dbname=wordpress_develop --dbuser=root --dbpass=password --dbhost=mysql --force --config-file="wp-config.php"` );

// Since WP-CLI runs as root, the wp-config.php created above will be read-only. This needs to be writable for the sake of E2E tests.
execSync( 'node ./tools/local-env/scripts/docker.js exec cli chmod 666 wp-config.php' );
Comment on lines +18 to +19
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear to me why this is needed now. Without it, E2E tests fail due to wp-config.php not being writable: https://github.com/WordPress/wordpress-develop/actions/runs/15627027244/job/44023152411

In 0e4ddbb I tried using the node API to set the file permissions, but this failed because it's not being run as root: https://github.com/WordPress/wordpress-develop/actions/runs/15624512371/job/44016093197

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in 0e4ddbb it was running on the local filesystem whereas in this implementation it is running in the container.


// Add the debug settings to wp-config.php.
// Windows requires this to be done as an additional step, rather than using the --extra-php option in the previous step.
@@ -61,7 +64,5 @@ wait_on( {
* @param {string} cmd The WP-CLI command to run.
*/
function wp_cli( cmd ) {
const composeFiles = local_env_utils.get_compose_files();

execSync( `docker compose ${composeFiles} run --quiet-pull --rm cli ${cmd} --path=/var/www/${process.env.LOCAL_DIR}`, { stdio: 'inherit' } );
execSync( `npm --silent run env:cli -- ${cmd} --path=/var/www/${process.env.LOCAL_DIR}`, { stdio: 'inherit' } );
}
61 changes: 51 additions & 10 deletions tools/local-env/scripts/start.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
/* jshint node:true */

const dotenv = require( 'dotenv' );
const dotenvExpand = require( 'dotenv-expand' );
const { execSync } = require( 'child_process' );
const { execSync, spawnSync } = require( 'child_process' );
const local_env_utils = require( './utils' );
const { constants, copyFile } = require( 'node:fs' );

// Copy the default .env file when one is not present.
copyFile( '.env.example', '.env', constants.COPYFILE_EXCL, (e) => {
copyFile( '.env.example', '.env', constants.COPYFILE_EXCL, () => {
console.log( '.env file already exists. .env.example was not copied.' );
});

@@ -28,18 +30,38 @@ try {
}

// Start the local-env containers.
const containers = ( process.env.LOCAL_PHP_MEMCACHED === 'true' )
? 'wordpress-develop memcached'
: 'wordpress-develop';
execSync( `docker compose ${composeFiles} up --quiet-pull -d ${containers}`, { stdio: 'inherit' } );
const containers = [ 'wordpress-develop', 'cli' ];
if ( process.env.LOCAL_PHP_MEMCACHED === 'true' ) {
containers.push( 'memcached' );
}

spawnSync(
'docker',
[
'compose',
...composeFiles.map( ( composeFile ) => [ '-f', composeFile ] ).flat(),
'up',
'--quiet-pull',
'-d',
...containers,
],
{ stdio: 'inherit' }
);

// If Docker Toolbox is being used, we need to manually forward LOCAL_PORT to the Docker VM.
if ( process.env.DOCKER_TOOLBOX_INSTALL_PATH ) {
// VBoxManage is added to the PATH on every platform except Windows.
const vboxmanage = process.env.VBOX_MSI_INSTALL_PATH ? `${ process.env.VBOX_MSI_INSTALL_PATH }/VBoxManage` : 'VBoxManage'
const vboxmanage = process.env.VBOX_MSI_INSTALL_PATH ? `${ process.env.VBOX_MSI_INSTALL_PATH }/VBoxManage` : 'VBoxManage';

// Check if the port forwarding is already configured for this port.
const vminfoBuffer = execSync( `"${ vboxmanage }" showvminfo "${ process.env.DOCKER_MACHINE_NAME }" --machinereadable` );
const vminfoBuffer = spawnSync(
vboxmanage,
[
'showvminfo',
process.env.DOCKER_MACHINE_NAME,
'--machinereadable'
]
).stdout;
const vminfo = vminfoBuffer.toString().split( /[\r\n]+/ );

vminfo.forEach( ( info ) => {
@@ -53,10 +75,29 @@ if ( process.env.DOCKER_TOOLBOX_INSTALL_PATH ) {

// Delete rules that are using the port we need.
if ( rule[ 3 ] === process.env.LOCAL_PORT || rule[ 5 ] === process.env.LOCAL_PORT ) {
execSync( `"${ vboxmanage }" controlvm "${ process.env.DOCKER_MACHINE_NAME }" natpf1 delete ${ rule[ 0 ] }`, { stdio: 'inherit' } );
spawnSync(
vboxmanage,
[
'controlvm',
process.env.DOCKER_MACHINE_NAME,
'natpf1',
'delete',
rule[ 0 ]
],
{ stdio: 'inherit' }
);
}
} );

// Add our port forwarding rule.
execSync( `"${ vboxmanage }" controlvm "${ process.env.DOCKER_MACHINE_NAME }" natpf1 "tcp-port${ process.env.LOCAL_PORT },tcp,127.0.0.1,${ process.env.LOCAL_PORT },,${ process.env.LOCAL_PORT }"`, { stdio: 'inherit' } );
spawnSync(
vboxmanage,
[
'controlvm',
process.env.DOCKER_MACHINE_NAME,
'natpf1',
`tcp-port${ process.env.LOCAL_PORT },tcp,127.0.0.1,${ process.env.LOCAL_PORT },,${ process.env.LOCAL_PORT }`
],
{ stdio: 'inherit' }
);
}
10 changes: 7 additions & 3 deletions tools/local-env/scripts/utils.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
/* jshint node:true */

const { existsSync } = require( 'node:fs' );

const local_env_utils = {
@@ -10,12 +12,14 @@ const local_env_utils = {
*
* When PHP 7.2 or 7.3 is used in combination with MySQL 8.4, an override file will also be returned to ensure
* that the mysql_native_password plugin authentication plugin is on and available for use.
*
* @return {string[]} Compose files.
*/
get_compose_files: function() {
var composeFiles = '-f docker-compose.yml';
const composeFiles = [ 'docker-compose.yml' ];

if ( existsSync( 'docker-compose.override.yml' ) ) {
composeFiles = composeFiles + ' -f docker-compose.override.yml';
composeFiles.push( 'docker-compose.override.yml' );
}

if ( process.env.LOCAL_DB_TYPE !== 'mysql' ) {
@@ -28,7 +32,7 @@ const local_env_utils = {

// PHP 7.2/7.3 in combination with MySQL 8.4 requires additional configuration to function properly.
if ( process.env.LOCAL_DB_VERSION === '8.4' ) {
composeFiles = composeFiles + ' -f tools/local-env/old-php-mysql-84.override.yml';
composeFiles.push( 'tools/local-env/old-php-mysql-84.override.yml' );
}

return composeFiles;

Unchanged files with check annotations Beta

test( 'Test dismissing failed upload works correctly', async ({ page, admin, requestUtils }) => {
// Log in before visiting admin page.
await requestUtils.login();
await admin.visitAdminPage( '/media-new.php' );

Check failure on line 14 in tests/e2e/specs/media-upload.test.js

GitHub Actions / Test with SCRIPT_DEBUG enabled / Run E2E tests

[chromium] › tests/e2e/specs/media-upload.test.js:11:5 › Test dismissing failed upload works correctly

2) [chromium] › tests/e2e/specs/media-upload.test.js:11:5 › Test dismissing failed upload works correctly Error: Not logged in 12 | // Log in before visiting admin page. 13 | await requestUtils.login(); > 14 | await admin.visitAdminPage( '/media-new.php' ); | ^ 15 | 16 | // It takes a moment for the multi-file uploader to become available. 17 | await page.waitForLoadState('load'); at Admin.visitAdminPage (/home/runner/work/wordpress-develop/wordpress-develop/node_modules/@wordpress/e2e-test-utils-playwright/src/admin/visit-admin-page.ts:36:9) at /home/runner/work/wordpress-develop/wordpress-develop/tests/e2e/specs/media-upload.test.js:14:2

Check failure on line 14 in tests/e2e/specs/media-upload.test.js

GitHub Actions / Test with SCRIPT_DEBUG disabled / Run E2E tests

[chromium] › tests/e2e/specs/media-upload.test.js:11:5 › Test dismissing failed upload works correctly

2) [chromium] › tests/e2e/specs/media-upload.test.js:11:5 › Test dismissing failed upload works correctly Retry #1 ─────────────────────────────────────────────────────────────────────────────────────── Error: Not logged in 12 | // Log in before visiting admin page. 13 | await requestUtils.login(); > 14 | await admin.visitAdminPage( '/media-new.php' ); | ^ 15 | 16 | // It takes a moment for the multi-file uploader to become available. 17 | await page.waitForLoadState('load'); at Admin.visitAdminPage (/home/runner/work/wordpress-develop/wordpress-develop/node_modules/@wordpress/e2e-test-utils-playwright/src/admin/visit-admin-page.ts:36:9) at /home/runner/work/wordpress-develop/wordpress-develop/tests/e2e/specs/media-upload.test.js:14:2

Check failure on line 14 in tests/e2e/specs/media-upload.test.js

GitHub Actions / Test with SCRIPT_DEBUG disabled / Run E2E tests

[chromium] › tests/e2e/specs/media-upload.test.js:11:5 › Test dismissing failed upload works correctly

2) [chromium] › tests/e2e/specs/media-upload.test.js:11:5 › Test dismissing failed upload works correctly Error: Not logged in 12 | // Log in before visiting admin page. 13 | await requestUtils.login(); > 14 | await admin.visitAdminPage( '/media-new.php' ); | ^ 15 | 16 | // It takes a moment for the multi-file uploader to become available. 17 | await page.waitForLoadState('load'); at Admin.visitAdminPage (/home/runner/work/wordpress-develop/wordpress-develop/node_modules/@wordpress/e2e-test-utils-playwright/src/admin/visit-admin-page.ts:36:9) at /home/runner/work/wordpress-develop/wordpress-develop/tests/e2e/specs/media-upload.test.js:14:2
// It takes a moment for the multi-file uploader to become available.
await page.waitForLoadState('load');
await expect(
page,
'should redirect to the installation page'
).toHaveURL( /wp-admin\/install\.php$/ );

Check failure on line 40 in tests/e2e/specs/install.test.js

GitHub Actions / Test with SCRIPT_DEBUG enabled / Run E2E tests

[chromium] › tests/e2e/specs/install.test.js:34:6 › WordPress installation process › should install WordPress with pre-existing database credentials

1) [chromium] › tests/e2e/specs/install.test.js:34:6 › WordPress installation process › should install WordPress with pre-existing database credentials Error: should redirect to the installation page Timed out 5000ms waiting for expect(locator).toHaveURL(expected) Locator: locator(':root') Expected pattern: /wp-admin\/install\.php$/ Received string: "http://localhost:8889/" Call log: - should redirect to the installation page with timeout 5000ms - waiting for locator(':root') 9 × locator resolved to <html lang="en-US">…</html> - unexpected value "http://localhost:8889/" 38 | page, 39 | 'should redirect to the installation page' > 40 | ).toHaveURL( /wp-admin\/install\.php$/ ); | ^ 41 | 42 | await expect( 43 | page.getByText( /WordPress database error/ ), at /home/runner/work/wordpress-develop/wordpress-develop/tests/e2e/specs/install.test.js:40:5

Check failure on line 40 in tests/e2e/specs/install.test.js

GitHub Actions / Test with SCRIPT_DEBUG disabled / Run E2E tests

[chromium] › tests/e2e/specs/install.test.js:34:6 › WordPress installation process › should install WordPress with pre-existing database credentials

1) [chromium] › tests/e2e/specs/install.test.js:34:6 › WordPress installation process › should install WordPress with pre-existing database credentials Error: should redirect to the installation page Timed out 5000ms waiting for expect(locator).toHaveURL(expected) Locator: locator(':root') Expected pattern: /wp-admin\/install\.php$/ Received string: "http://localhost:8889/" Call log: - should redirect to the installation page with timeout 5000ms - waiting for locator(':root') 9 × locator resolved to <html lang="en-US">…</html> - unexpected value "http://localhost:8889/" 38 | page, 39 | 'should redirect to the installation page' > 40 | ).toHaveURL( /wp-admin\/install\.php$/ ); | ^ 41 | 42 | await expect( 43 | page.getByText( /WordPress database error/ ), at /home/runner/work/wordpress-develop/wordpress-develop/tests/e2e/specs/install.test.js:40:5
await expect(
page.getByText( /WordPress database error/ ),