diff --git a/.github/scripts/query-regression-comment.cjs b/.github/scripts/query-regression-comment.cjs index 6f4386fc3b..130b1c7efa 100644 --- a/.github/scripts/query-regression-comment.cjs +++ b/.github/scripts/query-regression-comment.cjs @@ -48,7 +48,7 @@ function text(value) { .replace(/!/g, '\\!') .replace(/@/g, '@\u200b') .replace(/\|/g, '\\|') - .replace(/\r?\n/g, ' '); + .replace(/\r\n|\r|\n/g, ' '); return result; } @@ -56,9 +56,20 @@ function statusEmoji(status) { return { ok: 'โœ…', measured: 'โœ…', failed: 'โŒ', planned: '๐Ÿ“', 'fixture-ready': '๐Ÿงช' }[status] || 'โš ๏ธ'; } -function fmtMs(value) { +function finiteNumber(value) { + if (typeof value !== 'number' && typeof value !== 'string') { + return null; + } + if (typeof value === 'string' && value.trim() === '') { + return null; + } const number = Number(value); - return Number.isFinite(number) ? number.toFixed(2) : 'N/A'; + return Number.isFinite(number) ? number : null; +} + +function fmtMs(value) { + const number = finiteNumber(value); + return number === null ? 'N/A' : number.toFixed(2); } function measurementsByName(target) { @@ -71,51 +82,177 @@ function measurementsByName(target) { } function regression(base, candidate) { - const b = Number(base); - const c = Number(candidate); - if (!Number.isFinite(b) || !Number.isFinite(c) || b === 0) return 'N/A'; + const b = finiteNumber(base); + const c = finiteNumber(candidate); + if (b === null || c === null || b === 0) return 'N/A'; return `${(((c - b) / b) * 100).toFixed(1)}%`; } -function renderReport(report, reportPath) { +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'; +} + +function hasScopedQuery(threshold) { + return threshold?.query !== null + && threshold?.query !== undefined + && String(threshold.query) !== ''; +} + +function hasValue(value) { + return value !== null && value !== undefined && String(value) !== ''; +} + +function formatThreshold(threshold) { + const scope = []; + if (hasValue(threshold?.target)) scope.push(`target=${threshold.target}`); + if (hasValue(threshold?.encoding)) scope.push(`encoding=${threshold.encoding}`); + const name = threshold?.threshold || 'threshold'; + const status = threshold?.status || 'unknown'; + const reason = hasValue(threshold?.reason) ? ` (reason: ${threshold.reason})` : ''; + return `${name}${scope.length > 0 ? ` [${scope.join(', ')}]` : ''}: ${status}${reason}`; +} + +function syntheticThresholdStatus(thresholds, measurementNames) { + const unscoped = []; + const unmatched = new Map(); + for (const threshold of Array.isArray(thresholds) ? thresholds : []) { + if (!hasScopedQuery(threshold)) { + unscoped.push(threshold); + continue; + } + const query = String(threshold.query); + if (!measurementNames.has(query)) { + const entries = unmatched.get(query) || []; + entries.push(threshold); + unmatched.set(query, entries); + } + } + + const parts = []; + if (unscoped.length > 0) { + parts.push(`case/storage threshold: ${thresholdStatus(unscoped)}`); + } + for (const query of Array.from(unmatched.keys()).sort()) { + parts.push(`unmatched query ${query}: ${thresholdStatus(unmatched.get(query))}`); + } + return parts.length > 0 ? parts.join('; ') : 'N/A'; +} + +function joinDetails(...details) { + const present = details.filter(detail => detail && detail !== 'N/A'); + return present.length > 0 ? present.join('; ') : 'N/A'; +} + +function missingMeasurementDetails(base, candidate) { + const details = []; + if (finiteNumber(base?.latency_ms_median) === null) details.push('base measurement missing'); + if (finiteNumber(candidate?.latency_ms_median) === null) details.push('candidate measurement missing'); + return details; +} + +function collectReportRows(report, reportPath) { + const fallbackName = typeof reportPath === 'string' + ? path.basename(path.dirname(reportPath)) || 'unknown' + : 'unknown'; + if (report === null || Array.isArray(report) || typeof report !== 'object') { + return [{ + caseName: fallbackName, + query: 'N/A', + status: 'missing', + baseMedian: 'N/A', + candidateMedian: 'N/A', + regression: 'N/A', + threshold: 'invalid report object', + }]; + } const caseInfo = report.case || {}; - const name = caseInfo.name || path.basename(path.dirname(reportPath)); + const name = caseInfo.name || fallbackName; const status = report.status || 'missing'; - const lines = [ - `### ${statusEmoji(status)} ${text(name)}`, - '', - `- **Status:** \`${text(status)}\``, - `- **Case path:** \`${text(report.case_path)}\``, - `- **Query mode:** \`${text(report.query_mode)}\``, - ]; + const thresholds = Array.isArray(report.thresholds) ? report.thresholds : []; if (report.error) { - lines.push(`- **Error:** \`${text(report.error)}\``); + return [{ + caseName: name, + query: 'N/A', + status, + baseMedian: 'N/A', + candidateMedian: 'N/A', + regression: 'N/A', + threshold: joinDetails(`error: ${report.error}`, syntheticThresholdStatus(thresholds, new Set())), + }]; } const targets = Array.isArray(report.targets) ? report.targets : []; - lines.push('', '| Target | Status | Validation errors | Region |', '| --- | --- | ---: | --- |'); - for (const target of targets) { - const discovered = target?.discovered || {}; - const region = Array.isArray(discovered) - ? discovered.map(item => item?.region_id).filter(Boolean).join(', ') - : discovered.region_id; - lines.push( - `| ${text(target?.name)} | ${statusEmoji(target?.status)} \`${text(target?.status)}\` | ${(target?.validation_errors || []).length} | ${text(region)} |` - ); + if (targets.length < 2) { + return [{ + caseName: name, + query: 'N/A', + status, + baseMedian: 'N/A', + candidateMedian: 'N/A', + regression: 'N/A', + threshold: joinDetails('base/candidate measurements missing', syntheticThresholdStatus(thresholds, new Set())), + }]; } - if (targets.length >= 2) { - const base = measurementsByName(targets[0]); - const candidate = measurementsByName(targets[1]); - const names = Array.from(new Set([...base.keys(), ...candidate.keys()])).sort(); - lines.push('', '| Query | Base median ms | Candidate median ms | Regression |', '| --- | ---: | ---: | ---: |'); - for (const query of names) { - const b = base.get(query) || {}; - const c = candidate.get(query) || {}; - lines.push( - `| ${text(query)} | ${fmtMs(b.latency_ms_median)} | ${fmtMs(c.latency_ms_median)} | ${regression(b.latency_ms_median, c.latency_ms_median)} |` - ); - } + const base = measurementsByName(targets[0]); + const candidate = measurementsByName(targets[1]); + const names = Array.from(new Set([...base.keys(), ...candidate.keys()])).sort(); + if (names.length === 0) { + return [{ + caseName: name, + query: 'N/A', + status, + baseMedian: 'N/A', + candidateMedian: 'N/A', + regression: 'N/A', + threshold: joinDetails('no query measurements found', syntheticThresholdStatus(thresholds, new Set())), + }]; + } + + const measurementNames = new Set(names); + const rows = names.map(query => { + const b = base.get(query) || {}; + const c = candidate.get(query) || {}; + return { + caseName: name, + query, + status, + baseMedian: fmtMs(b.latency_ms_median), + candidateMedian: fmtMs(c.latency_ms_median), + regression: regression(b.latency_ms_median, c.latency_ms_median), + threshold: joinDetails( + ...missingMeasurementDetails(b, c), + thresholdStatus(thresholds, query) + ), + }; + }); + const syntheticThresholds = syntheticThresholdStatus(thresholds, measurementNames); + if (syntheticThresholds !== 'N/A') { + rows.push({ + caseName: name, + query: 'N/A', + status, + baseMedian: 'N/A', + candidateMedian: 'N/A', + regression: 'N/A', + threshold: syntheticThresholds, + }); + } + return rows; +} + +function renderSummaryTable(rows) { + const lines = [ + '| Case | Query | Case status | Base median ms | Candidate median ms | Regression | Threshold |', + '| --- | --- | --- | ---: | ---: | ---: | --- |', + ]; + for (const row of rows) { + 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)} |` + ); } return lines.join('\n'); } @@ -251,7 +388,7 @@ module.exports = async function validateQueryRegressionComment({ github, context if (reportPaths.length === 0) { body += 'No query-regression JSON reports were found in the artifact.\n'; } else { - const rendered = []; + const rows = []; for (const reportPath of reportPaths) { let report; try { @@ -259,9 +396,9 @@ module.exports = async function validateQueryRegressionComment({ github, context } catch (error) { return skip(core, `Invalid report JSON in ${reportPath}: ${error.message}`); } - rendered.push(renderReport(report, reportPath)); + rows.push(...collectReportRows(report, reportPath)); } - body += rendered.join('\n\n---\n\n') + '\n'; + body += renderSummaryTable(rows) + '\n'; } fs.writeFileSync(summaryPath, body); @@ -270,3 +407,5 @@ module.exports = async function validateQueryRegressionComment({ github, context core.setOutput('pr_number', String(prNumber)); core.setOutput('summary_path', summaryPath); }; + +module.exports._test = { collectReportRows, renderSummaryTable }; diff --git a/.github/scripts/query-regression-comment.test.cjs b/.github/scripts/query-regression-comment.test.cjs new file mode 100644 index 0000000000..7a74a0d88c --- /dev/null +++ b/.github/scripts/query-regression-comment.test.cjs @@ -0,0 +1,338 @@ +const fs = require('fs'); +const os = require('os'); +const path = require('path'); +const test = require('node:test'); +const assert = require('node:assert/strict'); + +const handler = require('./query-regression-comment.cjs'); +const { collectReportRows, renderSummaryTable } = handler._test; + +function report(name, measurements, thresholds = []) { + return { + case: { name }, + status: 'ok', + targets: [ + { measurements: measurements.base }, + { measurements: measurements.candidate }, + ], + thresholds, + }; +} + +test('keeps the default export callable and exposes only the test seam', () => { + assert.equal(typeof handler, 'function'); + assert.equal(handler.constructor.name, 'AsyncFunction'); + assert.deepEqual(Object.keys(handler._test).sort(), ['collectReportRows', 'renderSummaryTable']); +}); + +test('renders every case in one summary table without per-case separators', () => { + const rows = [ + ...collectReportRows(report('first', { + base: [{ name: 'q1', latency_ms_median: 10 }], + candidate: [{ name: 'q1', latency_ms_median: 11 }], + }), '/reports/first/query-regression-report.json'), + ...collectReportRows(report('second', { + base: [{ name: 'q2', latency_ms_median: 20 }], + candidate: [{ name: 'q2', latency_ms_median: 18 }], + }), '/reports/second/query-regression-report.json'), + ]; + + const table = renderSummaryTable(rows); + + assert.equal((table.match(/^\| Case \| Query \| Case status \|/gm) || []).length, 1); + assert.match(table, /\| first \| q1 \|/); + assert.match(table, /\| second \| q2 \|/); + assert.doesNotMatch(table, /^### /m); + assert.doesNotMatch(table, /^---$/m); +}); + +test('renders failed reports as N/A rows with their error', () => { + const rows = collectReportRows({ + case: { name: 'broken' }, + status: 'failed', + error: 'connection refused', + }, '/reports/broken/query-regression-report.json'); + + assert.deepEqual(rows, [{ + caseName: 'broken', + query: 'N/A', + status: 'failed', + baseMedian: 'N/A', + candidateMedian: 'N/A', + regression: 'N/A', + threshold: 'error: connection refused', + }]); +}); + +test('reports missing targets and empty measurements', () => { + const missingTargets = collectReportRows({ case: { name: 'missing' }, status: 'failed' }, '/reports/missing/report.json'); + const emptyMeasurements = collectReportRows(report('empty', { base: [], candidate: [] }), '/reports/empty/report.json'); + + assert.equal(missingTargets[0].threshold, 'base/candidate measurements missing'); + assert.equal(emptyMeasurements[0].threshold, 'no query measurements found'); +}); + +test('renders null, array, and primitive reports as invalid report rows', () => { + for (const invalidReport of [null, [], 'not an object']) { + assert.deepEqual( + collectReportRows(invalidReport, '/reports/fallback/query-regression-report.json'), + [{ + caseName: 'fallback', + query: 'N/A', + status: 'missing', + baseMedian: 'N/A', + candidateMedian: 'N/A', + regression: 'N/A', + threshold: 'invalid report object', + }] + ); + } +}); + +test('rejects null medians and diagnoses missing asymmetric measurements', () => { + const rows = collectReportRows(report('missing-values', { + base: [ + { name: 'null-median', latency_ms_median: null }, + { name: 'base-only', latency_ms_median: 10 }, + { name: 'candidate-null', latency_ms_median: 20 }, + ], + candidate: [ + { name: 'null-median', latency_ms_median: 20 }, + { name: 'candidate-null', latency_ms_median: null }, + { name: 'candidate-only', latency_ms_median: 30 }, + ], + }), '/reports/missing-values/query-regression-report.json'); + const byQuery = new Map(rows.map(row => [row.query, row])); + + assert.equal(byQuery.get('null-median').baseMedian, 'N/A'); + assert.equal(byQuery.get('null-median').candidateMedian, '20.00'); + assert.equal(byQuery.get('null-median').regression, 'N/A'); + assert.equal(byQuery.get('null-median').threshold, 'base measurement missing'); + assert.equal(byQuery.get('base-only').threshold, 'candidate measurement missing'); + assert.equal(byQuery.get('candidate-null').candidateMedian, 'N/A'); + assert.equal(byQuery.get('candidate-null').regression, 'N/A'); + assert.equal(byQuery.get('candidate-null').threshold, 'candidate measurement missing'); + assert.equal(byQuery.get('candidate-only').threshold, 'base measurement missing'); +}); + +test('rejects empty, blank, NaN, and infinite medians', () => { + const rows = collectReportRows(report('invalid-values', { + base: [ + { name: 'empty-base', latency_ms_median: '' }, + { name: 'blank-base', latency_ms_median: ' ' }, + { name: 'nan-candidate', latency_ms_median: 10 }, + { name: 'infinite-candidate', latency_ms_median: 10 }, + ], + candidate: [ + { name: 'empty-base', latency_ms_median: 10 }, + { name: 'blank-base', latency_ms_median: 10 }, + { name: 'nan-candidate', latency_ms_median: Number.NaN }, + { name: 'infinite-candidate', latency_ms_median: Number.POSITIVE_INFINITY }, + ], + }), '/reports/invalid-values/query-regression-report.json'); + const byQuery = new Map(rows.map(row => [row.query, row])); + + for (const query of ['empty-base', 'blank-base', 'nan-candidate', 'infinite-candidate']) { + assert.equal(byQuery.get(query).regression, 'N/A'); + } + assert.equal(byQuery.get('empty-base').baseMedian, 'N/A'); + assert.equal(byQuery.get('blank-base').baseMedian, 'N/A'); + assert.equal(byQuery.get('nan-candidate').candidateMedian, 'N/A'); + assert.equal(byQuery.get('infinite-candidate').candidateMedian, 'N/A'); +}); + +test('rejects boolean, array, and object median coercions', () => { + const rows = collectReportRows(report('invalid-types', { + base: [ + { name: 'boolean-base', latency_ms_median: true }, + { name: 'array-base', latency_ms_median: [] }, + { name: 'object-base', latency_ms_median: {} }, + { name: 'boolean-candidate', latency_ms_median: 10 }, + { name: 'array-candidate', latency_ms_median: 10 }, + { name: 'object-candidate', latency_ms_median: 10 }, + ], + candidate: [ + { name: 'boolean-base', latency_ms_median: 10 }, + { name: 'array-base', latency_ms_median: 10 }, + { name: 'object-base', latency_ms_median: 10 }, + { name: 'boolean-candidate', latency_ms_median: false }, + { name: 'array-candidate', latency_ms_median: [] }, + { name: 'object-candidate', latency_ms_median: {} }, + ], + }), '/reports/invalid-types/query-regression-report.json'); + + for (const row of rows) { + assert.equal(row.regression, 'N/A'); + } + assert.equal(rows.find(row => row.query === 'boolean-base').baseMedian, 'N/A'); + assert.equal(rows.find(row => row.query === 'array-base').baseMedian, 'N/A'); + assert.equal(rows.find(row => row.query === 'object-base').baseMedian, 'N/A'); + assert.equal(rows.find(row => row.query === 'boolean-candidate').candidateMedian, 'N/A'); + assert.equal(rows.find(row => row.query === 'array-candidate').candidateMedian, 'N/A'); + assert.equal(rows.find(row => row.query === 'object-candidate').candidateMedian, 'N/A'); +}); + +test('sorts the base and candidate query union and aggregates scoped thresholds', () => { + const rows = collectReportRows(report('union', { + base: [{ name: 'z', latency_ms_median: 10 }], + candidate: [{ name: 'a', latency_ms_median: 20 }], + }, [ + { query: 'z', threshold: 'p95', target: 'base', status: 'warn' }, + { query: 'z', threshold: 'absolute', target: 'candidate', encoding: 'plain', status: 'pass' }, + ]), '/reports/union/query-regression-report.json'); + + assert.deepEqual(rows.map(row => row.query), ['a', 'z']); + assert.equal(rows[0].baseMedian, 'N/A'); + assert.equal(rows[0].candidateMedian, '20.00'); + 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' + ); +}); + +test('preserves unscoped and unmatched thresholds in a synthetic N/A row', () => { + const rows = collectReportRows(report('thresholds', { + base: [{ name: 'measured', latency_ms_median: 10 }], + candidate: [{ name: 'measured', latency_ms_median: 11 }], + }, [ + { query: 'measured', threshold: 'query limit', target: 'base', status: 'passed' }, + { threshold: 'min_files', target: 'base', status: 'passed' }, + { threshold: 'min_files', target: 'candidate', status: 'failed' }, + { threshold: 'encoding limit', target: 'candidate', encoding: 'plain', status: 'failed' }, + { + query: 'not-measured', + threshold: 'orphaned limit', + target: 'base', + encoding: 'json', + status: 'failed', + reason: 'measurement unavailable', + }, + ]), '/reports/thresholds/query-regression-report.json'); + + assert.equal(rows.length, 2); + 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].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)' + ); +}); + +test('keeps unscoped thresholds out of undefined and null query rows', () => { + const rows = collectReportRows(report('collisions', { + base: [ + { name: 'undefined', latency_ms_median: 10 }, + { name: 'null', latency_ms_median: 10 }, + ], + candidate: [ + { name: 'undefined', latency_ms_median: 11 }, + { name: 'null', latency_ms_median: 11 }, + ], + }, [ + { threshold: 'min_files', target: 'base', status: 'passed' }, + { query: null, threshold: 'min_files', target: 'candidate', status: 'failed' }, + ]), '/reports/collisions/query-regression-report.json'); + const byQuery = new Map(rows.map(row => [row.query, row])); + + 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').threshold, + 'case/storage threshold: min_files [target=base]: passed, min_files [target=candidate]: failed' + ); +}); + +test('escapes Markdown table content, including bare carriage returns', () => { + const table = renderSummaryTable([{ + caseName: 'safe\r| injected |\n@user ', + query: 'query`|\n@team', + status: 'failed', + baseMedian: '1|2', + candidateMedian: '3\n4', + regression: '`@all', + threshold: 'x|y\r\n`@here ', + }]); + + assert.equal(table.split('\n').length, 3); + assert.match(table, /safe \\\| injected \\\| @\u200buser <tag>/); + assert.match(table, /query`\\\| @\u200bteam/); + assert.match(table, /1\\\|2/); + assert.match(table, /3 4/); + assert.match(table, /`@\u200ball/); + assert.match(table, /x\\\|y `@\u200bhere <html>/); + assert.doesNotMatch(table, /hidden|comment|drop|\r/); +}); + +test('writes the explicit no-report summary without an empty table', async () => { + const originalCwd = process.cwd(); + const originalRunId = process.env.WORKFLOW_RUN_ID; + const originalRunAttempt = process.env.WORKFLOW_RUN_ATTEMPT; + const temporaryDir = fs.mkdtempSync(path.join(os.tmpdir(), 'query-regression-comment-')); + const artifactDir = path.join(temporaryDir, 'query-regression-comment'); + const outputs = new Map(); + + try { + fs.mkdirSync(artifactDir); + fs.writeFileSync(path.join(artifactDir, 'query-regression-pr.json'), JSON.stringify({ + run_id: 101, + run_attempt: 1, + base_repo: 'owner/repo', + pr_number: 42, + head_sha: 'head-sha', + head_repo: 'fork/repo', + built_base_sha: 'base-sha', + event_base_sha: 'event-base-sha', + candidate_sha: 'candidate-sha', + })); + process.chdir(temporaryDir); + process.env.WORKFLOW_RUN_ID = '101'; + process.env.WORKFLOW_RUN_ATTEMPT = '1'; + + await handler({ + core: { + info() {}, + warning() {}, + setOutput(name, value) { outputs.set(name, value); }, + }, + context: { + repo: { owner: 'owner', repo: 'repo' }, + payload: { + workflow_run: { + event: 'pull_request', + head_sha: 'head-sha', + head_repository: { full_name: 'fork/repo' }, + pull_requests: [{ number: 42 }], + }, + }, + }, + github: { + rest: { + pulls: { + get: async () => ({ + data: { + state: 'open', + base: { repo: { full_name: 'owner/repo' } }, + head: { repo: { full_name: 'fork/repo' }, sha: 'head-sha' }, + }, + }), + }, + }, + }, + }); + + const summary = fs.readFileSync(path.join(artifactDir, 'query-regression-summary.md'), 'utf8'); + assert.equal(outputs.get('should_post'), 'true'); + assert.match(summary, /No query-regression JSON reports were found in the artifact\./); + assert.doesNotMatch(summary, /\| Case \| Query \|/); + } finally { + process.chdir(originalCwd); + if (originalRunId === undefined) delete process.env.WORKFLOW_RUN_ID; + else process.env.WORKFLOW_RUN_ID = originalRunId; + if (originalRunAttempt === undefined) delete process.env.WORKFLOW_RUN_ATTEMPT; + else process.env.WORKFLOW_RUN_ATTEMPT = originalRunAttempt; + fs.rmSync(temporaryDir, { recursive: true, force: true }); + } +}); diff --git a/.github/workflows/develop.yml b/.github/workflows/develop.yml index 0d9eceacb8..d9f6b0c061 100644 --- a/.github/workflows/develop.yml +++ b/.github/workflows/develop.yml @@ -51,6 +51,21 @@ jobs: with: config: licenserc-enterprise.toml + github-script-tests: + if: ${{ github.repository == 'GreptimeTeam/greptimedb' }} + name: GitHub Script Tests + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - uses: actions/checkout@v4 + with: + persist-credentials: false + - uses: actions/setup-node@v4 + with: + node-version: 24 + - name: Run GitHub script tests + run: node --test .github/scripts/query-regression-comment.test.cjs + check: if: ${{ github.repository == 'GreptimeTeam/greptimedb' }} name: Check