test: add PHPUnit suite covering shortcode escaping, plus the missing CI workflow - #26
Merged
Conversation
…ntracts Closes the "No unit tests" item in ROADMAP.md's Deferred / not planned section. The motivation is specific rather than generic coverage. Both shortcode callbacks build and *return* markup, and PHPCS's EscapeOutput sniff only inspects echo/print, so an unescaped value concatenated into the returned string lints clean. That is the bug class that actually shipped (ROADMAP item 4), and it is the plugin's only permanent blind spot. PHPUnit is the single new dev dependency. Tests run against hand-written WordPress/ACF doubles in tests/wp-stubs.php -- no WordPress install, no database, no ACF Pro, ~10ms. Brain Monkey was rejected because the have_rows()/the_row() cursor has to be hand-written either way and it handles include-time hook registration badly; integration tests were rejected because the CPT and repeater come from ACF Pro, a paid non-public dependency. esc_html() is a faithful double but wp_kses_post() is a pass-through spy, so the suite asserts the escape-before-concatenate contract rather than real KSES filtering. Documented at the definition and in CLAUDE.md, since a green suite is not proof of XSS safety. Every test was mutation-checked: deleting either escaper, making wp_reset_postdata() unreachable again, regressing the stylesheet path, typoing the CPT slug, or dropping the_row() from a loop each fail the suite. Also adds .github/workflows/ci.yml. The repo had no CI at all, so the "lint exits 0" invariant was enforced only by discipline; it now runs PHPCS and PHPUnit on every PR. tests/ is deliberately excluded from the PHPCS scan -- the doubles must declare unprefixed global names, which PrefixAllGlobals forbids by design. composer run lint still exits 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…time GitHub warned on the first CI run that actions/checkout@v4 and the actions/cache pulled in by ramsey/composer-install@v3 target Node.js 20, which is deprecated and already being force-run on Node 24. actions/checkout v4 -> v7, ramsey/composer-install v3 -> v4. Neither is breaking here: checkout v7's changes are unrelated to this usage, and composer-install v4's only breaking change is that its bundled actions/cache v5 needs Actions Runner 2.327.1+, which GitHub-hosted ubuntu-latest satisfies. Noted in the workflow header in case these jobs ever move to a self-hosted runner. shivammathur/setup-php stays on v2 -- that is still its current major. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
Closes the No unit tests item in ROADMAP.md's Deferred / not planned section, and adds the CI workflow the repo has been missing.
Why
The motivation is a specific defect class, not generic coverage. Both shortcode callbacks build and return markup rather than echoing it, and PHPCS's
WordPress.Security.EscapeOutputsniff only inspectsecho/print— so an unescaped value concatenated into a returned string lints perfectly clean. ROADMAP item 4 says so outright:That is a permanent blind spot in the only automated check this repo had. It is also exactly the bug that shipped once already.
Separately: there was no CI workflow at all.
.github/held one file, a release caller. Nothing rancomposer run linton a PR, so the CLAUDE.md invariant "lint exits 0, keep it that way" was enforced only by discipline.What's here
22 tests, 38 assertions, ~10ms. PHPUnit
^12.5is the only new dev dependency. Tests run against hand-written WordPress/ACF doubles intests/wp-stubs.php— no WordPress install, no database, no ACF Pro.Coverage, in rough order of value:
ShortcodeEscapingTest) — both the single and archive paths, guarding ROADMAP item 4wp_reset_postdata()reachability (ShortcodeArchiveTest) — guarding ROADMAP item 2AdminStylesTest) — guarding ROADMAP item 1RegistrationTest) — shortcode tags, filters, admin hooks, themanage_optionsgate, and the two hard-coded settings-slug literals that must stay in sync acrosssettings.phpandgeneral.phpAcfJsonPathsTest) — including a test pinning theunset( $paths[0] )gapped-array quirk, so changing it is deliberate.github/workflows/ci.ymlruns PHPCS and PHPUnit (PHP 8.3 / 8.4) on every PR.Verification
composer run check(lint + test) passes end to end.Every test was mutation-checked — a test that still passes with the escaper deleted is worthless. All 8 mutations were caught:
esc_html()removed, single loopwp_kses_post()removed, single loopesc_html()removed, archive titlewp_kses_post()removed, archive loopwp_reset_postdata()made unreachablelib/functions/WP_Queryargsthe_row()dropped from loopcomposer run lintstill exits 0.Reviewer notes
esc_html()is a faithful double;wp_kses_post()is a deliberate pass-through spy. The real KSES allowlist is not reproducible outside WordPress, and reproducing it is not the point — the regression to catch is "the value was concatenated without going through an escaper at all". The asymmetry means the two regressions fail different assertions. A green suite is not proof of XSS safety, and this is documented at the definition, in CLAUDE.md, and in ROADMAP.md.have_rows()/the_row()are a stateful iterator pair.have_rows()peeks and never advances; onlythe_row()moves the cursor. A double returning a constanttrueinfinite-loops — there is a 1000-call tripwire that throws instead of hanging CI.Why not Brain Monkey, the usual choice: the cursor double has to be hand-written either way, so it adds no leverage where the difficulty is, and it handles this plugin's include-time hook registration badly (it resets its registry per test). It stays the natural upgrade path if a
src/with classes ever appears.Why not integration tests: the CPT and repeater come from ACF Pro, a paid non-public dependency — CI would need a license key. The escape hatch means faking ACF anyway, with a WP install and MySQL around it.
tests/is deliberately excluded from the PHPCS scan. The doubles must declare unprefixed global names (have_rows,esc_html,class WP_Query) to stand in for the real functions, which is whatPrefixAllGlobalsforbids by design. Linting them would need ~6 sniff exclusions for ~110 lines of scaffolding. There's a comment in the ruleset recording this.Things to look at deliberately
composer.lockbumps WPCS 3.1.0 → 3.4.1 (plus phpcsextra/phpcsutils) as an incidental effect ofcomposer update. Lint still exits 0 at the new version, so I left it — but it wasn't part of the intent.shortcodes.phpcarries aVersion: 0.1.0header; it doesn't — that was removed in 78a2047.Follow-ups (filed, not in this PR)
Requires PHP: 7.0contradicts the: voidreturn type ingeneral.php(PHP 7.1+); the plugin fatals on 7.0 today. Kept separate because enabling PHPCompatibility may surface a batch of new lint findings.src/PSR-4 mapping. Untouched here.🤖 Generated with Claude Code