From 35f84fc73d80e48700f81c3bd02a31883361845a Mon Sep 17 00:00:00 2001 From: Chris Trzesniewski Date: Sun, 3 Aug 2025 08:58:05 +0200 Subject: [PATCH 01/11] fix: time units are not normalized * extract addBenchmarkEntry function --- src/addBenchmarkEntry.ts | 37 +++++++++++++++++++++++++++++++++++++ src/write.ts | 34 ++++++---------------------------- 2 files changed, 43 insertions(+), 28 deletions(-) create mode 100644 src/addBenchmarkEntry.ts diff --git a/src/addBenchmarkEntry.ts b/src/addBenchmarkEntry.ts new file mode 100644 index 000000000..58b80d92b --- /dev/null +++ b/src/addBenchmarkEntry.ts @@ -0,0 +1,37 @@ +import { Benchmark } from './extract'; +import * as core from '@actions/core'; +import { BenchmarkSuites } from './write'; + +export function addBenchmarkEntry( + benchName: string, + benchEntry: Benchmark, + entries: BenchmarkSuites, + maxItems: number | null, +): { prevBench: Benchmark | null } { + let prevBench: Benchmark | null = null; + + // Add benchmark result + if (entries[benchName] === undefined) { + entries[benchName] = [benchEntry]; + core.debug(`No suite was found for benchmark '${benchName}' in existing data. Created`); + } else { + const suites = entries[benchName]; + // Get last suite which has different commit ID for alert comment + for (const e of suites.slice().reverse()) { + if (e.commit.id !== benchEntry.commit.id) { + prevBench = e; + break; + } + } + + suites.push(benchEntry); + + if (maxItems !== null && suites.length > maxItems) { + suites.splice(0, suites.length - maxItems); + core.debug( + `Number of data items for '${benchName}' was truncated to ${maxItems} due to max-items-in-charts`, + ); + } + } + return { prevBench }; +} diff --git a/src/write.ts b/src/write.ts index e1ad4eafd..048853f22 100644 --- a/src/write.ts +++ b/src/write.ts @@ -9,6 +9,7 @@ import { Config, ToolType } from './config'; import { DEFAULT_INDEX_HTML } from './default_index_html'; import { leavePRComment } from './comment/leavePRComment'; import { leaveCommitComment } from './comment/leaveCommitComment'; +import { addBenchmarkEntry } from './addBenchmarkEntry'; export type BenchmarkSuites = { [name: string]: Benchmark[] }; export interface DataJson { @@ -321,39 +322,16 @@ function addBenchmarkToDataJson( bench: Benchmark, data: DataJson, maxItems: number | null, -): Benchmark | null { +): { prevBench: Benchmark | null } { const repoMetadata = getCurrentRepoMetadata(); const htmlUrl = repoMetadata.html_url ?? ''; - let prevBench: Benchmark | null = null; data.lastUpdate = Date.now(); data.repoUrl = htmlUrl; - // Add benchmark result - if (data.entries[benchName] === undefined) { - data.entries[benchName] = [bench]; - core.debug(`No suite was found for benchmark '${benchName}' in existing data. Created`); - } else { - const suites = data.entries[benchName]; - // Get last suite which has different commit ID for alert comment - for (const e of suites.slice().reverse()) { - if (e.commit.id !== bench.commit.id) { - prevBench = e; - break; - } - } - - suites.push(bench); - - if (maxItems !== null && suites.length > maxItems) { - suites.splice(0, suites.length - maxItems); - core.debug( - `Number of data items for '${benchName}' was truncated to ${maxItems} due to max-items-in-charts`, - ); - } - } + const { prevBench } = addBenchmarkEntry(benchName, bench, data.entries, maxItems); - return prevBench; + return { prevBench }; } function isRemoteRejectedError(err: unknown): err is Error { @@ -421,7 +399,7 @@ async function writeBenchmarkToGitHubPagesWithRetry( await io.mkdirP(benchmarkDataDirFullPath); const data = await loadDataJs(dataPath); - const prevBench = addBenchmarkToDataJson(name, bench, data, maxItemsInChart); + const { prevBench } = addBenchmarkToDataJson(name, bench, data, maxItemsInChart); await storeDataJs(dataPath, data); @@ -511,7 +489,7 @@ async function writeBenchmarkToExternalJson( ): Promise { const { name, maxItemsInChart, saveDataFile } = config; const data = await loadDataJson(jsonFilePath); - const prevBench = addBenchmarkToDataJson(name, bench, data, maxItemsInChart); + const { prevBench } = addBenchmarkToDataJson(name, bench, data, maxItemsInChart); if (!saveDataFile) { core.debug('Skipping storing benchmarks in external data file'); From 94ba2098db420aaf726d6f8f405b0bf2d210a5e4 Mon Sep 17 00:00:00 2001 From: Chris Trzesniewski Date: Sun, 3 Aug 2025 11:52:16 +0200 Subject: [PATCH 02/11] fix: time units are not normalized * normalize new entry values to match last entry units --- src/addBenchmarkEntry.ts | 10 +- src/extractRangeInfo.ts | 24 +++++ src/normalizeBenchmark.ts | 20 ++++ src/normalizeBenchmarkResult.ts | 30 ++++++ src/normalizeUnit.ts | 10 ++ src/write.ts | 33 +++--- test/extractRangeInfo.spec.ts | 28 +++++ test/normalizeBenchmarkResult.spec.ts | 144 ++++++++++++++++++++++++++ test/normalizeUnit.spec.ts | 20 ++++ test/write.spec.ts | 114 ++++++++++++++++---- 10 files changed, 392 insertions(+), 41 deletions(-) create mode 100644 src/extractRangeInfo.ts create mode 100644 src/normalizeBenchmark.ts create mode 100644 src/normalizeBenchmarkResult.ts create mode 100644 src/normalizeUnit.ts create mode 100644 test/extractRangeInfo.spec.ts create mode 100644 test/normalizeBenchmarkResult.spec.ts create mode 100644 test/normalizeUnit.spec.ts diff --git a/src/addBenchmarkEntry.ts b/src/addBenchmarkEntry.ts index 58b80d92b..3c782f1f3 100644 --- a/src/addBenchmarkEntry.ts +++ b/src/addBenchmarkEntry.ts @@ -1,14 +1,16 @@ import { Benchmark } from './extract'; import * as core from '@actions/core'; import { BenchmarkSuites } from './write'; +import { normalizeBenchmark } from './normalizeBenchmark'; export function addBenchmarkEntry( benchName: string, benchEntry: Benchmark, entries: BenchmarkSuites, maxItems: number | null, -): { prevBench: Benchmark | null } { +): { prevBench: Benchmark | null; normalizedCurrentBench: Benchmark } { let prevBench: Benchmark | null = null; + let normalizedCurrentBench: Benchmark = benchEntry; // Add benchmark result if (entries[benchName] === undefined) { @@ -24,7 +26,9 @@ export function addBenchmarkEntry( } } - suites.push(benchEntry); + normalizedCurrentBench = normalizeBenchmark(prevBench, benchEntry); + + suites.push(normalizedCurrentBench); if (maxItems !== null && suites.length > maxItems) { suites.splice(0, suites.length - maxItems); @@ -33,5 +37,5 @@ export function addBenchmarkEntry( ); } } - return { prevBench }; + return { prevBench, normalizedCurrentBench }; } diff --git a/src/extractRangeInfo.ts b/src/extractRangeInfo.ts new file mode 100644 index 000000000..01738e6f0 --- /dev/null +++ b/src/extractRangeInfo.ts @@ -0,0 +1,24 @@ +export function extractRangeInfo(range: string | undefined): { prefix: string; value: number } | undefined { + if (!range) { + return undefined; + } + + const matches = range.match(/(?(\+-|±)\s*)(?\d.+)/); + + if (!matches) { + return undefined; + } + + const valueString = matches.groups?.['value']; + + const value = Number(valueString); + + if (isNaN(value)) { + return undefined; + } + + return { + value, + prefix: matches.groups?.prefix ?? '', + }; +} diff --git a/src/normalizeBenchmark.ts b/src/normalizeBenchmark.ts new file mode 100644 index 000000000..41506c595 --- /dev/null +++ b/src/normalizeBenchmark.ts @@ -0,0 +1,20 @@ +import { Benchmark } from './extract'; +import { normalizeBenchmarkResult } from './normalizeBenchmarkResult'; + +export function normalizeBenchmark(prevBench: Benchmark | null, currentBench: Benchmark): Benchmark { + if (!prevBench) { + return currentBench; + } + + return { + ...currentBench, + benches: currentBench.benches + .map((currentBenchResult) => ({ + currentBenchResult, + prevBenchResult: prevBench.benches.find((result) => result.name === currentBenchResult.name), + })) + .map(({ currentBenchResult, prevBenchResult }) => + normalizeBenchmarkResult(prevBenchResult, currentBenchResult), + ), + }; +} diff --git a/src/normalizeBenchmarkResult.ts b/src/normalizeBenchmarkResult.ts new file mode 100644 index 000000000..d18caac55 --- /dev/null +++ b/src/normalizeBenchmarkResult.ts @@ -0,0 +1,30 @@ +import { BenchmarkResult } from './extract'; +import { normalizeUnit } from './normalizeUnit'; +import { extractRangeInfo } from './extractRangeInfo'; + +export function normalizeBenchmarkResult( + prevBenchResult: BenchmarkResult | undefined | null, + currenBenchResult: BenchmarkResult, +): BenchmarkResult { + if (!prevBenchResult) { + return currenBenchResult; + } + + const prevUnit = prevBenchResult.unit; + const currentUnit = currenBenchResult.unit; + const currentRange = currenBenchResult.range; + const currentRangeInfo = extractRangeInfo(currentRange); + + const normalizedValue = normalizeUnit(prevUnit, currentUnit, currenBenchResult.value); + const normalizedUnit = currenBenchResult.value !== normalizedValue ? prevUnit : currentUnit; + const normalizedRangeInfo = currentRangeInfo + ? { prefix: currentRangeInfo.prefix, value: normalizeUnit(prevUnit, currentUnit, currentRangeInfo.value) } + : undefined; + + return { + ...currenBenchResult, + value: normalizedValue, + unit: normalizedUnit, + range: normalizedRangeInfo ? `${normalizedRangeInfo.prefix}${normalizedRangeInfo.value}` : currentRange, + }; +} diff --git a/src/normalizeUnit.ts b/src/normalizeUnit.ts new file mode 100644 index 000000000..643ffdfef --- /dev/null +++ b/src/normalizeUnit.ts @@ -0,0 +1,10 @@ +export function normalizeUnit(prevUnit: string, currentUnit: string, value: number): number { + const prevUnitIndex = TIME_UNITS.indexOf(prevUnit); + const currentUnitIndex = TIME_UNITS.indexOf(currentUnit); + + const unitDiff = prevUnitIndex - currentUnitIndex; + + return value * 1000 ** unitDiff; +} + +const TIME_UNITS = ['s', 'ms', 'us', 'ns']; diff --git a/src/write.ts b/src/write.ts index 048853f22..4e9ebad6c 100644 --- a/src/write.ts +++ b/src/write.ts @@ -322,16 +322,16 @@ function addBenchmarkToDataJson( bench: Benchmark, data: DataJson, maxItems: number | null, -): { prevBench: Benchmark | null } { +): { prevBench: Benchmark | null; normalizedCurrentBench: Benchmark } { const repoMetadata = getCurrentRepoMetadata(); const htmlUrl = repoMetadata.html_url ?? ''; data.lastUpdate = Date.now(); data.repoUrl = htmlUrl; - const { prevBench } = addBenchmarkEntry(benchName, bench, data.entries, maxItems); + const { prevBench, normalizedCurrentBench } = addBenchmarkEntry(benchName, bench, data.entries, maxItems); - return { prevBench }; + return { prevBench, normalizedCurrentBench }; } function isRemoteRejectedError(err: unknown): err is Error { @@ -345,7 +345,7 @@ async function writeBenchmarkToGitHubPagesWithRetry( bench: Benchmark, config: Config, retry: number, -): Promise { +): Promise<{ prevBench: Benchmark | null; normalizedCurrentBench: Benchmark }> { const { name, tool, @@ -399,7 +399,7 @@ async function writeBenchmarkToGitHubPagesWithRetry( await io.mkdirP(benchmarkDataDirFullPath); const data = await loadDataJs(dataPath); - const { prevBench } = addBenchmarkToDataJson(name, bench, data, maxItemsInChart); + const { prevBench, normalizedCurrentBench } = addBenchmarkToDataJson(name, bench, data, maxItemsInChart); await storeDataJs(dataPath, data); @@ -447,10 +447,13 @@ async function writeBenchmarkToGitHubPagesWithRetry( ); } - return prevBench; + return { prevBench, normalizedCurrentBench }; } -async function writeBenchmarkToGitHubPages(bench: Benchmark, config: Config): Promise { +async function writeBenchmarkToGitHubPages( + bench: Benchmark, + config: Config, +): Promise<{ prevBench: Benchmark | null; normalizedCurrentBench: Benchmark }> { const { ghPagesBranch, skipFetchGhPages, ghRepository, githubToken } = config; if (!ghRepository) { if (!skipFetchGhPages) { @@ -486,14 +489,14 @@ async function writeBenchmarkToExternalJson( bench: Benchmark, jsonFilePath: string, config: Config, -): Promise { +): Promise<{ prevBench: Benchmark | null; normalizedCurrentBench: Benchmark }> { const { name, maxItemsInChart, saveDataFile } = config; const data = await loadDataJson(jsonFilePath); - const { prevBench } = addBenchmarkToDataJson(name, bench, data, maxItemsInChart); + const { prevBench, normalizedCurrentBench } = addBenchmarkToDataJson(name, bench, data, maxItemsInChart); if (!saveDataFile) { core.debug('Skipping storing benchmarks in external data file'); - return prevBench; + return { prevBench, normalizedCurrentBench }; } try { @@ -504,12 +507,12 @@ async function writeBenchmarkToExternalJson( throw new Error(`Could not store benchmark data as JSON at ${jsonFilePath}: ${err}`); } - return prevBench; + return { prevBench, normalizedCurrentBench }; } export async function writeBenchmark(bench: Benchmark, config: Config) { const { name, externalDataJsonPath } = config; - const prevBench = externalDataJsonPath + const { prevBench, normalizedCurrentBench } = externalDataJsonPath ? await writeBenchmarkToExternalJson(bench, externalDataJsonPath, config) : await writeBenchmarkToGitHubPages(bench, config); @@ -518,9 +521,9 @@ export async function writeBenchmark(bench: Benchmark, config: Config) { if (prevBench === null) { core.debug('Alert check was skipped because previous benchmark result was not found'); } else { - await handleComment(name, bench, prevBench, config); - await handleSummary(name, bench, prevBench, config); - await handleAlert(name, bench, prevBench, config); + await handleComment(name, normalizedCurrentBench, prevBench, config); + await handleSummary(name, normalizedCurrentBench, prevBench, config); + await handleAlert(name, normalizedCurrentBench, prevBench, config); } } diff --git a/test/extractRangeInfo.spec.ts b/test/extractRangeInfo.spec.ts new file mode 100644 index 000000000..1253fadff --- /dev/null +++ b/test/extractRangeInfo.spec.ts @@ -0,0 +1,28 @@ +import { extractRangeInfo } from '../src/extractRangeInfo'; + +describe('extractRangeInfo', () => { + it("should extract range with '±'", () => { + expect(extractRangeInfo('±20')).toEqual({ value: 20, prefix: '±' }); + }); + + it("should extract range with '± '", () => { + expect(extractRangeInfo('± 20')).toEqual({ value: 20, prefix: '± ' }); + }); + + it("should extract range with '+-'", () => { + expect(extractRangeInfo('+-20')).toEqual({ value: 20, prefix: '+-' }); + }); + + it("should extract range with '+- '", () => { + expect(extractRangeInfo('+- 20')).toEqual({ value: 20, prefix: '+- ' }); + }); + + it('should NOT extract range with unknown prefix', () => { + expect(extractRangeInfo('unknown prefix 20')).toBeUndefined(); + }); + + it('should NOT extract range with invalid number', () => { + expect(extractRangeInfo('± boo')).toBeUndefined(); + expect(extractRangeInfo('+- boo')).toBeUndefined(); + }); +}); diff --git a/test/normalizeBenchmarkResult.spec.ts b/test/normalizeBenchmarkResult.spec.ts new file mode 100644 index 000000000..646df4138 --- /dev/null +++ b/test/normalizeBenchmarkResult.spec.ts @@ -0,0 +1,144 @@ +import { normalizeBenchmarkResult } from '../src/normalizeBenchmarkResult'; + +describe('normalizeBenchmarkResult', () => { + describe('with range (±)', () => { + it('normalize smaller when new unit is larger', () => { + expect( + normalizeBenchmarkResult( + { name: 'Bench', value: 900, unit: 'ms', range: '± 20' }, + { name: 'Bench', value: 1.01, unit: 's', range: '± 0.1' }, + ), + ).toEqual({ name: 'Bench', value: 1_010, unit: 'ms', range: '± 100' }); + expect( + normalizeBenchmarkResult( + { name: 'Bench', value: 900, unit: 'us', range: '± 20' }, + { name: 'Bench', value: 1.01, unit: 's', range: '± 0.1' }, + ), + ).toEqual({ name: 'Bench', value: 1_010_000, unit: 'us', range: '± 100000' }); + expect( + normalizeBenchmarkResult( + { name: 'Bench', value: 900, unit: 'ns', range: '± 20' }, + { name: 'Bench', value: 1.01, unit: 's', range: '± 0.1' }, + ), + ).toEqual({ name: 'Bench', value: 1_010_000_000, unit: 'ns', range: '± 100000000' }); + }); + + it('normalize smaller when new unit is smaller', () => { + expect( + normalizeBenchmarkResult( + { name: 'Bench', value: 1.01, unit: 's', range: '± 0.1' }, + { name: 'Bench', value: 900, unit: 'ms', range: '± 20' }, + ), + ).toEqual({ name: 'Bench', value: 0.9, unit: 's', range: '± 0.02' }); + expect( + normalizeBenchmarkResult( + { name: 'Bench', value: 10_000.01, unit: 's', range: '± 10' }, + { name: 'Bench', value: 9_000_000, unit: 'us', range: '± 2000000' }, + ), + ).toEqual({ name: 'Bench', value: 9, unit: 's', range: '± 2' }); + }); + + it('NOT normalize when new unit is not supported', () => { + expect( + normalizeBenchmarkResult( + { name: 'Bench', value: 900, unit: 'unknown1', range: '± 20' }, + { name: 'Bench', value: 1.01, unit: 'unknown2', range: '± 0.1' }, + ), + ).toEqual({ name: 'Bench', value: 1.01, unit: 'unknown2', range: '± 0.1' }); + }); + }); + + describe('with range (+-)', () => { + it('normalize smaller when new unit is larger', () => { + expect( + normalizeBenchmarkResult( + { name: 'Bench', value: 900, unit: 'ms', range: '+- 20' }, + { name: 'Bench', value: 1.01, unit: 's', range: '+- 0.1' }, + ), + ).toEqual({ name: 'Bench', value: 1_010, unit: 'ms', range: '+- 100' }); + expect( + normalizeBenchmarkResult( + { name: 'Bench', value: 900, unit: 'us', range: '+- 20' }, + { name: 'Bench', value: 1.01, unit: 's', range: '+- 0.1' }, + ), + ).toEqual({ name: 'Bench', value: 1_010_000, unit: 'us', range: '+- 100000' }); + expect( + normalizeBenchmarkResult( + { name: 'Bench', value: 900, unit: 'ns', range: '+- 20' }, + { name: 'Bench', value: 1.01, unit: 's', range: '+- 0.1' }, + ), + ).toEqual({ name: 'Bench', value: 1_010_000_000, unit: 'ns', range: '+- 100000000' }); + }); + + it('normalize smaller when new unit is smaller', () => { + expect( + normalizeBenchmarkResult( + { name: 'Bench', value: 1.01, unit: 's', range: '+- 0.1' }, + { name: 'Bench', value: 900, unit: 'ms', range: '+- 20' }, + ), + ).toEqual({ name: 'Bench', value: 0.9, unit: 's', range: '+- 0.02' }); + expect( + normalizeBenchmarkResult( + { name: 'Bench', value: 10_000.01, unit: 's', range: '+- 10' }, + { name: 'Bench', value: 9_000_000, unit: 'us', range: '+- 2000000' }, + ), + ).toEqual({ name: 'Bench', value: 9, unit: 's', range: '+- 2' }); + }); + + it('NOT normalize when new unit is not supported', () => { + expect( + normalizeBenchmarkResult( + { name: 'Bench', value: 900, unit: 'unknown1', range: '± 20' }, + { name: 'Bench', value: 1.01, unit: 'unknown2', range: '± 0.1' }, + ), + ).toEqual({ name: 'Bench', value: 1.01, unit: 'unknown2', range: '± 0.1' }); + }); + }); + + describe('without range', () => { + it('normalize smaller when new unit is larger', () => { + expect( + normalizeBenchmarkResult( + { name: 'Bench', value: 900, unit: 'ms' }, + { name: 'Bench', value: 1.01, unit: 's' }, + ), + ).toEqual({ name: 'Bench', value: 1_010, unit: 'ms' }); + expect( + normalizeBenchmarkResult( + { name: 'Bench', value: 900, unit: 'us' }, + { name: 'Bench', value: 1.01, unit: 's' }, + ), + ).toEqual({ name: 'Bench', value: 1_010_000, unit: 'us' }); + expect( + normalizeBenchmarkResult( + { name: 'Bench', value: 900, unit: 'ns' }, + { name: 'Bench', value: 1.01, unit: 's' }, + ), + ).toEqual({ name: 'Bench', value: 1_010_000_000, unit: 'ns' }); + }); + + it('normalize smaller when new unit is smaller', () => { + expect( + normalizeBenchmarkResult( + { name: 'Bench', value: 1.01, unit: 's' }, + { name: 'Bench', value: 900, unit: 'ms' }, + ), + ).toEqual({ name: 'Bench', value: 0.9, unit: 's' }); + expect( + normalizeBenchmarkResult( + { name: 'Bench', value: 10_000.01, unit: 's' }, + { name: 'Bench', value: 9_000_000, unit: 'us' }, + ), + ).toEqual({ name: 'Bench', value: 9, unit: 's' }); + }); + + it('NOT normalize when new unit is not supported', () => { + expect( + normalizeBenchmarkResult( + { name: 'Bench', value: 900, unit: 'unknown1' }, + { name: 'Bench', value: 1.01, unit: 'unknown2' }, + ), + ).toEqual({ name: 'Bench', value: 1.01, unit: 'unknown2' }); + }); + }); +}); diff --git a/test/normalizeUnit.spec.ts b/test/normalizeUnit.spec.ts new file mode 100644 index 000000000..58b1fa0a0 --- /dev/null +++ b/test/normalizeUnit.spec.ts @@ -0,0 +1,20 @@ +import { normalizeUnit } from '../src/normalizeUnit'; + +describe('normalizeUnit', () => { + it('normalize smaller when new unit is larger', () => { + expect(normalizeUnit('ms', 's', 12)).toBe(12_000); + expect(normalizeUnit('us', 's', 12)).toBe(12_000_000); + expect(normalizeUnit('ns', 's', 12)).toBe(12_000_000_000); + }); + + it('normalize smaller when new unit is smaller', () => { + expect(normalizeUnit('s', 'ms', 12)).toBe(0.012); + expect(normalizeUnit('s', 'us', 12)).toBe(0.000_012); + expect(normalizeUnit('s', 'ns', 12_000_000_000)).toBe(12); + }); + + it('NOT normalize when new unit is not supported', () => { + expect(normalizeUnit('unknown1', 'unknown2', 12)).toBe(12); + expect(normalizeUnit('ops/s', 'ops/us', 12)).toBe(12); + }); +}); diff --git a/test/write.spec.ts b/test/write.spec.ts index e88543cbb..40f4b4441 100644 --- a/test/write.spec.ts +++ b/test/write.spec.ts @@ -211,6 +211,7 @@ describe.each(['https://github.com', 'https://github.enterprise.corp'])('writeBe config: Config; data: DataJson | null; added: Benchmark; + expectedAdded?: Benchmark; error?: string[]; commitComment?: string; repoPayload?: null | RepositoryPayloadSubset; @@ -241,6 +242,68 @@ describe.each(['https://github.com', 'https://github.enterprise.corp'])('writeBe }, gitServerUrl: serverUrl, }, + { + it: 'appends new result to existing data with normalized units - new unit smaller', + config: { ...defaultCfg, tool: 'catch2' }, + data: { + lastUpdate, + repoUrl, + entries: { + 'Test benchmark': [ + { + commit: commit('prev commit id'), + date: lastUpdate - 1000, + tool: 'catch2', + benches: [bench('bench_fib_10', 1.012, '± 0.02', 'ms')], + }, + ], + }, + }, + added: { + commit: commit('current commit id'), + date: lastUpdate, + tool: 'catch2', + benches: [bench('bench_fib_10', 990, '± 20', 'us')], + }, + expectedAdded: { + commit: commit('current commit id'), + date: lastUpdate, + tool: 'catch2', + benches: [bench('bench_fib_10', 0.99, '± 0.02', 'ms')], + }, + gitServerUrl: serverUrl, + }, + { + it: 'appends new result to existing data with normalized units - new unit larger', + config: { ...defaultCfg, tool: 'catch2' }, + data: { + lastUpdate, + repoUrl, + entries: { + 'Test benchmark': [ + { + commit: commit('prev commit id'), + date: lastUpdate - 1000, + tool: 'catch2', + benches: [bench('bench_fib_10', 990, '± 20', 'us')], + }, + ], + }, + }, + added: { + commit: commit('current commit id'), + date: lastUpdate, + tool: 'catch2', + benches: [bench('bench_fib_10', 1.012, '± 0.02', 'ms')], + }, + expectedAdded: { + commit: commit('current commit id'), + date: lastUpdate, + tool: 'catch2', + benches: [bench('bench_fib_10', 1012, '± 20', 'us')], + }, + gitServerUrl: serverUrl, + }, { it: 'creates new data file', config: defaultCfg, @@ -709,23 +772,28 @@ describe.each(['https://github.com', 'https://github.enterprise.corp'])('writeBe ]; it.each(normalCases)('$it', async function (t) { + const { data, added, config, repoPayload, error, commitComment } = t; + const expectedAdded = t.expectedAdded ?? added; + added.benches; // ? + expectedAdded.benches; // ? + gitHubContext.payload.repository = { private: false, html_url: `${serverUrl}/user/repo`, } as RepositoryPayloadSubset | null; - if (t.repoPayload !== undefined) { - gitHubContext.payload.repository = t.repoPayload; + if (repoPayload !== undefined) { + gitHubContext.payload.repository = repoPayload; } - if (t.data !== null) { - await fs.writeFile(dataJson, JSON.stringify(t.data), 'utf8'); + if (data !== null) { + await fs.writeFile(dataJson, JSON.stringify(data), 'utf8'); } let caughtError: Error | null = null; try { - await writeBenchmark(t.added, t.config); + await writeBenchmark(added, config); } catch (err: any) { - if (!t.error && !t.commitComment) { + if (!error && !commitComment) { throw err; } caughtError = err; @@ -734,24 +802,24 @@ describe.each(['https://github.com', 'https://github.enterprise.corp'])('writeBe const json: DataJson = JSON.parse(await fs.readFile(dataJson, 'utf8')); expect('number').toEqual(typeof json.lastUpdate); - expect(json.entries[t.config.name]).toBeTruthy(); - const len = json.entries[t.config.name].length; + expect(json.entries[config.name]).toBeTruthy(); + const len = json.entries[config.name].length; ok(len > 0); - expect(t.added).toEqual(json.entries[t.config.name][len - 1]); // Check last item is the newest + expect(expectedAdded).toEqual(json.entries[config.name][len - 1]); // Check the last item is the newest - if (t.data !== null) { - ok(json.lastUpdate > t.data.lastUpdate); - expect(t.data.repoUrl).toEqual(json.repoUrl); - for (const name of Object.keys(t.data.entries)) { - const entries = t.data.entries[name]; - if (name === t.config.name) { - if (t.config.maxItemsInChart === null || len < t.config.maxItemsInChart) { + if (data !== null) { + ok(json.lastUpdate > data.lastUpdate); + expect(data.repoUrl).toEqual(json.repoUrl); + for (const name of Object.keys(data.entries)) { + const entries = data.entries[name]; + if (name === config.name) { + if (config.maxItemsInChart === null || len < config.maxItemsInChart) { expect(entries.length + 1).toEqual(len); // Check benchmark data except for the last appended one are not modified expect(entries).toEqual(json.entries[name].slice(0, -1)); } else { - // When data items was truncated due to max-items-in-chart - expect(entries.length).toEqual(len); // Number of items did not change because first item was shifted + // When data items were truncated due to max-items-in-chart + expect(entries.length).toEqual(len); // The Number of items did not change because the first item was shifted expect(entries.slice(1)).toEqual(json.entries[name].slice(0, -1)); } } else { @@ -760,13 +828,13 @@ describe.each(['https://github.com', 'https://github.enterprise.corp'])('writeBe } } - if (t.error) { + if (error) { ok(caughtError); - const expected = t.error.join('\n'); + const expected = error.join('\n'); expect(caughtError.message).toEqual(expected); } - if (t.commitComment !== undefined) { + if (commitComment !== undefined) { ok(caughtError); // Last line is appended only for failure message const messageLines = caughtError.message.split('\n'); @@ -782,7 +850,7 @@ describe.each(['https://github.com', 'https://github.enterprise.corp'])('writeBe expect('current commit id').toEqual(opts.commit_sha); expect(expectedMessage).toEqual(opts.body); const commentLine = messageLines[messageLines.length - 1]; - expect(t.commitComment).toEqual(commentLine); + expect(commitComment).toEqual(commentLine); // Check the body is a correct markdown document by markdown parser // Validate markdown content via HTML @@ -795,7 +863,7 @@ describe.each(['https://github.com', 'https://github.enterprise.corp'])('writeBe expect(':warning: Performance Alert :warning:').toEqual(h1.text()); const tr = query('tbody tr'); - expect(t.added.benches.length).toEqual(tr.length); + expect(added.benches.length).toEqual(tr.length); const a = query('a'); expect(2).toEqual(a.length); From 38ce1873f73c9c2f4cc5bb09ec26150d922fd558 Mon Sep 17 00:00:00 2001 From: Chris Trzesniewski Date: Sun, 3 Aug 2025 12:11:37 +0200 Subject: [PATCH 03/11] fix: time units are not normalized * normalize ops per time unit --- src/normalizeUnit.ts | 18 ++++++++++++++++-- test/normalizeUnit.spec.ts | 17 ++++++++++++++--- 2 files changed, 30 insertions(+), 5 deletions(-) diff --git a/src/normalizeUnit.ts b/src/normalizeUnit.ts index 643ffdfef..8e18bbd06 100644 --- a/src/normalizeUnit.ts +++ b/src/normalizeUnit.ts @@ -2,9 +2,23 @@ export function normalizeUnit(prevUnit: string, currentUnit: string, value: numb const prevUnitIndex = TIME_UNITS.indexOf(prevUnit); const currentUnitIndex = TIME_UNITS.indexOf(currentUnit); - const unitDiff = prevUnitIndex - currentUnitIndex; + if (prevUnitIndex >= 0 && currentUnitIndex >= 0) { + const unitDiff = prevUnitIndex - currentUnitIndex; - return value * 1000 ** unitDiff; + return value * 1000 ** unitDiff; + } + + const prevUnitIndex2 = OPS_PER_TIME_UNIT.indexOf(prevUnit); + const currentUnitIndex2 = OPS_PER_TIME_UNIT.indexOf(currentUnit); + + if (prevUnitIndex2 >= 0 && currentUnitIndex2 >= 0) { + const unitDiff = prevUnitIndex2 - currentUnitIndex2; + + return value * 1000 ** unitDiff; + } + + return value; } const TIME_UNITS = ['s', 'ms', 'us', 'ns']; +const OPS_PER_TIME_UNIT = [...TIME_UNITS].reverse().map((unit) => `ops/${unit}`); diff --git a/test/normalizeUnit.spec.ts b/test/normalizeUnit.spec.ts index 58b1fa0a0..09fa20456 100644 --- a/test/normalizeUnit.spec.ts +++ b/test/normalizeUnit.spec.ts @@ -1,20 +1,31 @@ import { normalizeUnit } from '../src/normalizeUnit'; describe('normalizeUnit', () => { - it('normalize smaller when new unit is larger', () => { + it('normalize smaller when new unit is larger - time units', () => { expect(normalizeUnit('ms', 's', 12)).toBe(12_000); expect(normalizeUnit('us', 's', 12)).toBe(12_000_000); expect(normalizeUnit('ns', 's', 12)).toBe(12_000_000_000); }); - it('normalize smaller when new unit is smaller', () => { + it('normalize smaller when new unit is smaller - time units', () => { expect(normalizeUnit('s', 'ms', 12)).toBe(0.012); expect(normalizeUnit('s', 'us', 12)).toBe(0.000_012); expect(normalizeUnit('s', 'ns', 12_000_000_000)).toBe(12); }); + it('normalize smaller when new unit is smaller - ops per time units', () => { + expect(normalizeUnit('ops/ms', 'ops/s', 12)).toBe(0.012); + expect(normalizeUnit('ops/us', 'ops/s', 12)).toBe(0.000_012); + expect(normalizeUnit('ops/ns', 'ops/s', 12_000_000_000)).toBe(12); + }); + + it('normalize smaller when new unit is larger - ops per time units', () => { + expect(normalizeUnit('ops/s', 'ops/ms', 12)).toBe(12_000); + expect(normalizeUnit('ops/s', 'ops/us', 12)).toBe(12_000_000); + expect(normalizeUnit('ops/s', 'ops/ns', 12)).toBe(12_000_000_000); + }); + it('NOT normalize when new unit is not supported', () => { expect(normalizeUnit('unknown1', 'unknown2', 12)).toBe(12); - expect(normalizeUnit('ops/s', 'ops/us', 12)).toBe(12); }); }); From 61ac5f52f88342da1f60cf4726bef87602148a31 Mon Sep 17 00:00:00 2001 From: Chris Trzesniewski Date: Sat, 30 Aug 2025 17:20:52 +0200 Subject: [PATCH 04/11] fix: time units are not normalized * handle /iter unit * add tests for write function --- src/normalizeUnit.ts | 14 ++++++- test/normalizeBenchmarkResult.spec.ts | 32 +++++++++++++++ test/normalizeUnit.spec.ts | 12 ++++++ test/write.spec.ts | 58 ++++++++++++++++++++++++++- 4 files changed, 112 insertions(+), 4 deletions(-) diff --git a/src/normalizeUnit.ts b/src/normalizeUnit.ts index 8e18bbd06..f16df1bf7 100644 --- a/src/normalizeUnit.ts +++ b/src/normalizeUnit.ts @@ -8,8 +8,8 @@ export function normalizeUnit(prevUnit: string, currentUnit: string, value: numb return value * 1000 ** unitDiff; } - const prevUnitIndex2 = OPS_PER_TIME_UNIT.indexOf(prevUnit); - const currentUnitIndex2 = OPS_PER_TIME_UNIT.indexOf(currentUnit); + const prevUnitIndex2 = ITER_UNITS.indexOf(prevUnit); + const currentUnitIndex2 = ITER_UNITS.indexOf(currentUnit); if (prevUnitIndex2 >= 0 && currentUnitIndex2 >= 0) { const unitDiff = prevUnitIndex2 - currentUnitIndex2; @@ -17,8 +17,18 @@ export function normalizeUnit(prevUnit: string, currentUnit: string, value: numb return value * 1000 ** unitDiff; } + const prevUnitIndex3 = OPS_PER_TIME_UNIT.indexOf(prevUnit); + const currentUnitIndex3 = OPS_PER_TIME_UNIT.indexOf(currentUnit); + + if (prevUnitIndex3 >= 0 && currentUnitIndex3 >= 0) { + const unitDiff = prevUnitIndex3 - currentUnitIndex3; + + return value * 1000 ** unitDiff; + } + return value; } const TIME_UNITS = ['s', 'ms', 'us', 'ns']; +const ITER_UNITS = TIME_UNITS.map((unit) => `${unit}/iter`); const OPS_PER_TIME_UNIT = [...TIME_UNITS].reverse().map((unit) => `ops/${unit}`); diff --git a/test/normalizeBenchmarkResult.spec.ts b/test/normalizeBenchmarkResult.spec.ts index 646df4138..e69ece8bd 100644 --- a/test/normalizeBenchmarkResult.spec.ts +++ b/test/normalizeBenchmarkResult.spec.ts @@ -21,6 +21,25 @@ describe('normalizeBenchmarkResult', () => { { name: 'Bench', value: 1.01, unit: 's', range: '± 0.1' }, ), ).toEqual({ name: 'Bench', value: 1_010_000_000, unit: 'ns', range: '± 100000000' }); + + expect( + normalizeBenchmarkResult( + { name: 'Bench', value: 900, unit: 'ms/iter', range: '± 20' }, + { name: 'Bench', value: 1.01, unit: 's/iter', range: '± 0.1' }, + ), + ).toEqual({ name: 'Bench', value: 1_010, unit: 'ms/iter', range: '± 100' }); + expect( + normalizeBenchmarkResult( + { name: 'Bench', value: 900, unit: 'us/iter', range: '± 20' }, + { name: 'Bench', value: 1.01, unit: 's/iter', range: '± 0.1' }, + ), + ).toEqual({ name: 'Bench', value: 1_010_000, unit: 'us/iter', range: '± 100000' }); + expect( + normalizeBenchmarkResult( + { name: 'Bench', value: 900, unit: 'ns/iter', range: '± 20' }, + { name: 'Bench', value: 1.01, unit: 's/iter', range: '± 0.1' }, + ), + ).toEqual({ name: 'Bench', value: 1_010_000_000, unit: 'ns/iter', range: '± 100000000' }); }); it('normalize smaller when new unit is smaller', () => { @@ -36,6 +55,19 @@ describe('normalizeBenchmarkResult', () => { { name: 'Bench', value: 9_000_000, unit: 'us', range: '± 2000000' }, ), ).toEqual({ name: 'Bench', value: 9, unit: 's', range: '± 2' }); + + expect( + normalizeBenchmarkResult( + { name: 'Bench', value: 1.01, unit: 's/iter', range: '± 0.1' }, + { name: 'Bench', value: 900, unit: 'ms/iter', range: '± 20' }, + ), + ).toEqual({ name: 'Bench', value: 0.9, unit: 's/iter', range: '± 0.02' }); + expect( + normalizeBenchmarkResult( + { name: 'Bench', value: 10_000.01, unit: 's/iter', range: '± 10' }, + { name: 'Bench', value: 9_000_000, unit: 'us/iter', range: '± 2000000' }, + ), + ).toEqual({ name: 'Bench', value: 9, unit: 's/iter', range: '± 2' }); }); it('NOT normalize when new unit is not supported', () => { diff --git a/test/normalizeUnit.spec.ts b/test/normalizeUnit.spec.ts index 09fa20456..252bd7c0b 100644 --- a/test/normalizeUnit.spec.ts +++ b/test/normalizeUnit.spec.ts @@ -13,6 +13,18 @@ describe('normalizeUnit', () => { expect(normalizeUnit('s', 'ns', 12_000_000_000)).toBe(12); }); + it('normalize smaller when new unit is larger - iter units', () => { + expect(normalizeUnit('ms/iter', 's/iter', 12)).toBe(12_000); + expect(normalizeUnit('us/iter', 's/iter', 12)).toBe(12_000_000); + expect(normalizeUnit('ns/iter', 's/iter', 12)).toBe(12_000_000_000); + }); + + it('normalize smaller when new unit is smaller - iter units', () => { + expect(normalizeUnit('s/iter', 'ms/iter', 12)).toBe(0.012); + expect(normalizeUnit('s/iter', 'us/iter', 12)).toBe(0.000_012); + expect(normalizeUnit('s/iter', 'ns/iter', 12_000_000_000)).toBe(12); + }); + it('normalize smaller when new unit is smaller - ops per time units', () => { expect(normalizeUnit('ops/ms', 'ops/s', 12)).toBe(0.012); expect(normalizeUnit('ops/us', 'ops/s', 12)).toBe(0.000_012); diff --git a/test/write.spec.ts b/test/write.spec.ts index 40f4b4441..a896dbc97 100644 --- a/test/write.spec.ts +++ b/test/write.spec.ts @@ -351,7 +351,7 @@ describe.each(['https://github.com', 'https://github.enterprise.corp'])('writeBe commit: commit('prev commit id'), date: lastUpdate - 1000, tool: 'pytest', - benches: [bench('bench_fib_10', 100)], + benches: [bench('bench_fib_10', 100), bench('bench_fib_20', 900)], }, ], 'Other benchmark': [ @@ -368,7 +368,13 @@ describe.each(['https://github.com', 'https://github.enterprise.corp'])('writeBe commit: commit('current commit id'), date: lastUpdate, tool: 'pytest', - benches: [bench('bench_fib_10', 135)], + benches: [bench('bench_fib_10', 135), bench('bench_fib_20', 1.1, '± 0.02', 'us/iter')], + }, + expectedAdded: { + commit: commit('current commit id'), + date: lastUpdate, + tool: 'pytest', + benches: [bench('bench_fib_10', 135), bench('bench_fib_20', 1100, '± 20')], }, }, { @@ -410,6 +416,54 @@ describe.each(['https://github.com', 'https://github.enterprise.corp'])('writeBe 'CC: @user', ], }, + { + it: 'raises an alert when exceeding threshold 2.0 - different units', + config: defaultCfg, + data: { + lastUpdate, + repoUrl, + entries: { + 'Test benchmark': [ + { + commit: commit('prev commit id'), + date: lastUpdate - 1000, + tool: 'go', + benches: [bench('bench_fib_10', 100), bench('bench_fib_20', 900)], + }, + ], + }, + }, + added: { + commit: commit('current commit id'), + date: lastUpdate, + tool: 'go', + benches: [ + bench('bench_fib_10', 0.21, '± 0.02', 'us/iter'), + bench('bench_fib_20', 2.25, '± 0.02', 'us/iter'), + ], // Exceeds 2.0 threshold + }, + expectedAdded: { + commit: commit('current commit id'), + date: lastUpdate, + tool: 'go', + benches: [bench('bench_fib_10', 210), bench('bench_fib_20', 2250)], // Exceeds 2.0 threshold + }, + error: [ + '# :warning: **Performance Alert** :warning:', + '', + "Possible performance regression was detected for benchmark **'Test benchmark'**.", + 'Benchmark result of this commit is worse than the previous benchmark result exceeding threshold `2`.', + '', + '| Benchmark suite | Current: current commit id | Previous: prev commit id | Ratio |', + '|-|-|-|-|', + '| `bench_fib_10` | `210` ns/iter (`± 20`) | `100` ns/iter (`± 20`) | `2.10` |', + '| `bench_fib_20` | `2250` ns/iter (`± 20`) | `900` ns/iter (`± 20`) | `2.50` |', + '', + `This comment was automatically generated by [workflow](${serverUrl}/user/repo/actions?query=workflow%3AWorkflow%20name) using [github-action-benchmark](https://github.com/marketplace/actions/continuous-benchmark).`, + '', + 'CC: @user', + ], + }, { it: 'raises an alert with tool whose result value is bigger-is-better', config: defaultCfg, From 96984a70bc411d2f94769e84d7b28722a4e7286f Mon Sep 17 00:00:00 2001 From: Chris Trzesniewski Date: Sat, 30 Aug 2025 17:26:58 +0200 Subject: [PATCH 05/11] simplify normalizeUnit function --- src/normalizeUnit.ts | 31 ++++++++----------------------- 1 file changed, 8 insertions(+), 23 deletions(-) diff --git a/src/normalizeUnit.ts b/src/normalizeUnit.ts index f16df1bf7..bc0a98a49 100644 --- a/src/normalizeUnit.ts +++ b/src/normalizeUnit.ts @@ -1,29 +1,13 @@ export function normalizeUnit(prevUnit: string, currentUnit: string, value: number): number { - const prevUnitIndex = TIME_UNITS.indexOf(prevUnit); - const currentUnitIndex = TIME_UNITS.indexOf(currentUnit); + for (const units of SUPPORTED_UNITS) { + const prevUnitIndex = units.indexOf(prevUnit); + const currentUnitIndex = units.indexOf(currentUnit); - if (prevUnitIndex >= 0 && currentUnitIndex >= 0) { - const unitDiff = prevUnitIndex - currentUnitIndex; + if (prevUnitIndex >= 0 && currentUnitIndex >= 0) { + const unitDiff = prevUnitIndex - currentUnitIndex; - return value * 1000 ** unitDiff; - } - - const prevUnitIndex2 = ITER_UNITS.indexOf(prevUnit); - const currentUnitIndex2 = ITER_UNITS.indexOf(currentUnit); - - if (prevUnitIndex2 >= 0 && currentUnitIndex2 >= 0) { - const unitDiff = prevUnitIndex2 - currentUnitIndex2; - - return value * 1000 ** unitDiff; - } - - const prevUnitIndex3 = OPS_PER_TIME_UNIT.indexOf(prevUnit); - const currentUnitIndex3 = OPS_PER_TIME_UNIT.indexOf(currentUnit); - - if (prevUnitIndex3 >= 0 && currentUnitIndex3 >= 0) { - const unitDiff = prevUnitIndex3 - currentUnitIndex3; - - return value * 1000 ** unitDiff; + return value * 1000 ** unitDiff; + } } return value; @@ -32,3 +16,4 @@ export function normalizeUnit(prevUnit: string, currentUnit: string, value: numb const TIME_UNITS = ['s', 'ms', 'us', 'ns']; const ITER_UNITS = TIME_UNITS.map((unit) => `${unit}/iter`); const OPS_PER_TIME_UNIT = [...TIME_UNITS].reverse().map((unit) => `ops/${unit}`); +const SUPPORTED_UNITS = [TIME_UNITS, ITER_UNITS, OPS_PER_TIME_UNIT]; From 9bb96a66259d5c25626e09d25208227f5ad8955c Mon Sep 17 00:00:00 2001 From: Chris Trzesniewski Date: Sat, 30 Aug 2025 17:59:48 +0200 Subject: [PATCH 06/11] minor code improvements --- src/extractRangeInfo.ts | 6 +++--- src/normalizeUnit.ts | 3 ++- test/extractRangeInfo.spec.ts | 2 ++ 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/extractRangeInfo.ts b/src/extractRangeInfo.ts index 01738e6f0..6f85315fe 100644 --- a/src/extractRangeInfo.ts +++ b/src/extractRangeInfo.ts @@ -5,11 +5,11 @@ export function extractRangeInfo(range: string | undefined): { prefix: string; v const matches = range.match(/(?(\+-|±)\s*)(?\d.+)/); - if (!matches) { + if (!matches || !matches.groups) { return undefined; } - const valueString = matches.groups?.['value']; + const valueString = matches.groups.value; const value = Number(valueString); @@ -19,6 +19,6 @@ export function extractRangeInfo(range: string | undefined): { prefix: string; v return { value, - prefix: matches.groups?.prefix ?? '', + prefix: matches.groups.prefix ?? '', }; } diff --git a/src/normalizeUnit.ts b/src/normalizeUnit.ts index bc0a98a49..b294e9f7e 100644 --- a/src/normalizeUnit.ts +++ b/src/normalizeUnit.ts @@ -6,13 +6,14 @@ export function normalizeUnit(prevUnit: string, currentUnit: string, value: numb if (prevUnitIndex >= 0 && currentUnitIndex >= 0) { const unitDiff = prevUnitIndex - currentUnitIndex; - return value * 1000 ** unitDiff; + return value * UNIT_CONVERSION_MULTIPLIER ** unitDiff; } } return value; } +const UNIT_CONVERSION_MULTIPLIER = 1000; const TIME_UNITS = ['s', 'ms', 'us', 'ns']; const ITER_UNITS = TIME_UNITS.map((unit) => `${unit}/iter`); const OPS_PER_TIME_UNIT = [...TIME_UNITS].reverse().map((unit) => `ops/${unit}`); diff --git a/test/extractRangeInfo.spec.ts b/test/extractRangeInfo.spec.ts index 1253fadff..32524ac29 100644 --- a/test/extractRangeInfo.spec.ts +++ b/test/extractRangeInfo.spec.ts @@ -24,5 +24,7 @@ describe('extractRangeInfo', () => { it('should NOT extract range with invalid number', () => { expect(extractRangeInfo('± boo')).toBeUndefined(); expect(extractRangeInfo('+- boo')).toBeUndefined(); + expect(extractRangeInfo('± 1boo')).toBeUndefined(); + expect(extractRangeInfo('+- 1boo')).toBeUndefined(); }); }); From ba0e547335d75e726d75f1f963134e2ea95f8cd6 Mon Sep 17 00:00:00 2001 From: Chris Trzesniewski Date: Sat, 30 Aug 2025 18:49:47 +0200 Subject: [PATCH 07/11] add one more test case --- test/write.spec.ts | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/test/write.spec.ts b/test/write.spec.ts index a896dbc97..ec02b380b 100644 --- a/test/write.spec.ts +++ b/test/write.spec.ts @@ -1289,6 +1289,30 @@ describe.each(['https://github.com', 'https://github.enterprise.corp'])('writeBe `This comment was automatically generated by [workflow](${serverUrl}/user/repo/actions?query=workflow%3AWorkflow%20name) using [github-action-benchmark](https://github.com/marketplace/actions/continuous-benchmark).`, ], }, + { + it: 'fails when exceeding the threshold - different units', + config: defaultCfg, + added: { + commit: commit('current commit id'), + date: lastUpdate, + tool: 'cargo', + benches: [bench('bench_fib_10', 0.21, '± 0.02', 'us/iter')], // Exceeds 2.0 threshold + }, + gitServerUrl: serverUrl, + gitHistory: gitHistory(), + error: [ + '# :warning: **Performance Alert** :warning:', + '', + "Possible performance regression was detected for benchmark **'Test benchmark'**.", + 'Benchmark result of this commit is worse than the previous benchmark result exceeding threshold `2`.', + '', + '| Benchmark suite | Current: current commit id | Previous: prev commit id | Ratio |', + '|-|-|-|-|', + '| `bench_fib_10` | `210` ns/iter (`± 20`) | `100` ns/iter (`± 20`) | `2.10` |', + '', + `This comment was automatically generated by [workflow](${serverUrl}/user/repo/actions?query=workflow%3AWorkflow%20name) using [github-action-benchmark](https://github.com/marketplace/actions/continuous-benchmark).`, + ], + }, { it: 'sends commit message but does not raise an error when exceeding alert threshold but not exceeding failure threshold', config: { From 004ab160ae4152aef2de404b04e5f13748d7bc95 Mon Sep 17 00:00:00 2001 From: Chris Trzesniewski Date: Tue, 2 Sep 2025 22:31:51 +0200 Subject: [PATCH 08/11] apply suggestions * use spread operator consistently for cloning the array * rename normalizeUnit to normalizeValueByUnit * removed Wallaby debugging statements --- src/addBenchmarkEntry.ts | 4 +- src/normalizeBenchmarkResult.ts | 9 ++-- ...rmalizeUnit.ts => normalizeValueByUnit.ts} | 2 +- test/normalizeUnit.spec.ts | 43 ------------------- test/normalizeValueByUnit.spec.ts | 43 +++++++++++++++++++ test/write.spec.ts | 2 - 6 files changed, 52 insertions(+), 51 deletions(-) rename src/{normalizeUnit.ts => normalizeValueByUnit.ts} (87%) delete mode 100644 test/normalizeUnit.spec.ts create mode 100644 test/normalizeValueByUnit.spec.ts diff --git a/src/addBenchmarkEntry.ts b/src/addBenchmarkEntry.ts index 3c782f1f3..da2996d72 100644 --- a/src/addBenchmarkEntry.ts +++ b/src/addBenchmarkEntry.ts @@ -18,8 +18,8 @@ export function addBenchmarkEntry( core.debug(`No suite was found for benchmark '${benchName}' in existing data. Created`); } else { const suites = entries[benchName]; - // Get last suite which has different commit ID for alert comment - for (const e of suites.slice().reverse()) { + // Get the last suite which has different commit ID for alert comment + for (const e of [...suites].reverse()) { if (e.commit.id !== benchEntry.commit.id) { prevBench = e; break; diff --git a/src/normalizeBenchmarkResult.ts b/src/normalizeBenchmarkResult.ts index d18caac55..fe5e6fe36 100644 --- a/src/normalizeBenchmarkResult.ts +++ b/src/normalizeBenchmarkResult.ts @@ -1,5 +1,5 @@ import { BenchmarkResult } from './extract'; -import { normalizeUnit } from './normalizeUnit'; +import { normalizeValueByUnit } from './normalizeValueByUnit'; import { extractRangeInfo } from './extractRangeInfo'; export function normalizeBenchmarkResult( @@ -15,10 +15,13 @@ export function normalizeBenchmarkResult( const currentRange = currenBenchResult.range; const currentRangeInfo = extractRangeInfo(currentRange); - const normalizedValue = normalizeUnit(prevUnit, currentUnit, currenBenchResult.value); + const normalizedValue = normalizeValueByUnit(prevUnit, currentUnit, currenBenchResult.value); const normalizedUnit = currenBenchResult.value !== normalizedValue ? prevUnit : currentUnit; const normalizedRangeInfo = currentRangeInfo - ? { prefix: currentRangeInfo.prefix, value: normalizeUnit(prevUnit, currentUnit, currentRangeInfo.value) } + ? { + prefix: currentRangeInfo.prefix, + value: normalizeValueByUnit(prevUnit, currentUnit, currentRangeInfo.value), + } : undefined; return { diff --git a/src/normalizeUnit.ts b/src/normalizeValueByUnit.ts similarity index 87% rename from src/normalizeUnit.ts rename to src/normalizeValueByUnit.ts index b294e9f7e..26ba0bf79 100644 --- a/src/normalizeUnit.ts +++ b/src/normalizeValueByUnit.ts @@ -1,4 +1,4 @@ -export function normalizeUnit(prevUnit: string, currentUnit: string, value: number): number { +export function normalizeValueByUnit(prevUnit: string, currentUnit: string, value: number): number { for (const units of SUPPORTED_UNITS) { const prevUnitIndex = units.indexOf(prevUnit); const currentUnitIndex = units.indexOf(currentUnit); diff --git a/test/normalizeUnit.spec.ts b/test/normalizeUnit.spec.ts deleted file mode 100644 index 252bd7c0b..000000000 --- a/test/normalizeUnit.spec.ts +++ /dev/null @@ -1,43 +0,0 @@ -import { normalizeUnit } from '../src/normalizeUnit'; - -describe('normalizeUnit', () => { - it('normalize smaller when new unit is larger - time units', () => { - expect(normalizeUnit('ms', 's', 12)).toBe(12_000); - expect(normalizeUnit('us', 's', 12)).toBe(12_000_000); - expect(normalizeUnit('ns', 's', 12)).toBe(12_000_000_000); - }); - - it('normalize smaller when new unit is smaller - time units', () => { - expect(normalizeUnit('s', 'ms', 12)).toBe(0.012); - expect(normalizeUnit('s', 'us', 12)).toBe(0.000_012); - expect(normalizeUnit('s', 'ns', 12_000_000_000)).toBe(12); - }); - - it('normalize smaller when new unit is larger - iter units', () => { - expect(normalizeUnit('ms/iter', 's/iter', 12)).toBe(12_000); - expect(normalizeUnit('us/iter', 's/iter', 12)).toBe(12_000_000); - expect(normalizeUnit('ns/iter', 's/iter', 12)).toBe(12_000_000_000); - }); - - it('normalize smaller when new unit is smaller - iter units', () => { - expect(normalizeUnit('s/iter', 'ms/iter', 12)).toBe(0.012); - expect(normalizeUnit('s/iter', 'us/iter', 12)).toBe(0.000_012); - expect(normalizeUnit('s/iter', 'ns/iter', 12_000_000_000)).toBe(12); - }); - - it('normalize smaller when new unit is smaller - ops per time units', () => { - expect(normalizeUnit('ops/ms', 'ops/s', 12)).toBe(0.012); - expect(normalizeUnit('ops/us', 'ops/s', 12)).toBe(0.000_012); - expect(normalizeUnit('ops/ns', 'ops/s', 12_000_000_000)).toBe(12); - }); - - it('normalize smaller when new unit is larger - ops per time units', () => { - expect(normalizeUnit('ops/s', 'ops/ms', 12)).toBe(12_000); - expect(normalizeUnit('ops/s', 'ops/us', 12)).toBe(12_000_000); - expect(normalizeUnit('ops/s', 'ops/ns', 12)).toBe(12_000_000_000); - }); - - it('NOT normalize when new unit is not supported', () => { - expect(normalizeUnit('unknown1', 'unknown2', 12)).toBe(12); - }); -}); diff --git a/test/normalizeValueByUnit.spec.ts b/test/normalizeValueByUnit.spec.ts new file mode 100644 index 000000000..7f613287c --- /dev/null +++ b/test/normalizeValueByUnit.spec.ts @@ -0,0 +1,43 @@ +import { normalizeValueByUnit } from '../src/normalizeValueByUnit'; + +describe('normalizeValueByUnit', () => { + it('normalize smaller when new unit is larger - time units', () => { + expect(normalizeValueByUnit('ms', 's', 12)).toBe(12_000); + expect(normalizeValueByUnit('us', 's', 12)).toBe(12_000_000); + expect(normalizeValueByUnit('ns', 's', 12)).toBe(12_000_000_000); + }); + + it('normalize smaller when new unit is smaller - time units', () => { + expect(normalizeValueByUnit('s', 'ms', 12)).toBe(0.012); + expect(normalizeValueByUnit('s', 'us', 12)).toBe(0.000_012); + expect(normalizeValueByUnit('s', 'ns', 12_000_000_000)).toBe(12); + }); + + it('normalize smaller when new unit is larger - iter units', () => { + expect(normalizeValueByUnit('ms/iter', 's/iter', 12)).toBe(12_000); + expect(normalizeValueByUnit('us/iter', 's/iter', 12)).toBe(12_000_000); + expect(normalizeValueByUnit('ns/iter', 's/iter', 12)).toBe(12_000_000_000); + }); + + it('normalize smaller when new unit is smaller - iter units', () => { + expect(normalizeValueByUnit('s/iter', 'ms/iter', 12)).toBe(0.012); + expect(normalizeValueByUnit('s/iter', 'us/iter', 12)).toBe(0.000_012); + expect(normalizeValueByUnit('s/iter', 'ns/iter', 12_000_000_000)).toBe(12); + }); + + it('normalize smaller when new unit is smaller - ops per time units', () => { + expect(normalizeValueByUnit('ops/ms', 'ops/s', 12)).toBe(0.012); + expect(normalizeValueByUnit('ops/us', 'ops/s', 12)).toBe(0.000_012); + expect(normalizeValueByUnit('ops/ns', 'ops/s', 12_000_000_000)).toBe(12); + }); + + it('normalize smaller when new unit is larger - ops per time units', () => { + expect(normalizeValueByUnit('ops/s', 'ops/ms', 12)).toBe(12_000); + expect(normalizeValueByUnit('ops/s', 'ops/us', 12)).toBe(12_000_000); + expect(normalizeValueByUnit('ops/s', 'ops/ns', 12)).toBe(12_000_000_000); + }); + + it('NOT normalize when new unit is not supported', () => { + expect(normalizeValueByUnit('unknown1', 'unknown2', 12)).toBe(12); + }); +}); diff --git a/test/write.spec.ts b/test/write.spec.ts index ec02b380b..63611146a 100644 --- a/test/write.spec.ts +++ b/test/write.spec.ts @@ -828,8 +828,6 @@ describe.each(['https://github.com', 'https://github.enterprise.corp'])('writeBe it.each(normalCases)('$it', async function (t) { const { data, added, config, repoPayload, error, commitComment } = t; const expectedAdded = t.expectedAdded ?? added; - added.benches; // ? - expectedAdded.benches; // ? gitHubContext.payload.repository = { private: false, From d973feab05781230d51dc0fd61386091ee4c884d Mon Sep 17 00:00:00 2001 From: Chris Trzesniewski Date: Tue, 2 Sep 2025 22:55:53 +0200 Subject: [PATCH 09/11] apply suggestions * fix handling of single digit range --- src/extractRangeInfo.ts | 2 +- src/normalizeBenchmarkResult.ts | 14 +++++++------- test/extractRangeInfo.spec.ts | 13 +++++++++++++ 3 files changed, 21 insertions(+), 8 deletions(-) diff --git a/src/extractRangeInfo.ts b/src/extractRangeInfo.ts index 6f85315fe..e225b7fef 100644 --- a/src/extractRangeInfo.ts +++ b/src/extractRangeInfo.ts @@ -3,7 +3,7 @@ export function extractRangeInfo(range: string | undefined): { prefix: string; v return undefined; } - const matches = range.match(/(?(\+-|±)\s*)(?\d.+)/); + const matches = range.match(/(?(\+-|±)\s*)(?\d.*)/); if (!matches || !matches.groups) { return undefined; diff --git a/src/normalizeBenchmarkResult.ts b/src/normalizeBenchmarkResult.ts index fe5e6fe36..ace354c9c 100644 --- a/src/normalizeBenchmarkResult.ts +++ b/src/normalizeBenchmarkResult.ts @@ -4,19 +4,19 @@ import { extractRangeInfo } from './extractRangeInfo'; export function normalizeBenchmarkResult( prevBenchResult: BenchmarkResult | undefined | null, - currenBenchResult: BenchmarkResult, + currentBenchResult: BenchmarkResult, ): BenchmarkResult { if (!prevBenchResult) { - return currenBenchResult; + return currentBenchResult; } const prevUnit = prevBenchResult.unit; - const currentUnit = currenBenchResult.unit; - const currentRange = currenBenchResult.range; + const currentUnit = currentBenchResult.unit; + const currentRange = currentBenchResult.range; const currentRangeInfo = extractRangeInfo(currentRange); - const normalizedValue = normalizeValueByUnit(prevUnit, currentUnit, currenBenchResult.value); - const normalizedUnit = currenBenchResult.value !== normalizedValue ? prevUnit : currentUnit; + const normalizedValue = normalizeValueByUnit(prevUnit, currentUnit, currentBenchResult.value); + const normalizedUnit = currentBenchResult.value !== normalizedValue ? prevUnit : currentUnit; const normalizedRangeInfo = currentRangeInfo ? { prefix: currentRangeInfo.prefix, @@ -25,7 +25,7 @@ export function normalizeBenchmarkResult( : undefined; return { - ...currenBenchResult, + ...currentBenchResult, value: normalizedValue, unit: normalizedUnit, range: normalizedRangeInfo ? `${normalizedRangeInfo.prefix}${normalizedRangeInfo.value}` : currentRange, diff --git a/test/extractRangeInfo.spec.ts b/test/extractRangeInfo.spec.ts index 32524ac29..41c00fdab 100644 --- a/test/extractRangeInfo.spec.ts +++ b/test/extractRangeInfo.spec.ts @@ -17,6 +17,19 @@ describe('extractRangeInfo', () => { expect(extractRangeInfo('+- 20')).toEqual({ value: 20, prefix: '+- ' }); }); + it('should extract single-digit integer', () => { + expect(extractRangeInfo('±2')).toEqual({ value: 2, prefix: '±' }); + }); + it('should extract decimal and preserve prefix space', () => { + expect(extractRangeInfo('± 0.5')).toEqual({ value: 0.5, prefix: '± ' }); + }); + it('should extract scientific notation', () => { + expect(extractRangeInfo('+-1e-3')).toEqual({ value: 1e-3, prefix: '+-' }); + }); + it('should tolerate surrounding whitespace', () => { + expect(extractRangeInfo(' ±20 ')).toEqual({ value: 20, prefix: '±' }); + }); + it('should NOT extract range with unknown prefix', () => { expect(extractRangeInfo('unknown prefix 20')).toBeUndefined(); }); From 22f329e8655d697da72b1db8b6c676dd2fc54e31 Mon Sep 17 00:00:00 2001 From: Chris Trzesniewski Date: Tue, 2 Sep 2025 23:06:45 +0200 Subject: [PATCH 10/11] canonicalize units --- src/canonicalizeUnit.ts | 3 +++ src/normalizeBenchmarkResult.ts | 14 +++++++------- src/normalizeValueByUnit.ts | 8 ++++++-- test/normalizeValueByUnit.spec.ts | 12 ++++++++++++ 4 files changed, 28 insertions(+), 9 deletions(-) create mode 100644 src/canonicalizeUnit.ts diff --git a/src/canonicalizeUnit.ts b/src/canonicalizeUnit.ts new file mode 100644 index 000000000..2fc0a210b --- /dev/null +++ b/src/canonicalizeUnit.ts @@ -0,0 +1,3 @@ +export function canonicalizeUnit(u: string): string { + return u.trim().toLowerCase().replace(/µs|μs/g, 'us'); +} diff --git a/src/normalizeBenchmarkResult.ts b/src/normalizeBenchmarkResult.ts index ace354c9c..fe5e6fe36 100644 --- a/src/normalizeBenchmarkResult.ts +++ b/src/normalizeBenchmarkResult.ts @@ -4,19 +4,19 @@ import { extractRangeInfo } from './extractRangeInfo'; export function normalizeBenchmarkResult( prevBenchResult: BenchmarkResult | undefined | null, - currentBenchResult: BenchmarkResult, + currenBenchResult: BenchmarkResult, ): BenchmarkResult { if (!prevBenchResult) { - return currentBenchResult; + return currenBenchResult; } const prevUnit = prevBenchResult.unit; - const currentUnit = currentBenchResult.unit; - const currentRange = currentBenchResult.range; + const currentUnit = currenBenchResult.unit; + const currentRange = currenBenchResult.range; const currentRangeInfo = extractRangeInfo(currentRange); - const normalizedValue = normalizeValueByUnit(prevUnit, currentUnit, currentBenchResult.value); - const normalizedUnit = currentBenchResult.value !== normalizedValue ? prevUnit : currentUnit; + const normalizedValue = normalizeValueByUnit(prevUnit, currentUnit, currenBenchResult.value); + const normalizedUnit = currenBenchResult.value !== normalizedValue ? prevUnit : currentUnit; const normalizedRangeInfo = currentRangeInfo ? { prefix: currentRangeInfo.prefix, @@ -25,7 +25,7 @@ export function normalizeBenchmarkResult( : undefined; return { - ...currentBenchResult, + ...currenBenchResult, value: normalizedValue, unit: normalizedUnit, range: normalizedRangeInfo ? `${normalizedRangeInfo.prefix}${normalizedRangeInfo.value}` : currentRange, diff --git a/src/normalizeValueByUnit.ts b/src/normalizeValueByUnit.ts index 26ba0bf79..3b47463c1 100644 --- a/src/normalizeValueByUnit.ts +++ b/src/normalizeValueByUnit.ts @@ -1,7 +1,11 @@ +import { canonicalizeUnit } from './canonicalizeUnit'; + export function normalizeValueByUnit(prevUnit: string, currentUnit: string, value: number): number { for (const units of SUPPORTED_UNITS) { - const prevUnitIndex = units.indexOf(prevUnit); - const currentUnitIndex = units.indexOf(currentUnit); + const prev = canonicalizeUnit(prevUnit); + const current = canonicalizeUnit(currentUnit); + const prevUnitIndex = units.indexOf(prev); + const currentUnitIndex = units.indexOf(current); if (prevUnitIndex >= 0 && currentUnitIndex >= 0) { const unitDiff = prevUnitIndex - currentUnitIndex; diff --git a/test/normalizeValueByUnit.spec.ts b/test/normalizeValueByUnit.spec.ts index 7f613287c..694077e18 100644 --- a/test/normalizeValueByUnit.spec.ts +++ b/test/normalizeValueByUnit.spec.ts @@ -37,6 +37,18 @@ describe('normalizeValueByUnit', () => { expect(normalizeValueByUnit('ops/s', 'ops/ns', 12)).toBe(12_000_000_000); }); + it('handles microsecond symbol variants', () => { + expect(normalizeValueByUnit('ms', 'µs', 12)).toBe(0.012); + expect(normalizeValueByUnit('ms', 'μs', 12)).toBe(0.012); + expect(normalizeValueByUnit('us', 'µs', 12)).toBe(12); + expect(normalizeValueByUnit('us', 'μs', 12)).toBe(12); + expect(normalizeValueByUnit('µs', 'μs', 12)).toBe(12); + expect(normalizeValueByUnit('ops/µs', 'ops/s', 12_000_000)).toBe(12); + }); + it('tolerates surrounding whitespace and case', () => { + expect(normalizeValueByUnit(' S ', ' MS ', 12)).toBe(0.012); + }); + it('NOT normalize when new unit is not supported', () => { expect(normalizeValueByUnit('unknown1', 'unknown2', 12)).toBe(12); }); From 32b286938e19a77e4abfcb70b3ff959875a50fcc Mon Sep 17 00:00:00 2001 From: Chris Trzesniewski Date: Tue, 2 Sep 2025 23:18:31 +0200 Subject: [PATCH 11/11] cleanup --- src/normalizeBenchmarkResult.ts | 14 +++++++------- src/normalizeValueByUnit.ts | 4 ++-- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/normalizeBenchmarkResult.ts b/src/normalizeBenchmarkResult.ts index fe5e6fe36..ace354c9c 100644 --- a/src/normalizeBenchmarkResult.ts +++ b/src/normalizeBenchmarkResult.ts @@ -4,19 +4,19 @@ import { extractRangeInfo } from './extractRangeInfo'; export function normalizeBenchmarkResult( prevBenchResult: BenchmarkResult | undefined | null, - currenBenchResult: BenchmarkResult, + currentBenchResult: BenchmarkResult, ): BenchmarkResult { if (!prevBenchResult) { - return currenBenchResult; + return currentBenchResult; } const prevUnit = prevBenchResult.unit; - const currentUnit = currenBenchResult.unit; - const currentRange = currenBenchResult.range; + const currentUnit = currentBenchResult.unit; + const currentRange = currentBenchResult.range; const currentRangeInfo = extractRangeInfo(currentRange); - const normalizedValue = normalizeValueByUnit(prevUnit, currentUnit, currenBenchResult.value); - const normalizedUnit = currenBenchResult.value !== normalizedValue ? prevUnit : currentUnit; + const normalizedValue = normalizeValueByUnit(prevUnit, currentUnit, currentBenchResult.value); + const normalizedUnit = currentBenchResult.value !== normalizedValue ? prevUnit : currentUnit; const normalizedRangeInfo = currentRangeInfo ? { prefix: currentRangeInfo.prefix, @@ -25,7 +25,7 @@ export function normalizeBenchmarkResult( : undefined; return { - ...currenBenchResult, + ...currentBenchResult, value: normalizedValue, unit: normalizedUnit, range: normalizedRangeInfo ? `${normalizedRangeInfo.prefix}${normalizedRangeInfo.value}` : currentRange, diff --git a/src/normalizeValueByUnit.ts b/src/normalizeValueByUnit.ts index 3b47463c1..2b90285f8 100644 --- a/src/normalizeValueByUnit.ts +++ b/src/normalizeValueByUnit.ts @@ -1,9 +1,9 @@ import { canonicalizeUnit } from './canonicalizeUnit'; export function normalizeValueByUnit(prevUnit: string, currentUnit: string, value: number): number { + const prev = canonicalizeUnit(prevUnit); + const current = canonicalizeUnit(currentUnit); for (const units of SUPPORTED_UNITS) { - const prev = canonicalizeUnit(prevUnit); - const current = canonicalizeUnit(currentUnit); const prevUnitIndex = units.indexOf(prev); const currentUnitIndex = units.indexOf(current);