diff --git a/scripts/performance/report-markdown.test.ts b/scripts/performance/report-markdown.test.ts index 5655378f9..54d9e7605 100644 --- a/scripts/performance/report-markdown.test.ts +++ b/scripts/performance/report-markdown.test.ts @@ -1,5 +1,9 @@ import { expect, test } from 'bun:test' -import type { MetricComparison, PlatformComparison } from './comparison' +import { + REPORTING_THRESHOLD_PERCENT, + type MetricComparison, + type PlatformComparison, +} from './comparison' import { renderPerformanceReportMarkdown } from './report-markdown' const options = { @@ -33,7 +37,80 @@ function report( ): PlatformComparison { return { platform: 'ios', ...options, suiteComparable, comparisons: metrics } } -test('large Promise changes and disagreeing process pairs remain visible', () => { + +test('restores the original table, emphasis, colors, disclosure, and footer', () => { + const text = renderPerformanceReportMarkdown( + [ + report([ + metric('nitro-cpp/primitive/add-numbers', 20), + metric('nitro-platform/promise/immediate', -12.5), + metric('javascript/primitive/add-numbers', 2), + ]), + ], + options + ) + expect(text).toMatchInlineSnapshot(` + "## Performance Report + + > ⚠️ **Advisory:** Results do not fail this PR. + + ### iOS + + + + + + + + + + + + + + + + + + + + + + + + +
BenchmarkBeforeAfterDifference
C++ addNumbers()100.0 ns120.0 ns🔴 +20% slower
Swift immediatePromise()100.0 ns87.5 ns🟢 -12.5% faster
+ +
+ All Benchmarks + + + + + + + + + + + + + + + + + +
BenchmarkBeforeAfterDifference
JavaScript addNumbers()100.0 ns102.0 ns🔴 +2% slower
+
+ + Benchmarking Code Diff [\`aaaaaaaa\`...\`bbbbbbbb\`](https://github.com/margelo/nitro/compare/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa..bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb) ([view raw output](https://github.com/margelo/nitro/actions/runs/123)) + + Raw measurements: [performance-report-2 (JSON artifact)](https://github.com/margelo/nitro/actions/runs/123/artifacts/987). Run 123, attempt 2. Download requires GitHub access. + " + `) +}) + +test('large Promise changes remain visible without duplicate diagnostic tables', () => { const text = renderPerformanceReportMarkdown( [ report([ @@ -43,28 +120,96 @@ test('large Promise changes and disagreeing process pairs remain visible', () => ], options ) - const main = text.split('
')[0]! + const [main, collapsed] = text.split('
') expect(main).toContain('immediatePromise()') - expect(main).toContain('+20% slower; process pairs disagree') + expect(main).toContain('🔴 +20% slower') expect(main).not.toContain('addNumbers()') - expect(text).toContain('90.0 ns, 110.0 ns') - expect(text).toContain('10.0%, 12.0%') - expect(text).toContain('performance-report-2 (JSON artifact)') - expect(text).toContain('/actions/runs/123/artifacts/987') - expect(text).not.toMatch(/95%|unchanged!|calibrated.*budget/) + expect(collapsed).toContain('addNumbers()') + expect(collapsed).not.toContain('immediatePromise()') + expect(text.match(//g)).toHaveLength(2) + expect(text).not.toMatch(/process pairs|Sample MAD|Base process|95%|noisy/) +}) + +test.each([ + REPORTING_THRESHOLD_PERCENT, + -REPORTING_THRESHOLD_PERCENT, + REPORTING_THRESHOLD_PERCENT - 0.01, + -REPORTING_THRESHOLD_PERCENT + 0.01, +])('preserves the reporting threshold for a %s%% change', (delta) => { + const text = renderPerformanceReportMarkdown( + [report([metric('nitro-cpp/primitive/add-numbers', delta)])], + options + ) + const [main, collapsed] = text.split('
') + const visible = Math.abs(delta) >= REPORTING_THRESHOLD_PERCENT + expect(main!.includes('addNumbers()')).toBe(visible) + expect(collapsed!.includes('addNumbers()')).toBe(!visible) + expect(text.match(/addNumbers\(\)<\/code>/g)).toHaveLength(1) + expect(text).not.toMatch(/unchanged|equal performance|decisive|calibrat/) + if (visible) { + expect(collapsed).toContain( + `Every benchmark reached the ${REPORTING_THRESHOLD_PERCENT}% reporting threshold.` + ) + } else { + expect(main).toContain( + `No observed change reached the ${REPORTING_THRESHOLD_PERCENT}% reporting threshold.` + ) + } }) -test('small observed changes do not claim equality', () => { + +test('equal measurements show zero observed change without bolding either time', () => { const text = renderPerformanceReportMarkdown( [report([metric('nitro-cpp/primitive/add-numbers', 0, [-30, 30])])], options ) - expect(text).toContain('This does not establish equal performance.') - expect(text).toContain('process pairs disagree') + expect(text).toContain('⚪ ~0% observed change') + expect(text.match(/
100.0 ns<\/td>/g)).toHaveLength(2) + expect(text).not.toMatch(/unchanged|equal performance/) }) + +test('keeps iOS before Android and preserves names, escaping, and time units', () => { + const comparison = report([ + { + ...metric('nitro-platform/buffer/copy-1-mib', -6.25), + baseMedianNsPerOp: 1_600_000, + headMedianNsPerOp: 1_500_000, + }, + { + ...metric('turbo-module/primitive/add-numbers', 10), + baseMedianNsPerOp: 1_000, + headMedianNsPerOp: 1_100, + }, + metric('nitro-platform/custom/a-&"\'', 20), + ]) + const platforms: PlatformComparison[] = [ + { ...comparison, platform: 'android' }, + comparison, + ] + const text = renderPerformanceReportMarkdown(platforms, options) + expect(text.indexOf('### iOS')).toBeLessThan(text.indexOf('### Android')) + expect(platforms[0]!.platform).toBe('android') + expect(text).toContain('Swift copy(1 MiB)') + expect(text).toContain('Kotlin copy(1 MiB)') + expect(text).toContain( + 'TurboModule addNumbers()' + ) + expect(text).toContain('1.60 ms') + expect(text).toContain('1.50 ms') + expect(text).toContain('1.00 µs') + expect(text).toContain('1.10 µs') + expect(text).toContain('🟢 -6.25% faster') + expect(text).toContain('a-<b>&"'()') +}) + test('same revision and changed suites are explicit', () => { - expect( - renderPerformanceReportMarkdown([report([], false)], options) - ).toContain('require a new baseline') + const changedSuite = renderPerformanceReportMarkdown( + [report([], false)], + options + ) + expect(changedSuite).toContain( + '> Benchmark definitions changed in this PR. Results require a new baseline and are not compared.' + ) + expect(changedSuite).not.toContain('') expect( renderPerformanceReportMarkdown([], { ...options, @@ -72,3 +217,33 @@ test('same revision and changed suites are explicit', () => { }) ).toContain('Same-revision baseline run') }) + +test('keeps exact run, raw JSON, and platform artifact provenance', () => { + const text = renderPerformanceReportMarkdown([], { + ...options, + artifacts: { + android: { + measurementId: 654, + measurementAttempt: 2, + buildId: 321, + buildAttempt: 1, + }, + ios: { + measurementId: 765, + measurementAttempt: 2, + buildId: 432, + buildAttempt: 1, + }, + }, + }) + expect(text).toContain(`([view raw output](${options.workflowRunUrl}))`) + expect(text).toContain( + `Raw measurements: [performance-report-2 (JSON artifact)](${options.workflowRunUrl}/artifacts/987). Run 123, attempt 2. Download requires GitHub access.` + ) + expect(text).toContain( + `Android: [measurements, attempt 2](${options.workflowRunUrl}/artifacts/654), [apps, attempt 1](${options.workflowRunUrl}/artifacts/321).` + ) + expect(text).toContain( + `iOS: [measurements, attempt 2](${options.workflowRunUrl}/artifacts/765), [apps, attempt 1](${options.workflowRunUrl}/artifacts/432).` + ) +}) diff --git a/scripts/performance/report-markdown.ts b/scripts/performance/report-markdown.ts index a8fa909ef..893e08eb6 100644 --- a/scripts/performance/report-markdown.ts +++ b/scripts/performance/report-markdown.ts @@ -89,24 +89,59 @@ function directionalChange(deltaPercent: number): string { return '~0% observed change' } -function table( +function difference(metric: MetricComparison): string { + if (metric.deltaPercent > 0) + return `🔴 ${directionalChange(metric.deltaPercent)}` + if (metric.deltaPercent < 0) + return `🟢 ${directionalChange(metric.deltaPercent)}` + return `⚪ ${directionalChange(metric.deltaPercent)}` +} + +function measurement( + metric: MetricComparison, + revision: 'base' | 'head' +): string { + const before = metric.baseMedianNsPerOp + const after = metric.headMedianNsPerOp + const value = revision === 'base' ? before : after + const isFaster = revision === 'base' ? before < after : after < before + const formatted = formatNumber(value) + return isFaster ? `${formatted}` : formatted +} + +function renderMetricTable( metrics: readonly MetricComparison[], - platform: PlatformComparison['platform'] + platform: PlatformComparison['platform'], + indentation = 0 ): string { - return [ - '| Benchmark | Base p50 | Head p50 | Observed change |', - '| --- | ---: | ---: | --- |', - ...metrics.map((metric) => { - const pairMin = Math.min(...metric.pairChangesPercent) - const pairMax = Math.max(...metric.pairChangesPercent) - const quality = - pairMin <= -REPORTING_THRESHOLD_PERCENT && - pairMax >= REPORTING_THRESHOLD_PERCENT - ? '; process pairs disagree' - : '' - return `| ${benchmarkName(metric.id, platform)} | ${formatNumber(metric.baseMedianNsPerOp)} | ${formatNumber(metric.headMedianNsPerOp)} | ${directionalChange(metric.deltaPercent)}${quality} |` - }), - ].join('\n') + const indent = ' '.repeat(indentation) + const level1 = ' '.repeat(indentation + 2) + const level2 = ' '.repeat(indentation + 4) + const level3 = ' '.repeat(indentation + 6) + const lines = [ + `${indent}
`, + `${level1}`, + `${level2}`, + `${level3}`, + `${level3}`, + `${level3}`, + `${level3}`, + `${level2}`, + `${level1}`, + `${level1}`, + ] + for (const metric of metrics) { + lines.push( + `${level2}`, + `${level3}`, + `${level3}`, + `${level3}`, + `${level3}`, + `${level2}` + ) + } + lines.push(`${level1}`, `${indent}
BenchmarkBeforeAfterDifference
${benchmarkName(metric.id, platform)}${measurement(metric, 'base')}${measurement(metric, 'head')}${difference(metric)}
`) + return lines.join('\n') } export function renderPerformanceReportMarkdown( @@ -124,7 +159,7 @@ export function renderPerformanceReportMarkdown( const lines = [ '## Performance Report', '', - '> **Report only:** Measurements do not fail this PR. Each benchmark has one base/head process pair. Samples describe within-process variation; they do not establish repeatability between launches or statistical confidence.', + '> ⚠️ **Advisory:** Results do not fail this PR.', ] if (options.baseSha === options.headSha) { lines.push( @@ -138,43 +173,32 @@ export function renderPerformanceReportMarkdown( lines.push('', `### ${platformName(platform.platform)}`, '') if (!platform.suiteComparable) { lines.push( - 'Benchmark definitions changed. Results require a new baseline and are not compared.' + '> Benchmark definitions changed in this PR. Results require a new baseline and are not compared.' ) continue } const changed = platform.comparisons.filter( (metric) => Math.abs(metric.deltaPercent) >= REPORTING_THRESHOLD_PERCENT ) - lines.push( - changed.length === 0 - ? `No observed change reached the ${REPORTING_THRESHOLD_PERCENT}% reporting threshold. This does not establish equal performance.` - : table(changed, platform.platform) + const other = platform.comparisons.filter( + (metric) => Math.abs(metric.deltaPercent) < REPORTING_THRESHOLD_PERCENT ) lines.push( + changed.length === 0 + ? `No observed change reached the ${REPORTING_THRESHOLD_PERCENT}% reporting threshold.` + : renderMetricTable(changed, platform.platform), '', '
', - 'All benchmarks and sample variation', - '', - table(platform.comparisons, platform.platform), - '', - '| Benchmark | Base process p50 | Head process p50 | Paired changes | Sample MAD / p50 (base, head) |', - '| --- | --- | --- | --- | --- |' - ) - for (const metric of platform.comparisons) { - lines.push( - `| ${benchmarkName(metric.id, platform.platform)} | ${metric.baseProcessMedians.map(formatNumber).join(', ')} | ${metric.headProcessMedians.map(formatNumber).join(', ')} | ${metric.pairChangesPercent.map(directionalChange).join(', ')} | ${metric.baseMadPercent.toFixed(1)}%, ${metric.headMadPercent.toFixed(1)}% |` - ) - } - lines.push( - '', - 'p50 is the median of timed batch averages in ns/op, not individual-call latency. MAD describes sample spread; ordered raw samples retain within-process drift.', - '', + ' All Benchmarks', + other.length === 0 + ? `

Every benchmark reached the ${REPORTING_THRESHOLD_PERCENT}% reporting threshold.

` + : renderMetricTable(other, platform.platform, 2), '
' ) } lines.push( '', - `Benchmarking Code Diff [\`${options.baseSha.slice(0, 8)}\`...\`${options.headSha.slice(0, 8)}\`](https://github.com/${options.repository}/compare/${options.baseSha}..${options.headSha})${options.workflowRunUrl == null ? '' : ` ([view CI run](${options.workflowRunUrl}))`}`, + `Benchmarking Code Diff [\`${options.baseSha.slice(0, 8)}\`...\`${options.headSha.slice(0, 8)}\`](https://github.com/${options.repository}/compare/${options.baseSha}..${options.headSha})${options.workflowRunUrl == null ? '' : ` ([view raw output](${options.workflowRunUrl}))`}`, '' ) if (options.artifactId != null && options.workflowRunUrl != null) { diff --git a/scripts/performance/report-validation.test.ts b/scripts/performance/report-validation.test.ts index bb9673ee2..c51936d48 100644 --- a/scripts/performance/report-validation.test.ts +++ b/scripts/performance/report-validation.test.ts @@ -212,11 +212,9 @@ describe('trusted performance report validation', () => { expect(markdown).toContain( 'C++ addNumbers()' ) + expect(markdown).toContain('All Benchmarks') expect(markdown).toContain( - 'All benchmarks and sample variation' - ) - expect(markdown).toContain( - `Benchmarking Code Diff [\`${BASE_SHA.slice(0, 8)}\`...\`${HEAD_SHA.slice(0, 8)}\`](https://github.com/margelo/nitro/compare/${BASE_SHA}..${HEAD_SHA}) ([view CI run](https://github.com/margelo/nitro/actions/runs/123456789))` + `Benchmarking Code Diff [\`${BASE_SHA.slice(0, 8)}\`...\`${HEAD_SHA.slice(0, 8)}\`](https://github.com/margelo/nitro/compare/${BASE_SHA}..${HEAD_SHA}) ([view raw output](https://github.com/margelo/nitro/actions/runs/123456789))` ) const bmf = JSON.parse( await readFile(path.join(fixture.output, 'bencher-ios.json'), 'utf8')