Add browser smoke tests for demo fixtures#42
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 391283cd67
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Makefile
Outdated
|
|
||
| browser-setup: | ||
| @printf "$(PROGRESS) Installing browser test dependencies...\n" | ||
| @npm ci && npx playwright install chromium && \ |
There was a problem hiding this comment.
browser-setup
On fresh Debian/Ubuntu developer machines, npx playwright install chromium only downloads the browser binary; it does not install the shared libraries Chromium needs. Playwright’s own install docs use install --with-deps for that step (https://playwright.dev/docs/intro#updating-playwright), and this same PR already needs --with-deps in .github/workflows/ci.yml:31-34 to make CI pass. As written, make browser-setup still leaves make test-browser failing with missing system dependencies on common Linux setups even though it is documented as the one-time setup command.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Updated. browser-setup now uses npx playwright install --with-deps chromium so the one-time local setup matches the CI dependency path and installs the shared libraries Chromium needs on Debian/Ubuntu-class Linux machines.
scripts/demo_server.py
Outdated
| path_only = path.split("?", 1)[0].split("#", 1)[0] | ||
| if path_only.startswith("/sf/"): | ||
| path_only = "/static" + path_only | ||
| return str(ROOT / path_only.lstrip("/")) |
There was a problem hiding this comment.
translate_path() now concatenates the raw request path onto ROOT without normalizing or rejecting .. segments, so make demo-serve will serve arbitrary local files outside the repo. I verified that requesting /../../etc/hosts from this server returns 200, which is a regression from the previous python -m http.server behavior. This matters whenever the demo port is exposed via a forwarded port or shared dev environment.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8c544a3. translate_path() now URL-decodes, normalizes the path via PurePosixPath, drops .. segments, resolves the candidate path, and rejects anything that escapes ROOT. I verified /../../../../etc/hosts now returns 404, while /sf/fonts/space-grotesk.woff2 still returns 200.
1 participant
Summary
Add browser-level smoke coverage for the shipped demo fixtures and wire it into local and CI workflows.
What Changed
demos/full-surface.htmlanddemos/rail.htmlmake browser-setupandmake test-browser, and included browser verification in the standard local validation targets/sf/*tostatic/sf/*so bundled font URLs resolve in local demo servingValidation
node --test tests/*.test.jsnode tests/demo-browser-check.jspython3 -m py_compile scripts/demo_server.pyFixes #38