ci(pages): add smoke test step to verify build output is servable - #741
Conversation
Serve dist/ after build and assert key SPA routes return HTML with bundle references so broken templates cannot slip through CI. Co-authored-by: Cursor <cursoragent@cursor.com>
|
🔍 OpenCodeReview found 2 issue(s) in this PR.
|
| npx serve dist -s -l tcp://127.0.0.1:3999 & | ||
| SERVER_PID=$! | ||
| trap 'kill $SERVER_PID 2>/dev/null || true' EXIT | ||
| sleep 2 |
There was a problem hiding this comment.
[maintainability · medium]
Reliability concern: fixed sleep 2 is fragile.
On a busy CI runner, 2 seconds may not be enough for the server to be ready. Consider using a polling loop with retries instead, which is more resilient to variable startup times:
for i in $(seq 1 20); do
curl -sf http://127.0.0.1:3999/ > /dev/null && break
sleep 0.5
done| BODY=$(curl -sf "http://127.0.0.1:3999${path}") | ||
| if [ $? -ne 0 ]; then |
There was a problem hiding this comment.
[bug · high]
Bug: set -e will kill the script before the $? check is reached.
GitHub Actions run: steps use bash -eo pipefail by default. When curl -sf fails (e.g., the server isn't ready or a path returns an error), the command substitution BODY=$(curl -sf ...) will propagate the non-zero exit code, and set -e will immediately terminate the script — before the if [ $? -ne 0 ] check ever runs. This means the first failing path aborts the entire loop, skipping remaining paths and the exit $FAILED line.
Restructure so that curl's failure is caught within an if condition (which is exempt from set -e):
Suggestion:
| BODY=$(curl -sf "http://127.0.0.1:3999${path}") | |
| if [ $? -ne 0 ]; then | |
| if ! BODY=$(curl -sf "http://127.0.0.1:3999${path}"); then |
|
@HuijungYoon Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. |
|
@lizhengfeng101 I've signed the CLA! I'm really looking forward to contributing more to this project. Thank you! |
ok |
|
Hey @HuijungYoon, nice addition — the smoke step is a sensible lightweight alternative to spinning up Playwright. Two things I'd love your take on before this goes in: 1. All four routes resolve to the same 2. Minor: the if ! BODY=$(curl -sf "http://127.0.0.1:3999${path}"); then
echo "FAIL: ${path} — not reachable"; FAILED=1
elif ! echo "$BODY" | grep -q '\.bundle\.js'; then
...Also +1 on the |
Clarify the SPA fallback limitation, make the curl failure branch work under set -e, and add a /404.html static-file assertion to confirm dist assets are actually served.
|
Thanks for the thoughtful review I pushed a follow-up commit that addresses both items:
Really appreciate the careful read and the concrete suggestions. |
Summary
serveas a pagesdevDependencyso CI can statically servedist/without downloading on every run.pages-ci.yml, smoke-test/,/docs/contributing,/docs/quickstart, and/featuresfor HTTP 200 responses that include.bundle.jsreferences (SPA mode viaserve -s).Test plan
pages/**path filter)cd pages && npm install && npm run build, then run the smoke script from the workflow