Repository navigation
Every WP-CLI subcommand, run through wp rather than through PHP - #2
Merged
Merged
Conversation
CI exercised three of the thirteen subcommands and asserted nothing about
any of them beyond an exit status, plus one hand-rolled check that
`import --mode=replace` refuses without `--yes`. The other ten were
covered by nobody: not by the integration suite, which calls PHP and so
never sees WP-CLI, and not by CI.
That gap is not theoretical. What PHPUnit cannot reach is everything
WP-CLI owns -- whether a method is registered as a subcommand at all,
whether its `## OPTIONS` block parses, whether a flag the docblock
advertises is accepted, and what the process exits with. A command can be
correct PHP and unreachable from a shell, which is the only place anybody
runs it.
tests/cli/commands.sh runs all thirteen and makes 25 assertions about
what they print and what they exit with. It runs the same way by hand and
in CI:
BFW_WP="ddev wp" bash tests/cli/commands.sh
Two behaviours are pinned because they are the ones that would be read
wrong. `check` exits 0 whether or not the address was blocked -- the
status reports whether the query ran, so a deploy script treating
non-zero as "blocked" would be wrong on every run. And a destructive
command must exit NON-zero when it refuses: WP_CLI::confirm() calls
halt(0) on EOF, so the failure being guarded against is a prompt nobody
answers followed by an exit 0 that reads as success.
Agreeing to a destructive command is opt-in through
BFW_CLI_DESTRUCTIVE=1. CI opts in, because its site goes away with the
runner; a hand run does not, so pointing this at a site you care about
does not empty its block list. The refusals are still asserted either
way, which is the half that has actually broken.
Nothing here reads a file back from the script's own filesystem. `ddev
wp` runs inside a container, so a path this script can see is not a path
the command can write to -- the first draft used mktemp and failed on
exactly that.
Found while running them by hand, and left alone pending a decision:
`sources` lists presets while `refresh-sources` re-fetches the lists a
rule references. Two different things wearing one word. The README now
says so; renaming is free while nothing is released, and is not mine to
do unasked. `refresh-sources` was also missing from the README's command
list entirely, which is fixed.
1 task done
Same nine jobs, same matrix, same refusal to let anything fail softly.
Kanopi runs CircleCI everywhere else, and a plugin whose CI lives
somewhere other than the rest of the estate is one nobody watches.
Not built on kanopi/ci-tools. The orb's `composer` job is what the site
repos use for PHPCS and PHPStan, and it is a good fit there -- but it
carries Pantheon deploy machinery and the kanopi-code context, and this
repo needs a PHP matrix and a MySQL service that the orb job does not
express. The config is self-contained instead, in the same house style:
versions as anchors at the top, cimg images, caches keyed on the lock.
Two things the images forced. `cimg/php` carries Composer, mysqli,
pdo_mysql, unzip and dockerize, and carries neither the mysql client nor
WP-CLI. So WP-CLI is installed from the phar, and the root password is
set in the service's own environment rather than ALTERed afterwards --
which removes the mysql client the old workflow needed, and is a
simplification the move paid for rather than a compromise it forced.
Better than what it replaces, in three small ways. `dockerize -wait`
instead of `sleep 3` before the database and before the built-in web
server, which is the fragility behind the one e2e flake seen on nightly.
JUnit output from all three suites into `store_test_results`, so a
failure is a named test in the UI rather than a log to scroll. And the
zip as a build artifact on every run, which is the first time it has
been retrievable without running the build yourself.
Verified by running the jobs, not by validating the YAML. `circleci
config validate` passes and `config process` expands all nine job names,
but that proves only that the file parses. Each job was then run in the
image it will actually use, with MySQL in a shared network namespace so
it answers on 127.0.0.1 exactly as CircleCI arranges it:
static cimg/php:8.1 platform-reqs, phpcs, phpstan green
unit cimg/php:8.2 55 tests, junit written green
unit cimg/php:8.4 55 tests, junit written green
integration cimg/php:8.1 122 + 6 e2e + 25 wp-cli green
package cimg/php:8.3 scoped zip, installs with no
composer on PATH green
That run also caught the kind of thing validation cannot: `wp core
download` takes an optional positional download URL, so a stray flag
becomes the URL and the command fails with "Version option is not
available for URL downloads". Mine was in the throwaway script rather
than in the config, but the config would have failed the same way.
The repo still has to be enabled in the CircleCI app before any of this
runs, and GitHub's required checks still name the old jobs.
Merged
1 task done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CI ran three of the thirteen WP-CLI subcommands and asserted nothing about any of them beyond an exit status, plus one hand-rolled check that
import --mode=replacerefuses without--yes. The other ten were covered by nobody — not by the integration suite, which calls PHP and so never sees WP-CLI, and not by CI.I ran all thirteen by hand against the dev site first. They work. Then I wrote the thing that means nobody has to do that again.
What this covers that PHPUnit cannot
Everything WP-CLI owns: whether a method is registered as a subcommand at all, whether its
## OPTIONSblock parses, whether a flag the docblock advertises is accepted, and what the process exits with. A command can be correct PHP and unreachable from a shell, which is the only place anybody runs it.tests/cli/commands.sh— 13 subcommands, 25 assertions, same invocation by hand and in CI:BFW_WP="ddev wp" bash tests/cli/commands.shTwo behaviours are pinned because they are the ones that get read wrong:
checkexits 0 whether or not the address was blocked. The status reports whether the query ran. A deploy script treating non-zero as "this address is blocked" would be wrong on every run.WP_CLI::confirm()callshalt(0)on EOF, so the failure being guarded against is a prompt nobody answers followed by an exit 0 that a deploy script reads as success.Safe to run against a site you care about
Agreeing to a destructive command (
clear-blocked --yes,import --mode=replace --yes) is opt-in throughBFW_CLI_DESTRUCTIVE=1. CI opts in because its site goes away with the runner; a hand run does not, so pointing this at your dev site does not empty its block list. The refusals are asserted either way — that is the half that has actually broken.Two things the manual pass turned up
sourceslists presets.refresh-sourcesre-fetches the lists a rule references. Two different things wearing one word, andwp basic-firewall sourcesreturning presets is a surprise if you came looking for your referenced lists. The README now says so plainly. Renaming is free while nothing is released — worth doing, and not something I'd change unasked.refresh-sourceswas missing from the README's command list entirely — it shipped without ever being documented. Fixed.Verification
25 assertions pass locally in both modes; the dev site is unchanged afterwards (
Rules configured 7, mode unchanged).shellcheckclean.phpcs,phpstan, 55 unit and 122 integration tests unaffected. The exactBFW_WP="wp --path=… --allow-root"form CI uses was run inside the container first, because global flags ahead of the subcommand is the sort of thing that works everywhere except where you need it.