Skip to content

Commit e3061e3

Browse files
committed
ci: enforce the website patch gate and trim comment rationale
Review feedback from DrJKL: - Make the 80% website patch status effective. informational: true always passes regardless of target, so the gate was advertised but inert. The project status stays informational; patch is the one that answers "is the code you just wrote tested?". - Condense the CI/coverage comments to the constraint itself rather than narrating the implementation. - Drop the README note about unit tests no longer being in that list. A README documents what is, not what changed. - behaviour -> behavior.
1 parent 999742d commit e3061e3

5 files changed

Lines changed: 14 additions & 32 deletions

File tree

.coderabbit.yaml

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -67,15 +67,15 @@ reviews:
6767
Changes confined to `packages/` are deliberately out of scope here, even though the website consumes those packages: the generic "End-to-end regression coverage for fixes" check already requires a `browser_tests/` regression test for them. Do not demand a second website-specific test for a shared-package change.
6868
6969
Fail if all of the following are true:
70-
1. The diff itself changes observable website runtime behaviour — fixes a user-visible bug, adds a page or route, or changes an interactive flow, form, or navigation. A `fix`/`bugfix`/`hotfix` style title or commit subject is only a hint: confirm it against the diff, and never treat the wording alone as qualifying.
71-
2. The PR does not add or update a Playwright assertion under `apps/website/e2e/**/*.spec.ts` that actually exercises the route, flow, or behaviour this PR changed. An unrelated assertion elsewhere in the suite does not satisfy this.
70+
1. The diff itself changes observable website runtime behavior — fixes a user-visible bug, adds a page or route, or changes an interactive flow, form, or navigation. A `fix`/`bugfix`/`hotfix` style title or commit subject is only a hint: confirm it against the diff, and never treat the wording alone as qualifying.
71+
2. The PR does not add or update a Playwright assertion under `apps/website/e2e/**/*.spec.ts` that actually exercises the route, flow, or behavior this PR changed. An unrelated assertion elsewhere in the suite does not satisfy this.
7272
3. The PR description lacks a concrete explanation of why an end-to-end regression test was not added.
7373
74-
Do not fail for text/copy-only edits, translation-only changes, static asset swaps, styling-only changes, generated files, dependency metadata, refactors that preserve behaviour, or changes confined to tests, tooling, or CI.
74+
Do not fail for text/copy-only edits, translation-only changes, static asset swaps, styling-only changes, generated files, dependency metadata, refactors that preserve behavior, or changes confined to tests, tooling, or CI.
7575
A reformatted or whitespace-only edit to an existing spec does not count as adding coverage.
7676
7777
Pass otherwise.
78-
When failing, name the behaviour-changing signal you found and ask the author to either add or update a Playwright regression test under `apps/website/e2e/` or record in the PR description why an end-to-end test is not practical.
78+
When failing, name the behavior-changing signal you found and ask the author to either add or update a Playwright regression test under `apps/website/e2e/` or record in the PR description why an end-to-end test is not practical.
7979
8080
- name: ADR compliance for entity/litegraph changes
8181
mode: warning
@@ -128,7 +128,7 @@ reviews:
128128
- path: 'apps/website/src/**/*.{ts,vue}'
129129
instructions: |
130130
Changed lines here are measured by the `website-unit` Codecov patch
131-
status, so new behaviour needs a colocated Vitest test. Treat
131+
status, so new behavior needs a colocated Vitest test. Treat
132132
`docs/guidance/vitest.md` as required review context. Flag new
133133
exported logic that no test exercises.
134134
This glob is wider than the gate: `coverage.exclude` in

.github/workflows/ci-website-unit.yaml

Lines changed: 4 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -48,19 +48,13 @@ jobs:
4848
files: ./coverage/lcov.info
4949
disable_search: true
5050
flags: website-unit
51-
# LCOV records paths relative to apps/website (`src/...`), which on
52-
# their own would collide with the root app's `src/`. Uploading from
53-
# that directory keeps report and network paths aligned, and the
54-
# prefix re-roots both onto `apps/website/src/...`.
51+
# Re-root LCOV paths to prevent collisions with root-app coverage.
5552
network_prefix: apps/website/
5653
token: ${{ secrets.CODECOV_TOKEN }}
57-
# Forks receive no CODECOV_TOKEN; a failed upload must not block an
58-
# external contributor. Everywhere else a silent upload failure
59-
# would silently disable the coverage gate.
54+
# Forks have no token, so upload failures cannot block contributors.
6055
fail_ci_if_error: ${{ github.event.pull_request.head.repo.fork != true }}
6156

62-
# Always runs, so branch protection has a context that reliably posts even
63-
# when the test job is correctly skipped for non-website changes.
57+
# Stable branch-protection context when tests are skipped.
6458
website-unit-gate:
6559
needs: [changes, website-unit]
6660
if: ${{ always() }}
@@ -77,9 +71,7 @@ jobs:
7771
echo "Website unit tests passed."
7872
exit 0
7973
fi
80-
# Only a deliberate skip counts as a pass: the filter itself must
81-
# have succeeded and reported nothing relevant changed. Otherwise a
82-
# failed filter job would silently satisfy this gate.
74+
# Pass skipped tests only after successful filtering found no relevant changes.
8375
if [ "$UNIT_RESULT" = "skipped" ] &&
8476
[ "$CHANGES_RESULT" = "success" ] &&
8577
[ "$WEBSITE_CHANGED" != "true" ] &&

apps/website/README.md

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -36,10 +36,6 @@ inline that into the client bundle).
3636
This repo's `.github/workflows/*.yaml` changes cannot be pushed by a
3737
GitHub App. A maintainer must apply the following edits **once**:
3838

39-
Unit tests are no longer part of this list — they run in
40-
`.github/workflows/ci-website-unit.yaml`, which also uploads coverage to
41-
Codecov under the `website-unit` flag.
42-
4339
**`.github/workflows/ci-website-build.yaml`** — pass the env into the
4440
build step:
4541

apps/website/vitest.config.ts

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,7 @@ export default defineConfig({
2323
provider: 'v8',
2424
reporter: ['text', 'lcov'],
2525
reportsDirectory: './coverage',
26-
// Load-bearing: without it, untested files are absent from the report
27-
// rather than counted as 0%, so patch coverage passes on untested code.
26+
// Include untested files so patch coverage counts them as 0%.
2827
include: ['src/**/*.{ts,vue}'],
2928
exclude: [
3029
'src/**/*.{test,spec}.ts',

codecov.yml

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,7 @@
11
coverage:
22
status:
33
project:
4-
# Scoped to the pre-existing uploads so that adding the website report
5-
# (which starts near 22%) cannot drag this status down and fail PRs
6-
# that never touched the website.
4+
# Keep website coverage from lowering the existing frontend status.
75
default:
86
target: auto
97
flags:
@@ -24,13 +22,11 @@ coverage:
2422
default:
2523
informational: true
2624

27-
# The enforcing gate: lines changed under apps/website must be covered.
28-
# Deliberately patch-based — it asks "is the code you just wrote
29-
# tested?" rather than punishing authors for pre-existing gaps.
25+
# Require 80% coverage on changed website lines.
3026
website:
3127
target: '80%'
3228
threshold: '0%'
33-
informational: true
29+
informational: false
3430
flags:
3531
- website-unit
3632
paths:
@@ -54,8 +50,7 @@ flags:
5450
carryforward: true
5551
e2e:
5652
carryforward: true
57-
# No carryforward: the website job is the sole authority for these paths, so
58-
# a missing upload must never be papered over with stale coverage.
53+
# Do not hide missing website coverage with stale data.
5954
website-unit:
6055
paths:
6156
- apps/website/src/**

0 commit comments

Comments
 (0)