diff --git a/.github/scripts/query-regression-comment.cjs b/.github/scripts/query-regression-comment.cjs index 130b1c7efa..2d6c9c40dc 100644 --- a/.github/scripts/query-regression-comment.cjs +++ b/.github/scripts/query-regression-comment.cjs @@ -92,7 +92,7 @@ function thresholdStatus(thresholds, query) { const hits = (Array.isArray(thresholds) ? thresholds : []) .filter(item => query === undefined || (hasScopedQuery(item) && String(item.query) === query)) .map(formatThreshold); - return hits.length > 0 ? hits.join(', ') : 'N/A'; + return hits.length > 0 ? hits.join('; ') : 'N/A'; } function hasScopedQuery(threshold) { @@ -115,7 +115,7 @@ function formatThreshold(threshold) { return `${name}${scope.length > 0 ? ` [${scope.join(', ')}]` : ''}: ${status}${reason}`; } -function syntheticThresholdStatus(thresholds, measurementNames) { +function classifyThresholds(thresholds, measurementNames) { const unscoped = []; const unmatched = new Map(); for (const threshold of Array.isArray(thresholds) ? thresholds : []) { @@ -130,7 +130,11 @@ function syntheticThresholdStatus(thresholds, measurementNames) { unmatched.set(query, entries); } } + return { unscoped, unmatched }; +} +function syntheticThresholdStatus(thresholds, measurementNames) { + const { unscoped, unmatched } = classifyThresholds(thresholds, measurementNames); const parts = []; if (unscoped.length > 0) { parts.push(`case/storage threshold: ${thresholdStatus(unscoped)}`); @@ -141,6 +145,21 @@ function syntheticThresholdStatus(thresholds, measurementNames) { return parts.length > 0 ? parts.join('; ') : 'N/A'; } +// Case-level (query-unscoped) thresholds formatted for the collapsible details +// block below the summary table: individual items, `; `-separated, without the +// `case/storage threshold:` label (the section heading already says that). +function syntheticThresholdDetail(thresholds, measurementNames) { + const { unscoped, unmatched } = classifyThresholds(thresholds, measurementNames); + const items = []; + if (unscoped.length > 0) { + items.push(thresholdStatus(unscoped)); + } + for (const query of Array.from(unmatched.keys()).sort()) { + items.push(`unmatched query ${query}: ${thresholdStatus(unmatched.get(query))}`); + } + return items.length > 0 ? items.join('; ') : 'N/A'; +} + function joinDetails(...details) { const present = details.filter(detail => detail && detail !== 'N/A'); return present.length > 0 ? present.join('; ') : 'N/A'; @@ -229,7 +248,7 @@ function collectReportRows(report, reportPath) { ), }; }); - const syntheticThresholds = syntheticThresholdStatus(thresholds, measurementNames); + const syntheticThresholds = syntheticThresholdDetail(thresholds, measurementNames); if (syntheticThresholds !== 'N/A') { rows.push({ caseName: name, @@ -239,22 +258,46 @@ function collectReportRows(report, reportPath) { candidateMedian: 'N/A', regression: 'N/A', threshold: syntheticThresholds, + kind: 'case-thresholds', }); } return rows; } +function renderThresholdDetails(rows) { + const entries = rows.filter(row => row.kind === 'case-thresholds'); + if (entries.length === 0) { + return ''; + } + // The
/ tags are emitted literally; only the entry content + // goes through text() (so `[ ] ( ) |` etc. stay escaped while the tags render). + const lines = [ + '
Case / storage thresholds', + '', + ]; + for (const row of entries) { + lines.push(`- ${text(row.caseName)}: ${text(row.threshold)}`); + } + lines.push('
'); + return lines.join('\n'); +} + function renderSummaryTable(rows) { const lines = [ '| Case | Query | Case status | Base median ms | Candidate median ms | Regression | Threshold |', '| --- | --- | --- | ---: | ---: | ---: | --- |', ]; for (const row of rows) { + if (row.kind === 'case-thresholds') { + continue; + } lines.push( - `| ${text(row.caseName)} | ${text(row.query)} | ${statusEmoji(row.status)} \`${text(row.status)}\` | ${text(row.baseMedian)} | ${text(row.candidateMedian)} | ${text(row.regression)} | ${text(row.threshold)} |` + `| ${text(row.caseName)} | ${text(row.query)} | ${statusEmoji(row.status)} ${text(row.status)} | ${text(row.baseMedian)} | ${text(row.candidateMedian)} | ${text(row.regression)} | ${text(row.threshold)} |` ); } - return lines.join('\n'); + const details = renderThresholdDetails(rows); + const table = lines.join('\n'); + return details === '' ? table : `${table}\n\n${details}`; } module.exports = async function validateQueryRegressionComment({ github, context, core }) { diff --git a/.github/scripts/query-regression-comment.test.cjs b/.github/scripts/query-regression-comment.test.cjs index 7a74a0d88c..4b3d1a5cbf 100644 --- a/.github/scripts/query-regression-comment.test.cjs +++ b/.github/scripts/query-regression-comment.test.cjs @@ -187,11 +187,11 @@ test('sorts the base and candidate query union and aggregates scoped thresholds' assert.equal(rows[0].threshold, 'base measurement missing'); assert.equal( rows[1].threshold, - 'candidate measurement missing; p95 [target=base]: warn, absolute [target=candidate, encoding=plain]: pass' + 'candidate measurement missing; p95 [target=base]: warn; absolute [target=candidate, encoding=plain]: pass' ); }); -test('preserves unscoped and unmatched thresholds in a synthetic N/A row', () => { +test('keeps unscoped and unmatched thresholds in a case-thresholds detail row', () => { const rows = collectReportRows(report('thresholds', { base: [{ name: 'measured', latency_ms_median: 10 }], candidate: [{ name: 'measured', latency_ms_median: 11 }], @@ -214,9 +214,10 @@ test('preserves unscoped and unmatched thresholds in a synthetic N/A row', () => assert.equal(rows[0].query, 'measured'); assert.equal(rows[0].threshold, 'query limit [target=base]: passed'); assert.equal(rows[1].query, 'N/A'); + assert.equal(rows[1].kind, 'case-thresholds'); assert.equal( rows[1].threshold, - 'case/storage threshold: min_files [target=base]: passed, min_files [target=candidate]: failed, encoding limit [target=candidate, encoding=plain]: failed; unmatched query not-measured: orphaned limit [target=base, encoding=json]: failed (reason: measurement unavailable)' + 'min_files [target=base]: passed; min_files [target=candidate]: failed; encoding limit [target=candidate, encoding=plain]: failed; unmatched query not-measured: orphaned limit [target=base, encoding=json]: failed (reason: measurement unavailable)' ); }); @@ -239,12 +240,50 @@ test('keeps unscoped thresholds out of undefined and null query rows', () => { assert.equal(byQuery.get('undefined').threshold, 'N/A'); assert.equal(byQuery.get('null').threshold, 'N/A'); assert.equal(rows.filter(row => row.query === 'N/A').length, 1); + assert.equal(byQuery.get('N/A').kind, 'case-thresholds'); assert.equal( byQuery.get('N/A').threshold, - 'case/storage threshold: min_files [target=base]: passed, min_files [target=candidate]: failed' + 'min_files [target=base]: passed; min_files [target=candidate]: failed' ); }); +test('renders case/storage thresholds in a collapsible details block below the table', () => { + const rows = [ + ...collectReportRows(report('first', { + base: [{ name: 'q1', latency_ms_median: 10 }], + candidate: [{ name: 'q1', latency_ms_median: 11 }], + }, [ + { query: 'q1', threshold: 'query limit', target: 'base', status: 'passed' }, + { threshold: 'min_files', target: 'base', status: 'passed' }, + { threshold: 'min_files', target: 'candidate', status: 'failed' }, + ]), '/reports/first/query-regression-report.json'), + ...collectReportRows({ + case: { name: 'broken' }, + status: 'failed', + error: 'connection refused', + }, '/reports/broken/query-regression-report.json'), + ]; + + const rendered = renderSummaryTable(rows); + const detailsIndex = rendered.indexOf('
'); + const table = rendered.slice(0, detailsIndex); + const details = rendered.slice(detailsIndex); + + // Query rows and abnormal N/A rows stay in the main table. + assert.match(table, /\| first \| q1 \| ✅ ok \|/); + assert.match(table, /\| broken \| N\/A \| ❌ failed \|/); + // Storage thresholds are not main-table rows. + assert.doesNotMatch(table, /^\| first \| N\/A \|/m); + assert.doesNotMatch(table, /min_files/); + // Status column is plain text without code-span backticks. + assert.doesNotMatch(table, /`/); + // Details block keeps its tags unescaped and its items '; '-separated. + assert.ok(details.startsWith('
Case / storage thresholds')); + assert.ok(details.includes('- first: min_files \\[target=base\\]: passed; min_files \\[target=candidate\\]: failed')); + assert.ok(details.endsWith('
')); + assert.ok(!rendered.includes('<details>')); +}); + test('escapes Markdown table content, including bare carriage returns', () => { const table = renderSummaryTable([{ caseName: 'safe\r| injected |\n@user ',