Skip to content

[WOOTAX-292] Tweak - Modernize PHPUnit test-environment installation#2957

Open
bartech wants to merge 11 commits into
trunkfrom
wootax-292-modernize-phpunit-test-env-installation-port-shipstation
Open

[WOOTAX-292] Tweak - Modernize PHPUnit test-environment installation#2957
bartech wants to merge 11 commits into
trunkfrom
wootax-292-modernize-phpunit-test-env-installation-port-shipstation

Conversation

@bartech

@bartech bartech commented May 29, 2026

Copy link
Copy Markdown
Collaborator

Description

composer test (the PHPUnit integration suite) could not run in a fresh shell. Two root causes:

  1. Cross-plugin /tmp drift. tests/php/bootstrap.php resolved the WC root from a hard-coded /tmp/woocommerce/... (ignoring TMPDIR), and a stale wp-tests-config.php left behind by a sibling shipping plugin baked in the wrong DB port — so the suite pointed at a database that wasn't running.
  2. Required a running DB + a local mysql client. There was no tests/docker-compose.yml / tests/test.env, and the old tests/bin/install-wc-tests.sh assumed a pre-existing DB_HOST and shelled out to a host mysqladmin.

This ports the proven, resilient test-env pattern from the shipping plugins (woocommerce-shipping-usps#550, woocommerce-shipstation#370/#380) into this repo, adapted for the composer-driven (non-pnpm) workflow.

What changed:

  • Dockerized test DBtests/docker-compose.yml (MariaDB on 127.0.0.1:4416, adminer on 8062) + tests/test.env. An explicit Compose project name (wcservices_test) isolates this plugin so a down --remove-orphans no longer tears down a sibling plugin's DB container.
  • tests/bin/test-config.sh — single source of truth for DB params (reads test.env + the published compose port).
  • tests/bin/check-test-env.sh — cheap preflight probes that dispatch a granular repair: only the broken component is rebuilt. The cross-plugin port drift is fixed with a single sed — no downloads.
  • tests/bin/install-wc-tests.sh (rewritten) — namespaced/TMPDIR-aware paths, docker DB lifecycle, on-disk mysql/mysqladmin shims that proxy through the container (so no local mysql client is required), idempotent CREATE DATABASE IF NOT EXISTS + skip-database-creation=true, per-component REPAIR_* flags, and --force. Uses php bin/generate-feature-config.php (not pnpm).
  • tests/bin/run-tests.sh — health-check + repair, then PHPUnit.
  • tests/php/bootstrap.php — the WC-root fallback now resolves under the system temp dir (honors TMPDIR) so plain composer test finds the installer's path.
  • composer.jsoncomposer test now runs the preflight + PHPUnit; added composer test:setup and composer test:check.
  • AGENTS.md — documents the one-command flow.

No production code (classes/, src/) is touched; composer.lock is unchanged; no Node/toolchain pin bump.

Related issue(s)

Closes WOOTAX-292.

Steps to reproduce & screenshots/GIFs

On a machine with Docker but no host mysql client, with no /tmp/wordpress*:

composer test

The command brings the DB container up, installs WP + WC test deps, bootstraps the schema, and runs the suite:

Time: 00:30.342, Memory: 125.00 MB
OK (211 tests, 468 assertions)

Verified scenarios:

  • Clean install → full suite green (211 tests, 468 assertions).
  • Corrupting the DB port in wp-tests-config.php → preflight repairs only WP_CONFIG (single sed, no downloads), suite green.
  • bash tests/bin/install-wc-tests.sh --force → clean full reinstall, green.
  • Running back-to-back with a sibling plugin no longer clobbers its DB container (isolated Compose project name).

Checklist

  • unit tests
  • changelog.txt entry added
  • readme.txt entry added

@bartech bartech self-assigned this May 29, 2026

@Abdalsalaam Abdalsalaam left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤖 Automated AI pre-flight review — generated by Claude Code (ultra-review skill) on behalf of @Abdalsalaam.
This is NOT an approval and not a substitute for human review; it is an early automated pass to surface likely issues. Treat every finding as a suggestion to verify.


Findings

Blockers (must fix before merge)

None that meet the strict bar (no production/CI/data-loss/security-in-prod impact). Note: two reviewers rated the exit-code-masking issue below as a BLOCKER; it is downgraded here only because CI — the actual merge gate — bypasses this script. It remains the #1 fix.

Majors (should fix before merge)

M1 — composer test reports success even when PHPUnit fails (exit-code masking)
tests/bin/run-tests.sh (final pipeline):

"$PROJECT_ROOT/vendor/bin/phpunit" --testdox "$@" 2>&1 \
    | grep -Ev "^(PHP Deprecated|PHP Notice|PHP Warning|Xdebug)" \
    || true

set -e is on but set -o pipefail is not, so the pipeline's status is grep's. grep -Ev exits 0 whenever it prints ≥1 line (always true for real PHPUnit output), and || true then discards even that. A failing suite — test failures, fatals, even a segfault — makes composer test exit 0. This directly defeats the PR's purpose: a developer (or any wrapper/pre-commit hook) is told "green" on red.
Fix: set -o pipefail after set -e, drop || true, and propagate the real status, e.g.:

set -o pipefail
"$PROJECT_ROOT/vendor/bin/phpunit" --testdox "$@" 2>&1 \
  | grep -Ev "^(PHP Deprecated|PHP Notice|PHP Warning|Xdebug)" || true
exit "${PIPESTATUS[0]}"

(Confirmed by Correctness, Silent-failure, and Conventions reviewers.)

M2 — Local suite is non-reproducible: WC_VERSION defaults to latest
tests/bin/install-wc-tests.sh:

WC_VERSION=${WC_VERSION:-latest}

The old installer pinned ${6-"10.3.4"}; CI tests a pinned matrix of the 3 latest WC minors. Defaulting local to latest means a fresh composer test can clone a different WC than CI, so local green/red can diverge from CI green/red (and may pull a WC requiring a newer PHP than the contributor has). Fix: pin a known-good WC_VERSION (ideally one CI also exercises), or document/require it explicitly.

M3 — mysql/mysqladmin shim planted in a predictable shared-/tmp path on PATH
tests/bin/install-wc-tests.sh:

SHIM_DIR="${TEST_TMP_DIR}/wcservices-mysql-shim"
mkdir -p "$SHIM_DIR"
...
export PATH="$SHIM_DIR:$PATH"

SHIM_DIR is a fixed name under shared /tmp, created with default umask and prepended to PATH. On a multi-user host, another user can pre-create/own /tmp/wcservices-mysql-shim and plant a malicious mysql/mysqladmin that then executes in the developer's shell. Proportionate (dev-only, common single-user case is fine), but a genuine local code-execution vector. Fix: use mktemp -d (or verify the dir is owned by the current uid and chmod 700 it; bail if pre-owned by another user).

Minors (nice to fix)

  • No re-verification after a granular repair. run-tests.sh calls check-test-env.sh, which execs the installer; on return PHPUnit runs with no second probe. A partial repair leaves a broken env that M1 then hides. Fix: re-run the health check after repair and fail non-zero if still unhealthy. (Mostly mitigated once M1 is fixed.)
  • Over-broad output filter. The grep -Ev "^(PHP Deprecated|PHP Notice|PHP Warning|...)" drops every such line, including a PHP Warning that precedes a fatal. Never filter PHP Fatal / PHP Parse error. Tie this to the M1 fix.
  • Cleartext WP version-check. download http://api.wordpress.org/core/version-check/1.7/ ... — the parsed version drives a download URL; use https://.
  • MariaDB version drift. Local mariadb:10.11 vs CI mariadb:10.9 vs root docker-compose.yml mariadb:10.5.8. Align local to 10.9 to match CI.
  • DB ports bind 0.0.0.0. tests/docker-compose.yml: - 4416:3306 and - "8062:8080" expose the test DB and an unauthenticated Adminer to the LAN. Bind to loopback: 127.0.0.1:4416:3306, 127.0.0.1:8062:8080 (and consider dropping adminer from the default stack).
  • DB password on argv. --password="$TEST_DB_PASS" in drop_test_db/ensure_test_db/wait_for_db is visible via ps//proc and triggers mysql's own cleartext warning. Prefer MYSQL_PWD or a defaults-extra-file. (Throwaway cred, low impact.)
  • No download integrity checks. WP zip / WC clone are fetched over https but never checksum/SHA-verified. Dev-only; acceptable, but worth a note.
  • Port-parse fragility. grep -oE '[0-9]+:3306' | head -1 relies on test_db appearing first in the compose file. Scope the grep to test_db's ports: block.
  • Orphan volume. tests/docker-compose.yml declares volumes: dockerdirectory: but no service mounts it; MariaDB data isn't persisted to a named volume. Remove it, or mount it at /var/lib/mysql if persistence was intended.
  • docker compose (v2) assumed unconditionally. No presence/version check; contributors on v1 docker-compose or without Docker get cryptic failures. Detect/alias, or fail early with a clear message.
  • Errors swallowed by 2>/dev/null || true. down -v --remove-orphans and drop_test_db hide daemon-down / auth failures; the real cause only surfaces (less clearly) at the next step. Keep || true for the "already absent" case; let connection/daemon errors propagate.
  • AGENTS.md gaps. New composer test:check is undocumented; no escape hatch documented for contributors without Docker (they can still run phpunit -c phpunit.xml.dist after manual setup, as CI does).

Nits

  • macOS sed -i .bak leaves *.bak litter in $WP_TESTS_DIR (install_wp_test_suite, repair_wp_config). Add rm -f "${cfg}.bak" after the Darwin branch.
  • version: '3' in tests/docker-compose.yml is obsolete under Compose v2 (deprecation warning). Drop the key. (Pre-existing in the root compose file too.)
  • Clone-fallback error reporting is opaque — the ERR trap reports a line number, not a cause. Splitting run_quiet ... || run_quiet ... into if ! ...; then ...; fi reads clearer (behavior is already correct).
  • /dev/tcp bashism in the DB probe — fine under the #!/usr/bin/env bash shebang; would break only if run via sh.
  • bootstrap.php temp-dir edge: sys_get_temp_dir() honors TMP/TEMP too, while test-config.sh uses ${TMPDIR:-/tmp} — they can diverge only if TMP/TEMP is set while TMPDIR is unset (rare on Unix; CI and macOS both resolve consistently).

bartech and others added 2 commits July 1, 2026 13:51
Addresses the WOOTAX-292 review (PR #2957):
- run-tests.sh: propagate PHPUnit's real exit code via pipefail + PIPESTATUS so
  `composer test` fails on red instead of masking it; add a TESTDOX=1/0/auto
  output toggle (compact when captured, --testdox at an interactive terminal).
- install-wc-tests.sh: create the mysql/mysqladmin shim with `mktemp -d` to
  avoid a shared-/tmp PATH hijack; fetch the WP version-check over https; remove
  macOS sed .bak files with if/fi (a `[[ ]] && rm` tail returned non-zero on
  Linux and aborted the repair dispatch).
- docker-compose.yml: bind the DB and Adminer to 127.0.0.1, align MariaDB to
  CI's 10.9, drop the obsolete `version:` key and the unused named volume.
- AGENTS.md: document the WC_VERSION/EXPECTED_WP_VERSION override and TESTDOX.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-phpunit-test-env-installation-port-shipstation

# Conflicts:
#	AGENTS.md
@bartech bartech requested a review from Abdalsalaam July 1, 2026 12:17
Comment thread tests/bin/check-test-env.sh Outdated
Comment thread tests/bin/install-wc-tests.sh Outdated
Comment thread tests/bin/install-wc-tests.sh Outdated
bartech added 2 commits July 1, 2026 14:48
Replace the GNU-only `\s` regex shorthand with the POSIX `[[:space:]]`
class in the wp-tests-config.php DB_HOST health probe and sed repair.
Stock macOS grep/sed (BSD) treat `\s` as a literal 's', so the probe
never matched a healthy config — reporting drift every run and
escalating the cheap sed repair to a full WP reinstall. POSIX classes
behave identically on GNU (Linux/CI) and BSD.
Capture the BSD/macOS vs GNU regex and `sed -i` pitfalls learned from
the DB_HOST probe fix so future shell edits stay cross-platform.
PHPUnit exits 0 even when it runs no tests ("No tests executed!"), so a
broken bootstrap or misconfigured suite would make `composer test` report
green — the same gap #2638 hit in CI. tee the filtered output to a temp log
and fail the run when it reports an empty suite; PHPUnit's real exit code
still wins via PIPESTATUS otherwise. mktemp uses an explicit template for
BSD/macOS portability.
Comment thread tests/bin/install-wc-tests.sh Outdated
Comment thread tests/docker-compose.yml Outdated

@CezaryDrewniak CezaryDrewniak left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM and works as advertised

Reviewed locally, happy path is green (OK (180 tests, 394 assertions)), the granular repair correctly handled a stale DB container + wp-tests-config.php DB_HOST drift with no full reinstall, and the e2f1628/e089c0b exit-code fixes work: --filter=NoSuchTestXyz now correctly surfaces exit=1 with the "PHPUnit executed zero tests" guard.

Two small follow-ups before merge:

  1. tests/bin/install-wc-tests.sh:390{ resolve_wc_version; install_wc; } runs under set -e + ERR trap, so when resolve_wc_version returns 1 (GitHub API rate-limit / offline) the script aborts on the API failure and install_wc's built-in git clone … || git clone … default-branch fallback never gets a chance to run — defeating the self-heal in the exact scenario it was written for. Trivial fix:
    [[ "$REPAIR_WC" == "1" ]] && { resolve_wc_version || true; install_wc; }
  2. tests/docker-compose.yml:20image: adminer is the only unpinned image; please pin to e.g. adminer:4.8.1 to match the discipline now applied to mariadb:10.9 and the SHA-pinned third-party actions.

bartech added 2 commits July 2, 2026 15:53
resolve_wc_version ran under set -e inside the REPAIR_WC dispatch group, so
a GitHub API rate-limit or offline failure killed the script before
install_wc's default-branch clone fallback could run — defeating the
self-heal in the exact scenario it was written for. Make the resolution
best-effort so the fallback clone (not subject to the API rate limit)
takes over.
adminer was the only unpinned image; pin it to 4.8.1 to match the
reproducibility discipline of the mariadb:10.9 pin.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants