Skip to content

Commit e80846d

Browse files
slang25claude
andcommitted
fix: address PR review feedback
- Exit non-zero on startup failures in render test runner (P1) - Wait for browser-side runAllTests() completion before reading canvases (P2) - Use || instead of ?? for font metric fallbacks so 0 triggers fallback (P2) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 47fdaa0 commit e80846d

3 files changed

Lines changed: 12 additions & 17 deletions

File tree

demo/bin/render-test.ts

Lines changed: 7 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -133,18 +133,9 @@ async function main() {
133133
timeout: 30000,
134134
});
135135

136-
// Wait for fonts to load - match the logic in render-test.html
137-
await page.evaluate(async () => {
138-
const fontVariants = [
139-
'14px "JetBrainsMono NF"',
140-
'bold 14px "JetBrainsMono NF"',
141-
'italic 14px "JetBrainsMono NF"',
142-
'bold italic 14px "JetBrainsMono NF"',
143-
];
144-
await Promise.all(fontVariants.map(font => document.fonts.load(font)));
145-
await document.fonts.ready;
146-
});
147-
await new Promise((r) => setTimeout(r, 500)); // Extra time for font rendering
136+
// Wait for the page's runAllTests() to complete.
137+
// render-test.html sets window.__testsComplete = true when done.
138+
await page.waitForFunction('window.__testsComplete === true', { timeout: 60000 });
148139

149140
// Get test cases from the page
150141
const testCases = await page.evaluate(() => {
@@ -288,4 +279,7 @@ function calculateDiffPercent(buf1: Buffer, buf2: Buffer): number {
288279
return (diffBytes / maxSize) * 100;
289280
}
290281

291-
main().catch(console.error);
282+
main().catch((e) => {
283+
console.error(e);
284+
process.exit(1);
285+
});

demo/render-test.html

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -779,6 +779,7 @@ <h1>Visual Render Tests</h1>
779779
}
780780

781781
document.getElementById('run-btn').disabled = false;
782+
window.__testsComplete = true;
782783
}
783784

784785
// Initialize

lib/renderer.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -226,12 +226,12 @@ export class CanvasRenderer {
226226
// characters (U+E0B0, U+E0B6, etc.) which are designed to fill the full cell height.
227227
// Fall back to actual metrics if font metrics aren't available.
228228
const ascent =
229-
widthMetrics.fontBoundingBoxAscent ??
230-
widthMetrics.actualBoundingBoxAscent ??
229+
widthMetrics.fontBoundingBoxAscent ||
230+
widthMetrics.actualBoundingBoxAscent ||
231231
this.fontSize * 0.8;
232232
const descent =
233-
widthMetrics.fontBoundingBoxDescent ??
234-
widthMetrics.actualBoundingBoxDescent ??
233+
widthMetrics.fontBoundingBoxDescent ||
234+
widthMetrics.actualBoundingBoxDescent ||
235235
this.fontSize * 0.2;
236236

237237
const height = Math.ceil(ascent + descent);

0 commit comments

Comments
 (0)