Skip to content

Commit a5f1a9b

Browse files
christian-byrneConnor Byrne
andauthored
fix: restore E2E coverage source mapping (#14950)
## Summary E2E coverage has been reported as either missing or 100% since `build.sourcemap: 'hidden'` landed; this restores the source mapping and adds guards so a degenerate tracefile fails loudly instead of being announced as a milestone. ## Changes - **What**: - The E2E build sets `COLLECT_COVERAGE=true`, which re-injects the `//# sourceMappingURL=` comment. Everything that ships to users keeps `'hidden'`, so the `.js.map` 404 noise FE-1405 removed stays gone. `GENERATE_SOURCEMAP` can't carry this — `Comfy-Org/cloud` already sets it true for the staging and prod builds. - `CI: E2E Coverage` asserts the merged tracefile still has source-mapped files before stripping bundle paths, with an error that names the cause. - `coverage-slack-notify.ts` counts only `src/`/`packages/` files and treats an implausibly small file set as no data. ## Review Focus Root cause: monocart-coverage-reports resolves source maps *only* from the `//# sourceMappingURL=` comment in the served bundle (`lib/converter/collect-source-maps.js`). #14209 switched `build.sourcemap` to `'hidden'`, which emits the `.map` files but strips that comment, so every V8 coverage entry stayed at its served path (`localhost-8188/assets/*.js`). The merge job strips those paths, which left two failure modes: - Nothing survives → `lcov: ERROR: no valid records found` → job fails → no `e2e-coverage` artifact → the Slack report silently drops its E2E line (what you see after #14809). - One third-party script survives (`js.stripe.com/dahlia/stripe.js`, fully covered) → E2E reported as 100% (`e2e-coverage` artifacts drop from ~350 KB to ~28 KB). That is the source of the "GOAL REACHED: E2E test coverage hit 100%" messages. Replaying the current script against the real 28 KB artifact reproduces the Slack message exactly (`E2E: 68.5% → 100.0% (+31.5%)`); with this change the same input produces no post. The only source-mapped E2E artifacts after 2026-07-30 came from PR branches not yet rebased past #14209 — hence the decay from ~170/day to zero rather than a clean cutover. Note the stored `e2e-coverage-baseline` currently holds the bogus 100%, so the first run after this merges will look like a large regression and post nothing; the baseline self-heals on that same run. --------- Co-authored-by: Connor Byrne <c.byrne@comfy.org>
1 parent 78ee22a commit a5f1a9b

5 files changed

Lines changed: 92 additions & 6 deletions

File tree

.github/workflows/ci-tests-e2e-coverage.yaml

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,16 @@ jobs:
8585
fi
8686
done
8787
88+
- name: Assert coverage was mapped back to source
89+
if: steps.coverage-shards.outputs.has-coverage == 'true'
90+
run: |
91+
MAPPED_SF=$(grep -cE '^SF:(src|packages)/' coverage/playwright/coverage.lcov || true)
92+
echo "Source-mapped files: $MAPPED_SF" >> "$GITHUB_STEP_SUMMARY"
93+
if [ "${MAPPED_SF:-0}" -lt 100 ]; then
94+
echo "::error::Only $MAPPED_SF files under src/ or packages/ in the merged tracefile. Observed paths: $(grep -m 5 '^SF:' coverage/playwright/coverage.lcov | tr '\n' ' '). Served bundle paths mean the E2E build dropped its '//# sourceMappingURL=' comment — check it ran with COLLECT_COVERAGE=true (vite.config.mts build.sourcemap)."
95+
exit 1
96+
fi
97+
8898
- name: Strip non-source entries from coverage
8999
if: steps.coverage-shards.outputs.has-coverage == 'true'
90100
run: |

.github/workflows/ci-tests-e2e.yaml

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,8 @@ jobs:
3636
uses: ./.github/actions/setup-frontend
3737
with:
3838
include_build_step: true
39+
env:
40+
COLLECT_COVERAGE: 'true'
3941

4042
# Upload only built dist/ (containerized test jobs will pnpm install without cache)
4143
- name: Upload built frontend
@@ -260,7 +262,7 @@ jobs:
260262
261263
- name: Download built frontend
262264
if: steps.detect.outputs.has-new-tests == 'true'
263-
uses: actions/download-artifact@v7
265+
uses: actions/download-artifact@v8
264266
with:
265267
name: frontend-dist
266268
path: dist/
Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
import { describe, expect, it } from 'vitest'
2+
3+
import { parseLcovContent } from './coverage-slack-notify'
4+
5+
function lcov(entries: [file: string, lf: number, lh: number][]): string {
6+
return entries
7+
.map(([file, lf, lh]) => `SF:${file}\nLF:${lf}\nLH:${lh}\nend_of_record`)
8+
.join('\n')
9+
}
10+
11+
function sourceEntries(
12+
count: number,
13+
lf: number,
14+
lh: number
15+
): [string, number, number][] {
16+
return Array.from({ length: count }, (_, i) => [
17+
`src/components/Component${i}.vue`,
18+
lf,
19+
lh
20+
])
21+
}
22+
23+
describe('parseLcovContent', () => {
24+
it('reports the ratio of covered to total lines', () => {
25+
const result = parseLcovContent(lcov(sourceEntries(120, 10, 7)))
26+
27+
expect(result).toEqual({
28+
percentage: 70,
29+
totalLines: 1200,
30+
coveredLines: 840
31+
})
32+
})
33+
34+
it('ignores files outside src/ and packages/', () => {
35+
const result = parseLcovContent(
36+
lcov([
37+
...sourceEntries(120, 10, 5),
38+
['localhost-8188/assets/index-a1b2c3.js', 1000, 1000],
39+
['js.stripe.com/dahlia/stripe.js', 500, 500]
40+
])
41+
)
42+
43+
expect(result?.totalLines).toBe(1200)
44+
expect(result?.percentage).toBe(50)
45+
})
46+
47+
// E2E coverage that fails to map back to source leaves only third-party
48+
// scripts behind, which are fully covered and would report as 100%.
49+
it('returns null when too few project files are present', () => {
50+
expect(
51+
parseLcovContent(lcov([['js.stripe.com/dahlia/stripe.js', 500, 500]]))
52+
).toBeNull()
53+
54+
expect(parseLcovContent(lcov(sourceEntries(99, 10, 10)))).toBeNull()
55+
expect(parseLcovContent(lcov(sourceEntries(100, 10, 10)))).not.toBeNull()
56+
})
57+
58+
it('returns null for an empty tracefile', () => {
59+
expect(parseLcovContent('')).toBeNull()
60+
})
61+
})

scripts/coverage-slack-notify.ts

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,11 @@ const MILESTONE_STEP = 5
55
const MIN_DELTA = 0.05
66
const BAR_WIDTH = 20
77

8+
/** Repo-relative prefixes of the files whose coverage this report is about. */
9+
const PROJECT_SOURCE = /^(src|packages)\//
10+
/** Below this the tracefile is degenerate, not sparse, and its ratio is noise. */
11+
const MIN_SOURCE_FILES = 100
12+
813
interface CoverageData {
914
percentage: number
1015
totalLines: number
@@ -19,13 +24,16 @@ interface SlackBlock {
1924
}
2025
}
2126

22-
function parseLcovContent(content: string): CoverageData | null {
27+
export function parseLcovContent(content: string): CoverageData | null {
2328
const perFile = new Map<string, { lf: number; lh: number }>()
2429
let currentFile = ''
2530

2631
for (const line of content.split('\n')) {
2732
if (line.startsWith('SF:')) {
28-
currentFile = line.slice(3)
33+
const file = line.slice(3)
34+
currentFile = PROJECT_SOURCE.test(file) ? file : ''
35+
} else if (!currentFile) {
36+
continue
2937
} else if (line.startsWith('LF:')) {
3038
const n = parseInt(line.slice(3), 10) || 0
3139
const entry = perFile.get(currentFile) ?? { lf: 0, lh: 0 }
@@ -46,7 +54,7 @@ function parseLcovContent(content: string): CoverageData | null {
4654
coveredLines += lh
4755
}
4856

49-
if (totalLines === 0) return null
57+
if (totalLines === 0 || perFile.size < MIN_SOURCE_FILES) return null
5058

5159
return {
5260
percentage: (coveredLines / totalLines) * 100,
@@ -230,4 +238,6 @@ function main() {
230238
process.stdout.write(JSON.stringify(payload))
231239
}
232240

233-
main()
241+
if (process.argv[1] && import.meta.url === `file://${process.argv[1]}`) {
242+
main()
243+
}

vite.config.mts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ const ANALYZE_BUNDLE = process.env.ANALYZE_BUNDLE === 'true'
2828
const VITE_REMOTE_DEV = process.env.VITE_REMOTE_DEV === 'true'
2929
const DISABLE_TEMPLATES_PROXY = process.env.DISABLE_TEMPLATES_PROXY === 'true'
3030
const GENERATE_SOURCEMAP = process.env.GENERATE_SOURCEMAP !== 'false'
31+
const COLLECT_COVERAGE = process.env.COLLECT_COVERAGE === 'true'
3132
const IS_STORYBOOK = process.env.npm_lifecycle_event === 'storybook'
3233

3334
const CRITICAL_COVERAGE_DIRS = [
@@ -546,7 +547,9 @@ export default defineConfig({
546547
// browser-facing `//# sourceMappingURL=` comment is NOT injected into the JS
547548
// bundles. This kills the ~57k/3d `/assets/*.js.map` 404 noise in prod
548549
// (the .map files aren't served) without losing Sentry symbolication. See FE-1405.
549-
sourcemap: GENERATE_SOURCEMAP ? 'hidden' : false,
550+
// A coverage build serves its own .map files and needs the comment back:
551+
// monocart maps V8 coverage to src/** only by following it.
552+
sourcemap: GENERATE_SOURCEMAP && (COLLECT_COVERAGE || 'hidden'),
550553
// Exclude heavy optional vendor chunks from initial module preload
551554
// These chunks are only needed when their features are used (3D, terminal, etc.)
552555
modulePreload: {

0 commit comments

Comments
 (0)