Skip to content

Organize CI build and validation into pre-build, build, and post-build#29177

Draft
mvvmm wants to merge 4 commits intoproductionfrom
build-validation-ordering
Draft

Organize CI build and validation into pre-build, build, and post-build#29177
mvvmm wants to merge 4 commits intoproductionfrom
build-validation-ordering

Conversation

@mvvmm
Copy link
Contributor

@mvvmm mvvmm commented Mar 20, 2026

Summary

What changed

The CI pipeline had a build + validate structure where lint, formatting, and test failures were only surfaced after waiting 20+ minutes for the build to complete. This restructures the pipeline into three explicit phases.

New job topology

pre-build ───────────────────────────────► build ──────────────────────► post-build
  CRLF check                                 │                               Workers tests (require dist/)
  invalid file extension check               │                               link validation
  type check (check + check:worker)          │
  eslint (reviewdog)                         ├──────────────────────────► publish-preview
  format check                               │
  redirect validation                        └──────────────────────────► notify
  Node + Astro tests

Changes at a glance

  • pre-build (new): All fast validation steps that do not require a built dist/ move here — CRLF check, file extension check, type checking, ESLint, format check, redirect validation, and Node/Astro tests (test:prebuild). Failures surface in ~2-3 minutes instead of after the full build.
  • build: Now needs: pre-build. Stripped of all steps moved to pre-build — responsible only for running npm run build and uploading the dist/ artifact.
  • post-build (renamed from validate): Runs after build. Downloads the dist/ artifact, runs the Workers test suite (test:postbuild) which requires dist/ to exist, then surfaces any link validation failures.
  • notify: Updated needs to [pre-build, build, post-build] and job name filters updated to match new names.
  • compile: Untouched (pending separate removal).
  • publish-preview: Untouched.

Why tests are split

The Workers vitest pool reads wrangler.toml at startup, which specifies assets.directory = "./dist". Running all tests in pre-build caused an immediate crash since dist/ doesn't exist yet. Two scripts were added to package.jsontest:prebuild (Node + Astro suites) and test:postbuild (Workers suite) — to split them across the appropriate phases.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 20, 2026

This pull request requires reviews from CODEOWNERS as it changes files that match the following patterns:

Pattern Owners
/.github/ @cloudflare/content-engineering, @kodster28, @mvvmm, @colbywhite, @ahaywood, @MohamedH1998
*.ts @cloudflare/content-engineering, @kodster28
package.json @cloudflare/content-engineering

@mvvmm
Copy link
Contributor Author

mvvmm commented Mar 20, 2026

/bonk please review

@ask-bonk
Copy link
Contributor

ask-bonk bot commented Mar 20, 2026

This PR effectively restructures the CI pipeline to provide faster feedback on lint/format/test failures. The logic is sound - splitting tests based on their dist/ dependency makes sense given the Workers vitest pool reads wrangler.toml at startup.

One issue to address:

The compile job still exists (lines 13-99) and duplicates many steps now in pre-build. Since pre-build now handles:

  • File extension validation
  • Type checking (check + check:worker)
  • ESLint
  • Format checking
  • Redirect validation
  • Node/Astro tests

The compile job appears redundant. It even runs npm run test (line 92) which would try to run all test suites including Workers tests that require dist/, potentially causing failures.

Suggestion: Either remove the compile job entirely in this PR, or add a comment indicating it's pending removal. The current state has two jobs doing overlapping validation work.

Everything else looks good:

  • Job dependencies are correctly wired (pre-buildbuildpost-build)
  • notify job properly updated with all three dependencies
  • post-pr-ci-failure-comment/index.ts correctly filters for new job names
  • Test scripts in package.json properly split by project type

github run

@github-actions
Copy link
Contributor

github-actions bot commented Mar 20, 2026

@mvvmm mvvmm force-pushed the build-validation-ordering branch from 6ea9dcb to f48a883 Compare March 20, 2026 19:54
run: npm ci
if: steps.node-modules-cache.outputs.cache-hit != 'true'

- uses: actions/cache@v5
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- uses: actions/cache@v5
- name: Restore Astro assets from cache
- uses: actions/cache@v5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants