From a1f6e54f9d59c54edbd711af72157641130a2f4a Mon Sep 17 00:00:00 2001 From: kristianwind Date: Wed, 16 Sep 2026 13:18:22 +0200 Subject: [PATCH] The smoke test could not fail, and other people's eyes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four fixes, two of them serious, three of them found by the session porting this into subscriptions. 1. THE SMOKE TEST COULD NOT FAIL. `wp eval-file` runs the file through eval() INSIDE A METHOD, so the top level is function scope. `$wc_edm_failures = 0` at the top and `global $wc_edm_failures` in the helper were two different variables: the helpers incremented the real global, the verdict read the local, and the local was always zero. Measured rather than reasoned. A deliberately false assertion added to the unmodified file: FAIL DELIBERATE FAILURE — this run must go red All assertions passed on WordPress 7.1 with WooCommerce 11.1.0. EXIT=0 and with the fix, the same forced failure: FAIL DELIBERATE FAILURE — this run must go red 1 assertion(s) failed on WordPress 7.1. EXIT=1 The FAIL lines always printed, so failures found by reading the output were real finds. What was broken is the CI gate: a regression on either forge would have gone green. Four lines at the top now prove the counter reaches the verdict before anything relies on it — this is the one file whose whole job is to be able to go red, so it checks that it can. 2. `git ls-files` QUOTES NON-ASCII PATHS. "TESTMILJØ.md" comes back as the literal string "TESTMILJ\303\230.md", which matches no pattern in .distignore, so the file is kept — an internal document walking into the release zip with nothing said. Fixed in ci/dist-files.sh and in ci/static.sh's trailing-?> check, which had the same bug and would have silently skipped the file it exists to check. Verified by adding such a filename and watching it be excluded. `done < <(...)` rather than a pipe, or the loop runs in a subshell and the failure counter stops incrementing. 3. THE FEED CACHE WAS SIX HOURS, and Kristian hit it: a release published and his shop shown nothing. The reasoning ("WordPress checks twice a day") was wrong. WordPress gates its own check by screen in wp_update_plugins() — one minute on Dashboard → Updates, one hour on Plugins, 2–12 hours in the background — so WordPress is already the rate limiter and a longer cache only hides releases underneath it. Fifteen minutes now, pinned by an assertion. 4. The wp_update_plugins() assertion made a real HTTP request, because the force-check tests had just deleted the transient. It passed only because this feed repo is public, which made it measure the environment rather than the code. Re-primed first. ci_in_container() replaces the inline docker plumbing in ci_php(), taken from the subscriptions port: the script goes in as a file rather than interpolated into `sh -c`, and --entrypoint sh matters for images whose entrypoint swallows its arguments. Co-Authored-By: Claude Opus 5 (1M context) --- ci/dist-files.sh | 9 +++++- ci/lib.sh | 48 ++++++++++++++++++++--------- ci/static.sh | 6 ++-- docs/REUSE.md | 31 ++++++++++++++++--- docs/TESTING-AND-RELEASES.md | 20 +++++++++--- includes/class-updater.php | 20 ++++++++++-- tests/smoke.php | 60 +++++++++++++++++++++++++++++++----- 7 files changed, 156 insertions(+), 38 deletions(-) diff --git a/ci/dist-files.sh b/ci/dist-files.sh index 64836e6..22be557 100755 --- a/ci/dist-files.sh +++ b/ci/dist-files.sh @@ -18,7 +18,14 @@ while IFS= read -r line; do [ -n "$line" ] && patterns+=("${line%/}") done < .distignore -git ls-files | while IFS= read -r file; do +# -z, not plain `git ls-files`: git QUOTES any path containing a non-ASCII +# byte, so "TESTMILJØ.md" comes out as the literal 14-character string +# "TESTMILJ\303\230.md". That form matches no pattern in .distignore, so the +# file is kept — an internal document walking straight into the release zip +# with nothing said. (-z also covers paths with spaces.) Found in a sibling +# plugin that has exactly such a filename; this repo has none today, which is +# precisely why it would have sat here until someone added one. +git ls-files -z | while IFS= read -r -d '' file; do keep=1 for p in "${patterns[@]}"; do # A bare name excludes the file or directory wherever it appears, which is diff --git a/ci/lib.sh b/ci/lib.sh index d34d3c1..1edb45a 100644 --- a/ci/lib.sh +++ b/ci/lib.sh @@ -46,33 +46,51 @@ ci_plugin_header() { "$(ci_repo_root)/$PLUGIN_MAIN" | head -1 | sed -E 's/[[:space:]]+$//' } +# Run a shell script against a checkout of this repository, inside a container. +# +# The repository goes in over a tarball and `docker cp`, never a bind mount: a +# Gitea Actions job runs inside a container talking to the HOST's docker +# daemon, so a host path here lands on an empty directory nobody created and +# the check passes by checking nothing. The symptom reads like a typo in a path. +# +# Two details are load-bearing. The script is copied in as a FILE rather than +# interpolated into `sh -c`, so it can contain $, quotes and newlines without +# being mangled — which matters the moment it is more than one command. And +# --entrypoint sh is not decoration: some images (composer:2, for one) have an +# entrypoint that swallows its arguments. +# +# Taken from the subscriptions port, which needed it for composer. +ci_in_container() { # image script-text + local image="$1" script="$2" + local dir cid code=0 + + dir=$(mktemp -d -t wpci-XXXXXX) + ( cd "$(ci_repo_root)" && git ls-files -z | tar --null -T - -cf "$dir/src.tar" ) + printf '%s\n' "$script" > "$dir/run.sh" + + cid=$(docker create --entrypoint sh -w /src "$image" \ + -c 'mkdir -p /src && tar -xf /tmp/src.tar -C /src && exec sh /tmp/run.sh') + docker cp "$dir/src.tar" "$cid:/tmp/src.tar" >/dev/null + docker cp "$dir/run.sh" "$cid:/tmp/run.sh" >/dev/null + docker start -a "$cid" || code=$? + docker rm -f "$cid" >/dev/null + rm -rf "$dir" + return "$code" +} + # Run one of this repository's PHP scripts. # # Uses the local php when there is one — a laptop, GitHub's hosted runners — # and a container otherwise. The Gitea runner image has no php, and losing a # build to "php: command not found" after everything else passed is a silly way # to spend an afternoon. Measured: it happened on the very first Gitea run. -# -# The repository goes in over a tarball and `docker cp`, never a bind mount: a -# Gitea Actions job runs inside a container talking to the HOST's docker -# daemon, so a host path here lands on an empty directory nobody created. ci_php() { if command -v php >/dev/null 2>&1; then php "$@" return fi - local tarball cid code=0 - tarball=$(mktemp -t wpci-php-XXXXXX.tar) - ( cd "$(ci_repo_root)" && git ls-files -z | tar --null -T - -cf "$tarball" ) - - cid=$(docker create "php:8.4-cli" sh -c \ - "mkdir -p /src && tar -xf /tmp/src.tar -C /src && cd /src && exec php $(printf '%q ' "$@")") - docker cp "$tarball" "$cid:/tmp/src.tar" >/dev/null - docker start -a "$cid" || code=$? - docker rm -f "$cid" >/dev/null - rm -f "$tarball" - return "$code" + ci_in_container "php:8.4-cli" "exec php $(printf '%q ' "$@")" } ci_step() { printf '\n\033[1m→ %s\033[0m\n' "$*"; } diff --git a/ci/static.sh b/ci/static.sh index bd001ef..9cb3bb0 100755 --- a/ci/static.sh +++ b/ci/static.sh @@ -76,11 +76,13 @@ before=$fails if git ls-files -z '*.php' | xargs -0 grep -nP '<\?(?!php|=|xml)' ; then note_fail "Use " ] \ && { ci_annotate error "$f" 1 "Remove the trailing ?> at the end of the file."; fails=$((fails + 1)); } -done <<< "$(git ls-files '*.php')" +done < <(git ls-files -z '*.php') [ "$fails" = "$before" ] && echo " clean" ci_step "text domain" diff --git a/docs/REUSE.md b/docs/REUSE.md index f3df3cd..bc93110 100644 --- a/docs/REUSE.md +++ b/docs/REUSE.md @@ -71,11 +71,14 @@ Three things in that file are load-bearing and none of them look it: WordPress 5.8. Remove the header and a .org plugin that happens to share the slug could be pushed onto a shop running yours. -The answer is cached for six hours, which is right for the check WordPress runs -twice a day and wrong for the **Check again** link — so `forget_on_force_check()` -drops the cache on `force-check`. Without it, someone who has just published a -release clicks that link, sees nothing, and concludes the whole mechanism is -broken. +🔴 **Keep the feed cache short — WordPress is already the rate limiter.** It +gates its own check by screen: one minute on Dashboard → Updates, one hour on +the Plugins screen, 2–12 hours in the background. This filter only runs when +that check runs, so any cache longer than that cadence simply hides new +releases underneath it. Ours was six hours "because WordPress checks twice a +day anyway", and the symptom was a release going out and the shop being shown +nothing. It is fifteen minutes now, and `forget_on_force_check()` drops it +entirely on the **Check again** link. **6. Make the Gitea repository public**, or point `Update URI` at a separate public releases-only repository. Every shop reads that feed without @@ -105,6 +108,24 @@ it cannot. - `ci/workflows-in-sync.sh` — fails when the two workflow roots drift, which is the only way to notice that a workflow stopped running on Gitea. +## Running these locally: `git add` first + +Every one of these scripts derives "what ships" from `git ls-files`, which +reads the **index**. An uncommitted file is therefore invisible to +`ci/dist-files.sh`, to the release zip, and to the copy that goes into the test +site. + +That is deliberate — it is what catches a file that was written, referenced and +never committed, which is a release that is missing a file while every local +test passes because the file is sitting right there in the working tree. But it +also means running any of this before `git add` tests a release without your +newest file, and the failure is silent: the check does not complain, it just +measures something else. It cost two rounds here, once on +`includes/class-updater.php` and once on `assets/icon.svg`. + +If a script reports something you just fixed as still missing, `git status` +before you debug the script. + ## Two guards worth understanding before you move them **`.gitea/workflows/` hides `.github/workflows/` completely.** Gitea uses the diff --git a/docs/TESTING-AND-RELEASES.md b/docs/TESTING-AND-RELEASES.md index 8417b27..aac1b95 100644 --- a/docs/TESTING-AND-RELEASES.md +++ b/docs/TESTING-AND-RELEASES.md @@ -185,11 +185,21 @@ the real list table and asserts the flag rather than trusting the reasoning. ### "Check again" really checks again -The feed is cached for six hours. That is right for the check WordPress runs on -its own twice a day, and wrong for a link whose entire promise is that it looks -*now* — so the cache is dropped when wp-admin is loaded with `force-check`. -Without that, publishing a release and then clicking **Check again** shows -nothing for up to six hours, which reads exactly like a broken updater. +The feed is cached for fifteen minutes, and the cache is dropped entirely when +wp-admin is loaded with `force-check`. + +It was six hours, on the reasoning that WordPress checks twice a day anyway. +That reasoning was wrong. WordPress gates its own check **by screen**: + +| Screen | How often WordPress re-checks | +| --- | --- | +| Dashboard → Updates | every minute | +| Plugins | every hour | +| Background / cron | every 2–12 hours | + +This filter only runs when that check runs, so WordPress is already the rate +limiter. A cache longer than its cadence does nothing but hide new releases +underneath it — a release published, and a shop shown nothing. From the command line, `wp transient delete wc_edm_release_feed` does the same. diff --git a/includes/class-updater.php b/includes/class-updater.php index a2f09b5..94c0c0b 100644 --- a/includes/class-updater.php +++ b/includes/class-updater.php @@ -34,10 +34,24 @@ class WC_EDM_Updater { const TRANSIENT = 'wc_edm_release_feed'; /** - * How long that cache lives. WordPress checks for updates twice a day; a - * shorter window would only add requests, a longer one would delay a fix. + * How long that cache lives. + * + * Six hours, originally, on the reasoning that WordPress checks twice a + * day anyway. That reasoning was wrong, and the symptom was a shop that + * had just been told a new version existed and was shown nothing. + * + * WordPress does not check twice a day when somebody is looking. It gates + * its own check by screen — one minute on Dashboard → Updates, one hour on + * the Plugins screen, twelve hours in the background (wp_update_plugins(), + * switch on current_filter()). This filter only runs when that check runs, + * so WordPress is already the rate limiter, and a cache longer than its + * cadence does nothing but hide new releases underneath it. + * + * Fifteen minutes is left only to absorb a burst — several calls in one + * request, or other code calling wp_update_plugins() with extra stats, + * which bypasses the gate above. */ - const TTL = 6 * HOUR_IN_SECONDS; + const TTL = 15 * MINUTE_IN_SECONDS; /** * Hook into the update check for our own host only. diff --git a/tests/smoke.php b/tests/smoke.php index 0f26fc5..23f0a69 100644 --- a/tests/smoke.php +++ b/tests/smoke.php @@ -14,7 +14,29 @@ if ( ! defined( 'ABSPATH' ) ) { exit; } -$wc_edm_failures = 0; +/* + * The counter lives in $GLOBALS on purpose, and the four lines below prove it + * reaches the verdict before anything relies on it. + * + * `wp eval-file` runs this file through eval() INSIDE A METHOD, so the top + * level here is function scope, not global scope. A plain `$failures = 0` at + * the top and a `global $failures` in a helper below are therefore two + * different variables: the helpers increment the real global, the verdict at + * the bottom reads the local, and the local is always zero. Every assertion + * fails, every FAIL line prints, the file ends with "All assertions passed" + * and exits 0 — and CI goes green. That is not reasoning, it is what this file + * did until it was measured. + * + * This is the one file whose entire job is to be able to go red, so it checks + * that it can. + */ +$GLOBALS['wc_edm_failures'] = 0; +( static function () { ++$GLOBALS['wc_edm_failures']; } )(); +if ( 1 !== $GLOBALS['wc_edm_failures'] ) { + echo "The failure counter does not reach the verdict: this file cannot go red.\n"; + exit( 1 ); +} +$GLOBALS['wc_edm_failures'] = 0; /** * Assert a condition and keep going, so one run reports every failure. @@ -24,14 +46,12 @@ $wc_edm_failures = 0; * @param string $detail Shown when it fails. */ function wc_edm_ok( $what, $condition, $detail = '' ) { - global $wc_edm_failures; - if ( $condition ) { printf( " ok %s\n", $what ); return; } - ++$wc_edm_failures; + ++$GLOBALS['wc_edm_failures']; printf( " FAIL %s%s\n", $what, '' !== $detail ? " — {$detail}" : '' ); } @@ -345,7 +365,18 @@ wc_edm_is( 'the current version is answered too, not passed over', WC_EDM_VERSIO * WordPress does with our answer, and a test that needs the internet fails for * reasons that have nothing to do with it. */ -// "Check again" must discard the six-hour cache, or the button lies: someone +// The cache must not outlive WordPress's own check cadence. It gates by screen +// — one minute on Dashboard → Updates, one hour on the Plugins screen — and +// this filter only runs when that check runs, so anything longer here hides a +// new release underneath a cache the shop owner cannot see. This was six hours +// and produced exactly that: a release published, and a shop shown nothing. +wc_edm_ok( + 'the cache is shorter than the interval WordPress re-checks on', + WC_EDM_Updater::TTL <= 15 * MINUTE_IN_SECONDS, + 'TTL is ' . WC_EDM_Updater::TTL . 's; WordPress re-checks every 60s on Dashboard → Updates' +); + +// "Check again" must discard the cache, or the button lies: someone // who has just published a release clicks it, sees nothing, and concludes the // whole mechanism is broken. set_transient( WC_EDM_Updater::TRANSIENT, array( 'version' => '1.0.0', 'package' => 'x', 'url' => 'x', 'tested' => '' ), 600 ); @@ -368,6 +399,21 @@ delete_transient( WC_EDM_Updater::TRANSIENT ); // strlen(null) deprecation from class-wp-plugins-list-table.php. It is // WordPress's, not ours, which is why the deprecation check filters on this // plugin's directory rather than failing on everything it sees. +// Primed again deliberately. The force-check assertions above deleted it, and +// without this offer_update() makes a real HTTP request — which passes only +// because this plugin's feed repo happens to be public. That would make the +// assertion measure the environment rather than the code under test. +set_transient( + WC_EDM_Updater::TRANSIENT, + array( + 'version' => WC_EDM_VERSION, + 'package' => 'https://gitea.nolimit.dk/kw/demo/releases/download/v1/demo.zip', + 'url' => 'https://gitea.nolimit.dk/kw/demo', + 'tested' => '7.1', + ), + 600 +); + delete_site_transient( 'update_plugins' ); wp_update_plugins(); $transient = get_site_transient( 'update_plugins' ); @@ -407,8 +453,8 @@ delete_transient( WC_EDM_Updater::TRANSIENT ); /* --------------------------------------------------------------- verdict -- */ echo "\n"; -if ( $wc_edm_failures > 0 ) { - printf( "%d assertion(s) failed on WordPress %s.\n", $wc_edm_failures, get_bloginfo( 'version' ) ); +if ( $GLOBALS['wc_edm_failures'] > 0 ) { + printf( "%d assertion(s) failed on WordPress %s.\n", $GLOBALS['wc_edm_failures'], get_bloginfo( 'version' ) ); exit( 1 ); } printf(