From 1f4e2aeba5a5d227dfe7b33bc5197d4d27d2679a Mon Sep 17 00:00:00 2001 From: Mischa Date: Fri, 24 Jul 2026 22:33:46 +0200 Subject: [PATCH] fix(test): close the test-entry leak into real trip content MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A ui-test entry had survived into the active trip's dailies. Three independent failures had to line up for that, and all three were real: 1. cleanupEntry() used host-side fs.rmSync. Grav's Apache workers run as root, so every entry the form creates is root-owned and recursive removal needs write permission on that directory — which the host user lacks. Cleanup had never worked for form-created entries; it just threw inside a path nothing checked. It now falls back to `docker exec … rm -rf` in the container that actually serves USER_DIR. 2. globalTeardown's dailies sweep keyed off a `parent:` in post-form.md — a key deliberately removed (the write target comes from site.yaml active_trip, and CLAUDE.md forbids re-adding a static parent). The regex could never match, so dailiesDir was always null and the sweep silently did nothing. It now reuses helpers' own resolution instead of keeping a divergent copy. 3. Nothing pinned the suite to this checkout's server. playwright.config.js defaults to :8081, so a worktree run hit the MAIN checkout — entries created in one content tree while the specs asserted and cleaned up in another. test-ui now passes GRAV_BASE_URL from GRAV_PORT, and globalSetup hard-fails when the server's bind mount disagrees with the tree the specs read. Also fixed, found on the way to a green run: - test-account interpolated the password into an `sh -c` string, so a password containing a shell metacharacter was re-parsed by the container's shell (`sh: 2: : not found`, no account, every UI run dead). It now travels via `docker exec -e`, making the recipe indifferent to its contents. - `make start` in a worktree always failed: travel-memories declares `env_file: .env` and worktree-new creates none. It degrades to start-grav there — a worktree with no server is what sent runs to :8081 in the first place. - test-form-config asserted a hero_image field that 8cf1145 deliberately removed; it had been failing ever since. Verified: config 22/22, post 6/6, location-override 20/20, and a full UI run now leaves zero ui-test entries behind. The remaining UI failures are pre-existing on main — site.yaml pins owner_username to a real account while the suite logs in as testrunner, so owner-only controls never render for it. Only trip-publish.spec.js patches that; delete-flow, edit-mode and anon-view do not. Left for a separate branch. --- Makefile | 36 +++++++++++- scripts/test-form-config.sh | 5 +- tests/global-setup.js | 57 ++++++++++++++++++ tests/global-teardown.js | 77 ++++++++++--------------- tests/ui/helpers.js | 61 +++++++++++++++++++- tests/ui/post/location-override.spec.js | 31 +++++++++- user | 2 +- 7 files changed, 216 insertions(+), 53 deletions(-) diff --git a/Makefile b/Makefile index 9998605..6153a22 100644 --- a/Makefile +++ b/Makefile @@ -54,9 +54,15 @@ $(foreach t,$(REMOTE_TARGETS),$(foreach e,$(ENVS),$(eval $(call make-env-target, GRAV_TEST_USER ?= testrunner GRAV_TEST_PASS ?= Testpass1234 +# The password is handed to the container through `docker exec -e` (the bare +# form, which forwards the already-exported variable) rather than interpolated +# into the `sh -c` string. Interpolating it meant any shell-special character in +# GRAV_TEST_PASS was re-parsed by the container's shell — a `.env` password +# containing one produced `sh: 2: : not found` and no test account. +# The recipe is now indifferent to the password's contents. test-account: - @docker exec $(GRAV_CONTAINER) sh -c 'test -f /var/www/html/user/accounts/$(GRAV_TEST_USER).yaml \ - || php bin/plugin login new-user -u $(GRAV_TEST_USER) -p "$(GRAV_TEST_PASS)" \ + @docker exec -e GRAV_TEST_PASS $(GRAV_CONTAINER) sh -c 'test -f /var/www/html/user/accounts/$(GRAV_TEST_USER).yaml \ + || php bin/plugin login new-user -u $(GRAV_TEST_USER) -p "$$GRAV_TEST_PASS" \ -e $(GRAV_TEST_USER)@example.test -N "Test Runner" -P b --admin-type both -s enabled -n' test-config: @@ -65,6 +71,13 @@ test-config: test-post: test-account @bash scripts/test-post.sh +# Pinned to THIS checkout's port, not playwright.config.js's :8081 default. In a +# worktree that default silently pointed the suite at the main checkout's server, +# so entries were created in main's user/ while the specs asserted and cleaned up +# in the worktree's — leaving ui-test entries behind in real trip content. +# tests/global-setup.js now also hard-fails on that mismatch. +GRAV_BASE_URL ?= http://localhost:$(GRAV_PORT) + test-ui: test-account @npx playwright test @@ -98,8 +111,18 @@ build-assets: -w /app node:20-alpine \ sh -c "npm install && npm run build" +# In a worktree this degrades to start-grav. The travel-memories service declares +# `env_file: .env`, and worktree-new does not create a .env, so a plain +# `docker compose up -d` there dies with "env file ... not found" — leaving the +# worktree with no server at all, which is how test runs ended up silently +# targeting the main checkout. start: - docker compose up -d + @if [ -f .worktree-env ]; then \ + echo "→ worktree: starting the grav service only (travel-memories needs a .env, which worktrees have none)"; \ + docker compose up -d grav; \ + else \ + docker compose up -d; \ + fi # Grav service only — used by `make worktree-new` (a worktree rarely needs the # travel-memories service, and this keeps its footprint minimal). @@ -185,6 +208,13 @@ demo-load: # Load every fixture trip under docs/demo/trips/ into the pages tree. # Source uses dailies/ + 04.stories/; dailies/ maps to 01.dailies/ on copy. # All copies are `|| true` so a fixture absent from an older user/ is skipped. + # + # ⚠️ A fixture whose folder name matches a REAL trip's slug is copied straight + # over that live page — docs/demo/trips/italy-2025/ collides with the real + # italy-2025 trip on purpose (the fixture supplies its GPX + dailies). So any + # field the fixture's trip.md omits gets silently deleted from real content on + # every test run: it had been dropping the trip's tagline that way. Keep a + # colliding fixture's trip.md byte-identical to the live page. docker exec $(GRAV_CONTAINER) bash -c 'for src in /var/www/html/user/docs/demo/trips/*/; do \ slug=$$(basename "$$src"); dst=/var/www/html/user/pages/01.trips/$$slug; \ mkdir -p "$$dst/01.dailies" "$$dst/04.stories"; \ diff --git a/scripts/test-form-config.sh b/scripts/test-form-config.sh index c8badfd..18490f0 100755 --- a/scripts/test-form-config.sh +++ b/scripts/test-form-config.sh @@ -59,7 +59,10 @@ check_grep "location_country field present" "name: location_country" check_grep "weather_desc field present" "name: weather_desc" check_grep "weather_temp_c field present" "name: weather_temp_c" check_grep "transport_mode field present" "name: transport_mode" -check_grep "hero_image field present" "name: hero_image" +# No hero_image assertion: the field was deliberately dropped in 8cf1145 — +# entries render their hero from the first photo, so an explicit filename was +# redundant (see the comment at that spot in post-form.md). This check outlived +# the field and had been failing ever since. check_grep "force_connect field present" "name: force_connect" check_grep "featured field present" "name: featured" diff --git a/tests/global-setup.js b/tests/global-setup.js index ec598c4..b9c6864 100644 --- a/tests/global-setup.js +++ b/tests/global-setup.js @@ -2,6 +2,58 @@ const fs = require('fs'); const path = require('path'); const { execSync } = require('child_process'); +/** + * Fail fast if the server under test does not serve the `user/` tree the specs + * read from disk. + * + * This mismatch is silent and destructive. Every post spec submits through the + * live form (the write target is derived server-side from site.yaml + * `active_trip`, so there is no per-request override), then asserts and cleans up + * on disk via helpers' USER_DIR. Run the specs from a worktree whose own + * container is down and baseURL falls back to localhost:8081 — the MAIN + * checkout — so entries get created in one content tree while cleanup deletes + * from another. The entries are then left behind in real trip content, which is + * exactly what happened on 2026-07-24. + * + * Docker is the only thing that knows the mapping, so this is best-effort: if we + * cannot determine it we warn and continue rather than blocking non-Docker runs. + * But when we CAN determine it and it disagrees, that is always a bug. + */ +function assertServerServesUserDir(baseURL, userDir) { + const port = new URL(baseURL).port || '80'; + let mountedUserDir; + try { + const container = execSync("docker ps --format '{{.Names}}\t{{.Ports}}'", { encoding: 'utf-8' }) + .split('\n').filter(Boolean) + .find(l => l.includes(`:${port}->`)); + if (!container) { + console.warn(`[setup] no running container publishes port ${port} — is the dev server up? (make start)`); + return; + } + const name = container.split('\t')[0]; + mountedUserDir = execSync( + `docker inspect ${name} --format '{{range .Mounts}}{{if eq .Destination "/var/www/html/user"}}{{.Source}}{{end}}{{end}}'`, + { encoding: 'utf-8', stdio: ['pipe', 'pipe', 'ignore'] } + ).trim(); + if (!mountedUserDir) return; // no bind mount to compare against + } catch (_) { + return; // docker unavailable — nothing to check + } + + const served = fs.realpathSync(mountedUserDir); + const asserted = fs.realpathSync(userDir); + if (served !== asserted) { + throw new Error( + `Test target mismatch — refusing to run.\n` + + ` baseURL ${baseURL} is served from: ${served}\n` + + ` but the specs read/clean up: ${asserted}\n` + + `Entries would be created in one tree and cleanup would miss them, leaving\n` + + `test entries behind in real content. Start this checkout's own server\n` + + `(make start) and point the run at it, e.g. GRAV_BASE_URL=http://localhost:.` + ); + } +} + module.exports = async function globalSetup() { const envFile = path.join(__dirname, '../.env'); if (fs.existsSync(envFile)) { @@ -23,4 +75,9 @@ module.exports = async function globalSetup() { // Ensure demo content is loaded (italy-2026-demo trip + stories + GPX files) execSync('make demo-load', { cwd: path.join(__dirname, '..'), stdio: 'inherit' }); + + // Required last: helpers.js resolves USER_DIR at require time, and the .env + // load above can supply GRAV_USER_DIR. + const { USER_DIR } = require('./ui/helpers'); + assertServerServesUserDir(process.env.GRAV_BASE_URL || 'http://localhost:8081', USER_DIR); }; diff --git a/tests/global-teardown.js b/tests/global-teardown.js index 9441321..ca21f85 100644 --- a/tests/global-teardown.js +++ b/tests/global-teardown.js @@ -1,57 +1,44 @@ const fs = require('fs'); const path = require('path'); -const { execSync } = require('child_process'); -function resolveUserDir() { - if (process.env.GRAV_USER_DIR) return process.env.GRAV_USER_DIR; - try { - const raw = execSync( - "docker inspect intotheeast_grav --format '{{range .Mounts}}{{if eq .Destination \"/var/www/html/user\"}}{{.Source}}{{end}}{{end}}'", - { encoding: 'utf-8', stdio: ['pipe', 'pipe', 'ignore'] } - ).trim(); - if (raw) return raw; - } catch (_) {} - return path.join(__dirname, '../user'); -} +// Reuse the specs' own resolution rather than reimplementing it. The previous +// version of this file derived the dailies directory from a `parent:` key in +// pages/02.post/post-form.md — a key that was deliberately removed (the write +// target is injected server-side from site.yaml `active_trip`, and CLAUDE.md +// forbids re-adding a static parent). The regex therefore never matched, +// dailiesDir was always null, and the dailies sweep below silently did nothing. +// That is how ui-test entries survived into the active trip's content. +// removeEntryDir handles the root-owned case by deleting through the container — +// see its comment. Plain fs.rmSync cannot remove what Grav's Apache wrote. +const { USER_DIR, TRACKER_DIR, removeEntryDir } = require('./ui/helpers'); function sweepUiTestEntries(dir) { - if (!fs.existsSync(dir)) return 0; - const entries = fs.readdirSync(dir).filter(e => e.includes('ui-test')); - entries.forEach(e => fs.rmSync(path.join(dir, e), { recursive: true, force: true })); - return entries.length; + if (!dir || !fs.existsSync(dir)) return 0; + const found = fs.readdirSync(dir).filter(e => e.includes('ui-test')); + let removed = 0; + found.forEach(e => { + const target = path.join(dir, e); + try { + removeEntryDir(target); + removed++; + } catch (err) { + // Loud, not silent — a swallowed failure here is exactly what let a + // ui-test entry survive into the active trip's content. + console.error(`[teardown] COULD NOT REMOVE ${target}: ${err.message}`); + } + }); + return removed; } module.exports = async function globalTeardown() { - const userDir = resolveUserDir(); - - // Read active trip slug from post-form.md - const postFormPath = path.join(userDir, 'pages/02.post/post-form.md'); - let dailiesDir = null; - if (fs.existsSync(postFormPath)) { - const content = fs.readFileSync(postFormPath, 'utf-8'); - const m = content.match(/parent:\s*['"]?\/trips\/([^/'"]+)\/dailies/); - if (m) { - const tripSlug = m[1]; - const tripsBase = path.join(userDir, 'pages/01.trips'); - const tripFolder = fs.readdirSync(tripsBase).find( - f => f === tripSlug || f.endsWith('.' + tripSlug) || f.includes(tripSlug) - ); - if (tripFolder) { - const dailiesBase = path.join(tripsBase, tripFolder); - const dailiesFolder = fs.readdirSync(dailiesBase).find( - f => f === 'dailies' || f === '01.dailies' || f.endsWith('.dailies') - ); - if (dailiesFolder) dailiesDir = path.join(dailiesBase, dailiesFolder); - } - } - } - - // Sweep both the post inbox and the active trip's dailies - const postInbox = path.join(userDir, 'pages/02.post'); - const n1 = sweepUiTestEntries(postInbox); - const n2 = dailiesDir ? sweepUiTestEntries(dailiesDir) : 0; + // Sweep both the post inbox and the active trip's dailies. + const n1 = sweepUiTestEntries(path.join(USER_DIR, 'pages/02.post')); + const n2 = sweepUiTestEntries(TRACKER_DIR); if (n1 + n2 > 0) { - console.log(`[teardown] removed ${n1} ui-test entries from 02.post, ${n2} from dailies`); + console.log( + `[teardown] removed ${n1} ui-test entries from 02.post, ` + + `${n2} from ${path.relative(USER_DIR, TRACKER_DIR)}` + ); } }; diff --git a/tests/ui/helpers.js b/tests/ui/helpers.js index a825606..c11ec09 100644 --- a/tests/ui/helpers.js +++ b/tests/ui/helpers.js @@ -170,6 +170,60 @@ async function createPhotoEntry(page, tag, { content, publish = true, created } 'Entry posted successfully!', { timeout: 15_000 }); } +/** + * Resolve the Grav container that serves USER_DIR, so cleanup can delete as root. + * Prefers GRAV_CONTAINER (set by .worktree-env / .env), else matches on the bind + * mount so a worktree never picks the main checkout's container. + */ +function resolveGravContainer() { + if (process.env.GRAV_CONTAINER) return process.env.GRAV_CONTAINER; + try { + const want = fs.realpathSync(USER_DIR); + const names = execSync("docker ps --format '{{.Names}}'", { encoding: 'utf-8', stdio: ['pipe', 'pipe', 'ignore'] }) + .split('\n').filter(Boolean); + return names.find((n) => { + const src = execSync( + `docker inspect ${n} --format '{{range .Mounts}}{{if eq .Destination "/var/www/html/user"}}{{.Source}}{{end}}{{end}}'`, + { encoding: 'utf-8', stdio: ['pipe', 'pipe', 'ignore'] } + ).trim(); + return src && fs.realpathSync(src) === want; + }) || null; + } catch (_) { + return null; + } +} + +/** + * Delete an entry directory, falling back to the container when the host cannot. + * + * Grav's Apache workers run as root, so every entry the form creates is + * root-owned. Removing one recursively needs write permission on that directory, + * which the host user does not have — so a plain fs.rmSync throws EACCES and the + * entry survives. That is how a ui-test entry ended up committed-adjacent in the + * active trip's content on 2026-07-24: cleanup had never actually worked for + * form-created entries, it just failed inside a path nothing checked. + * + * `docker exec … rm -rf` runs as root in the container, which can remove them. + */ +function removeEntryDir(dir) { + try { + fs.rmSync(dir, { recursive: true }); + return true; + } catch (err) { + if (err.code !== 'EACCES' && err.code !== 'EPERM') throw err; + } + const container = resolveGravContainer(); + if (!container) { + throw new Error( + `Cannot remove ${dir}: it is root-owned (written by Grav in the container) and no ` + + `matching container was found to delete it as root. Set GRAV_CONTAINER or remove it manually.` + ); + } + execSync(`docker exec ${container} rm -rf '/var/www/html/user/${path.relative(USER_DIR, dir)}'`, + { stdio: ['pipe', 'pipe', 'pipe'] }); + return true; +} + /** * Find a tracker entry folder by a unique slug fragment, then delete it. */ @@ -179,7 +233,7 @@ function cleanupEntry(slugFragment) { const entries = fs.readdirSync(TRACKER_DIR); const match = entries.find(e => e.includes(slugFragment)); if (match) { - fs.rmSync(path.join(TRACKER_DIR, match), { recursive: true }); + removeEntryDir(path.join(TRACKER_DIR, match)); } } @@ -202,4 +256,7 @@ function readEntryMd(entryDir) { return fs.readFileSync(path.join(entryDir, name), 'utf-8'); } -module.exports = { fillEditor, waitForPhotoUpload, postEntry, createPhotoEntry, cleanupEntry, findEntry, readEntryMd, TEST_PHOTO, TRACKER_DIR, ACTIVE_TRIP_URL }; +// USER_DIR is exported so global-setup/global-teardown resolve the same tree the +// specs assert against, instead of keeping their own (previously divergent) copy +// of this logic. +module.exports = { fillEditor, waitForPhotoUpload, postEntry, createPhotoEntry, cleanupEntry, removeEntryDir, findEntry, readEntryMd, TEST_PHOTO, USER_DIR, TRACKER_DIR, ACTIVE_TRIP_URL }; diff --git a/tests/ui/post/location-override.spec.js b/tests/ui/post/location-override.spec.js index e37ca4e..fefc65c 100644 --- a/tests/ui/post/location-override.spec.js +++ b/tests/ui/post/location-override.spec.js @@ -330,6 +330,10 @@ test('submitting with an unresolved lat/lng mismatch is blocked', async ({ page }); // ── U4: lazy-load boundary — an ordinary GPS-only submit never fetches maplibre-gl ── +// The URL pattern deliberately covers BOTH halves of the lazy boundary: the JS +// chunk (js/post/maplibre-gl-*.js) and the stylesheet +// (css-compiled/maplibre-gl.css, ed by location-map.js at panel-open — +// see its ensureMaplibreCss). Neither may be requested when the panel stays shut. test('an ordinary submit without opening the panel never fetches the maplibre-gl chunk', async ({ page }) => { const chunkRequests = []; page.on('request', (req) => { @@ -346,7 +350,32 @@ test('an ordinary submit without opening the panel never fetches the maplibre-gl await expect(page.locator('.notices')).toContainText('Entry posted successfully!', { timeout: 15_000 }); created.push(tag); - expect(chunkRequests, 'maplibre-gl must not be fetched when the panel is never opened').toHaveLength(0); + expect(chunkRequests, 'neither the maplibre-gl chunk nor its stylesheet may be fetched when the panel is never opened').toHaveLength(0); +}); + +// ── The other half of that boundary: opening the panel DOES apply the vendor CSS ── +// Without this, the guard above could keep passing while the stylesheet silently +// stopped loading at all (a broken href, a missed build step), leaving the map +// unstyled with nothing to catch it. Asserts the exists AND parsed — +// link.sheet is null until the browser has actually applied it. +test('opening the panel lazily links maplibre\'s stylesheet and applies it', async ({ page }) => { + await page.goto('/post'); + + const hrefBefore = await page.evaluate(() => Array.from(document.styleSheets) + .map((s) => s.href || '').filter((h) => /maplibre-gl\.css/.test(h))); + expect(hrefBefore, 'the vendor stylesheet must not be present before the panel opens').toHaveLength(0); + + await openLocationDetails(page); + await expect(page.locator('#location-map canvas.maplibregl-canvas')).toHaveCount(1, { timeout: 10_000 }); + + await expect.poll( + () => page.evaluate(() => { + const link = Array.from(document.querySelectorAll('link[rel="stylesheet"]')) + .find((l) => /maplibre-gl\.css/.test(l.href)); + return link ? link.sheet !== null : false; + }), + { message: 'maplibre\'s stylesheet must be linked and applied once the panel opens', timeout: 10_000 } + ).toBe(true); }); // ── Full submit: a search-picked location round-trips into the frontmatter ── diff --git a/user b/user index e873a9c..2b91aa3 160000 --- a/user +++ b/user @@ -1 +1 @@ -Subproject commit e873a9cb2341008c3d0fbe481e79000cac729c9d +Subproject commit 2b91aa30c33a183e1ef24987feb045045616fd3c