Build/Test Tools: Propagate the exit status of the local environment commands - #12735
Build/Test Tools: Propagate the exit status of the local environment commands#12735adimoldovan wants to merge 3 commits into
Conversation
…commands.
`start.js` runs `docker compose up` through `spawnSync` and never inspects the
result, so a failed pull does not fail the script. Execution continues into
`composer update -W` with containers that may not exist, and the error surfaces
later and in the wrong place.
`docker.js` calls `process.exit( returns.status )`, and `status` is `null` when
Docker cannot be spawned. `process.exit( null )` exits 0, so `npm run env:pull`
reports success when the Docker CLI is missing.
Reproduce with:
LOCAL_PHP=this-tag-does-not-exist npm run env:start; echo $?
See #65745.
There was a problem hiding this comment.
Pull request overview
This PR updates the local Docker environment helper scripts to correctly propagate failures from spawnSync() so npm commands fail at the correct step (instead of continuing after Docker failures and surfacing errors later).
Changes:
- Capture and check the result of
docker compose upinstart.js, exiting non-zero when container startup fails. - Avoid exiting 0 when the Docker CLI cannot be spawned in
docker.jsby handlingstatus === nulland reporting a clearer error.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tools/local-env/scripts/start.js | Capture spawnSync() result for docker compose up and exit non-zero on failure to start/pull containers. |
| tools/local-env/scripts/docker.js | Emit an error when Docker cannot be spawned and ensure a non-zero exit code when appropriate. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
…nment command. `docker.js` exited 0 for any signal, so a command killed by SIGTERM or SIGKILL reported success. That reintroduced the false success this ticket set out to remove: the retry loop in `reusable-phpunit-tests-v3.yml` branches on the status of `npm run env:pull` and would treat a killed pull as a completed one. Restrict the exemption to SIGINT, which is how a long-running command such as `env:logs` is normally ended. Report every other signal and exit non-zero. `start.js` exempts no signal, because `env:start` runs `composer update -W` next and that must not run against containers that never came up. Say so in the comment, so the difference between the two files is deliberate. Also fold the unreachable `up.error` branch in `start.js` into the failure message. The `docker info` check above it already throws when the Docker CLI is missing or the daemon is down.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
There was a problem hiding this comment.
Tested with head a88479c
Confirmed failed image pulls and a missing Docker executable exit non-zero. SIGTERM exits non-zero, SIGINT remains clean for interactive commands, and a normal environment start and install > succeeded
The affected code shape was also verified on 6.8, 6.9, and 7.0.
Good to land.
tools/local-env/scripts/start.jsrunsdocker compose upthroughspawnSyncand never inspects the result. A failed pull therefore does not fail the script. Execution continues intocomposer update -Wwith containers that may not exist, and the error surfaces later and in the wrong step.tools/local-env/scripts/docker.jshas the same class of defect. It callsprocess.exit( returns.status ), andstatusisnullwhen Docker cannot be spawned.process.exit( null )exits 0, sonpm run env:pullreports success when the Docker CLI is missing.This PR checks the status in both files and propagates it, falling back to 1 when
statusisnull.A command killed by a signal is a failure, with one exception:
docker.jsexits 0 on SIGINT, because Ctrl+C is the normal way to end a long-running command such asnpm run env:logsand is not worth an npm error block. Every other signal exits non-zero.start.jsexempts nothing, becausenpm run env:startrunscomposer update -Wnext and that must not run against containers that never came up.The change is deliberately kept to that scope so it can be backported.
Backport
start.jsanddocker.json the 6.8, 6.9 and 7.0 branches carry the same defect. Branches 6.7 and earlier run these commands throughexecSync, which throws on a non-zero exit, so they are not affected.start.jsdocker.jsexecSync)execSync)Testing instructions
Reproduce the discarded status. Before this change the pull fails and the exit code is 0. After it, the exit code is 1.
Reproduce the
docker.jsdefect. Before this change the exit code is 0. After it, the exit code is 1.Confirm a normal start still works.
Confirm Ctrl+C during
npm run env:logsstill exits without an npm error block.Trac ticket: https://core.trac.wordpress.org/ticket/65745
Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 5
Used for: Investigating the CI failures, writing the patch, and iterative code review. The behaviour described above was reproduced and verified locally before and after the change.
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.