From e108afd01ab131e06b44c1a5ae340e4dc02163e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Chris=20Trze=C5=9Bniewski?= Date: Wed, 31 Jan 2024 15:01:02 +0100 Subject: [PATCH] fix: 204 ratio is NaN when previous value is 0 (#222) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * extract getRatio function * print 1 when both values are 0 * print +-∞ when divisor is 0 --- CHANGELOG.md | 2 + package-lock.json | 1 + package.json | 1 + src/write.ts | 27 +++-- test/buildComment.test.ts | 209 ++++++++++++++++++++++++++++++++++++++ test/fakedOctokit.ts | 34 +++++++ test/write.spec.ts | 36 +------ 7 files changed, 268 insertions(+), 42 deletions(-) create mode 100644 test/buildComment.test.ts create mode 100644 test/fakedOctokit.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 008290276..7646e7fe1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,7 @@ ## Unreleased +- **fix** ratio is NaN when previous value is 0. Now, print 1 when both values are 0 and `+-∞` when divisor is 0 (#222) + # [v1.19.2](https://github.com/benchmark-action/github-action-benchmark/releases/tag/v1.19.2) - 26 Jan 2024 - **fix** markdown rendering for summary is broken (#218) diff --git a/package-lock.json b/package-lock.json index ea1673b3b..b027b8187 100644 --- a/package-lock.json +++ b/package-lock.json @@ -27,6 +27,7 @@ "@typescript-eslint/parser": "^5.4.0", "acorn": "^7.1.1", "cheerio": "^1.0.0-rc.3", + "dedent": "^1.5.1", "deep-diff": "^1.0.2", "deep-equal": "^2.0.1", "eslint": "^8.2.0", diff --git a/package.json b/package.json index 2fd79f8ad..faa2b4aa4 100644 --- a/package.json +++ b/package.json @@ -49,6 +49,7 @@ "@typescript-eslint/parser": "^5.4.0", "acorn": "^7.1.1", "cheerio": "^1.0.0-rc.3", + "dedent": "^1.5.1", "deep-diff": "^1.0.2", "deep-equal": "^2.0.1", "eslint": "^8.2.0", diff --git a/src/write.ts b/src/write.ts index fdf248a66..3c71823a7 100644 --- a/src/write.ts +++ b/src/write.ts @@ -103,9 +103,7 @@ function findAlerts(curSuite: Benchmark, prevSuite: Benchmark, threshold: number continue; } - const ratio = biggerIsBetter(curSuite.tool) - ? prev.value / current.value // e.g. current=100, prev=200 - : current.value / prev.value; // e.g. current=200, prev=100 + const ratio = getRatio(curSuite.tool, prev, current); if (ratio > threshold) { core.warning( @@ -133,6 +131,10 @@ function getCurrentRepoMetadata() { } function floatStr(n: number) { + if (!Number.isFinite(n)) { + return `${n > 0 ? '+' : '-'}∞`; + } + if (Number.isInteger(n)) { return n.toFixed(0); } @@ -160,7 +162,12 @@ function commentFooter(): string { return `This comment was automatically generated by [workflow](${actionUrl}) using [github-action-benchmark](https://github.com/marketplace/actions/continuous-benchmark).`; } -function buildComment(benchName: string, curSuite: Benchmark, prevSuite: Benchmark, expandableDetails = true): string { +export function buildComment( + benchName: string, + curSuite: Benchmark, + prevSuite: Benchmark, + expandableDetails = true, +): string { const lines = [ `# ${benchName}`, '', @@ -175,9 +182,7 @@ function buildComment(benchName: string, curSuite: Benchmark, prevSuite: Benchma const prev = prevSuite.benches.find((i) => i.name === current.name); if (prev) { - const ratio = biggerIsBetter(curSuite.tool) - ? prev.value / current.value // e.g. current=100, prev=200 - : current.value / prev.value; + const ratio = getRatio(curSuite.tool, prev, current); line = `| \`${current.name}\` | ${strVal(current)} | ${strVal(prev)} | \`${floatStr(ratio)}\` |`; } else { @@ -566,3 +571,11 @@ async function handleSummary(benchName: string, currBench: Benchmark, prevBench: await summary.write(); } + +function getRatio(tool: ToolType, prev: BenchmarkResult, current: BenchmarkResult) { + if (prev.value === 0 && current.value === 0) return 1; + + return biggerIsBetter(tool) + ? prev.value / current.value // e.g. current=100, prev=200 + : current.value / prev.value; // e.g. current=200, prev=100 +} diff --git a/test/buildComment.test.ts b/test/buildComment.test.ts new file mode 100644 index 000000000..ea9431c51 --- /dev/null +++ b/test/buildComment.test.ts @@ -0,0 +1,209 @@ +import { buildComment } from '../src/write'; +import { FakedOctokit, fakedRepos } from './fakedOctokit'; +import dedent from 'dedent'; + +interface RepositoryPayloadSubset { + private: boolean; + html_url: string; +} + +const gitHubContext = { + repo: { + repo: 'repo', + owner: 'user', + }, + payload: { + repository: { + private: false, + html_url: 'https://github.com/user/repo', + } as RepositoryPayloadSubset | null, + }, + workflow: 'Workflow name', +}; + +jest.mock('@actions/github', () => ({ + get context() { + return gitHubContext; + }, + getOctokit(token: string) { + return new FakedOctokit(token); + }, +})); + +afterEach(function () { + fakedRepos.clear(); +}); + +describe('buildComment', () => { + it('should build comment when previous benchmarks are 0 and biggerIsBetter returns false', () => { + const comment = buildComment( + 'TestSuite', + { + date: 12345, + commit: { + id: 'testCommitIdCurrent', + author: { + name: 'TestUser', + }, + committer: { + name: 'TestUser', + }, + message: 'Test current commit message', + url: 'https://test.previous.commit.url', + }, + tool: 'cargo', + benches: [ + { + value: 0, + name: 'TestBench<1>', + unit: 'testUnit', + }, + { + value: 1, + name: 'TestBench<2>', + unit: 'testUnit', + }, + { + value: -1, + name: 'TestBench<3>', + unit: 'testUnit', + }, + ], + }, + { + date: 12345, + commit: { + id: 'testCommitIdPrevious', + author: { + name: 'TestUser', + }, + committer: { + name: 'TestUser', + }, + message: 'Test previous commit message', + url: 'https://test.previous.commit.url', + }, + tool: 'cargo', + benches: [ + { + value: 0, + name: 'TestBench<1>', + unit: 'testUnit', + }, + { + value: 0, + name: 'TestBench<2>', + unit: 'testUnit', + }, + { + value: 0, + name: 'TestBench<3>', + unit: 'testUnit', + }, + ], + }, + ); + + expect(comment).toEqual(dedent` + # TestSuite + +
+ + | Benchmark suite | Current: testCommitIdCurrent | Previous: testCommitIdPrevious | Ratio | + |-|-|-|-| + | \`TestBench<1>\` | \`0\` testUnit | \`0\` testUnit | \`1\` | + | \`TestBench<2>\` | \`1\` testUnit | \`0\` testUnit | \`+∞\` | + | \`TestBench<3>\` | \`-1\` testUnit | \`0\` testUnit | \`-∞\` | + +
+ + This comment was automatically generated by [workflow](https://github.com/user/repo/actions?query=workflow%3AWorkflow%20name) using [github-action-benchmark](https://github.com/marketplace/actions/continuous-benchmark). + `); + }); + + it('should build comment when current benchmarks are 0 and biggerIsBetter returns true', () => { + const comment = buildComment( + 'TestSuite', + { + date: 12345, + commit: { + id: 'testCommitIdCurrent', + author: { + name: 'TestUser', + }, + committer: { + name: 'TestUser', + }, + message: 'Test current commit message', + url: 'https://test.previous.commit.url', + }, + tool: 'benchmarkjs', + benches: [ + { + value: 0, + name: 'TestBench<1>', + unit: 'testUnit', + }, + { + value: 0, + name: 'TestBench<2>', + unit: 'testUnit', + }, + { + value: 0, + name: 'TestBench<3>', + unit: 'testUnit', + }, + ], + }, + { + date: 12345, + commit: { + id: 'testCommitIdPrevious', + author: { + name: 'TestUser', + }, + committer: { + name: 'TestUser', + }, + message: 'Test previous commit message', + url: 'https://test.previous.commit.url', + }, + tool: 'benchmarkjs', + benches: [ + { + value: 0, + name: 'TestBench<1>', + unit: 'testUnit', + }, + { + value: 1, + name: 'TestBench<2>', + unit: 'testUnit', + }, + { + value: -1, + name: 'TestBench<3>', + unit: 'testUnit', + }, + ], + }, + ); + + expect(comment).toEqual(dedent` + # TestSuite + +
+ + | Benchmark suite | Current: testCommitIdCurrent | Previous: testCommitIdPrevious | Ratio | + |-|-|-|-| + | \`TestBench<1>\` | \`0\` testUnit | \`0\` testUnit | \`1\` | + | \`TestBench<2>\` | \`0\` testUnit | \`1\` testUnit | \`+∞\` | + | \`TestBench<3>\` | \`0\` testUnit | \`-1\` testUnit | \`-∞\` | + +
+ + This comment was automatically generated by [workflow](https://github.com/user/repo/actions?query=workflow%3AWorkflow%20name) using [github-action-benchmark](https://github.com/marketplace/actions/continuous-benchmark). + `); + }); +}); diff --git a/test/fakedOctokit.ts b/test/fakedOctokit.ts new file mode 100644 index 000000000..70526f506 --- /dev/null +++ b/test/fakedOctokit.ts @@ -0,0 +1,34 @@ +type OctokitOpts = { owner: string; repo: string; commit_sha: string; body: string }; +class FakedOctokitRepos { + spyOpts: OctokitOpts[]; + constructor() { + this.spyOpts = []; + } + createCommitComment(opt: OctokitOpts) { + this.spyOpts.push(opt); + return Promise.resolve({ + status: 201, + data: { + html_url: 'https://dummy-comment-url', + }, + }); + } + lastCall(): OctokitOpts { + return this.spyOpts[this.spyOpts.length - 1]; + } + clear() { + this.spyOpts = []; + } +} + +export const fakedRepos = new FakedOctokitRepos(); + +export class FakedOctokit { + rest = { + repos: fakedRepos, + }; + opt: { token: string }; + constructor(token: string) { + this.opt = { token }; + } +} diff --git a/test/write.spec.ts b/test/write.spec.ts index 1078c3385..fee85a224 100644 --- a/test/write.spec.ts +++ b/test/write.spec.ts @@ -7,6 +7,7 @@ import { Config } from '../src/config'; import { Benchmark } from '../src/extract'; import { DataJson, writeBenchmark } from '../src/write'; import { expect } from '@jest/globals'; +import { FakedOctokit, fakedRepos } from './fakedOctokit'; const ok: (x: any, msg?: string) => asserts x = (x, msg) => { try { @@ -19,41 +20,6 @@ const ok: (x: any, msg?: string) => asserts x = (x, msg) => { } }; -type OctokitOpts = { owner: string; repo: string; commit_sha: string; body: string }; -class FakedOctokitRepos { - spyOpts: OctokitOpts[]; - constructor() { - this.spyOpts = []; - } - createCommitComment(opt: OctokitOpts) { - this.spyOpts.push(opt); - return Promise.resolve({ - status: 201, - data: { - html_url: 'https://dummy-comment-url', - }, - }); - } - lastCall(): OctokitOpts { - return this.spyOpts[this.spyOpts.length - 1]; - } - clear() { - this.spyOpts = []; - } -} - -const fakedRepos = new FakedOctokitRepos(); - -class FakedOctokit { - rest = { - repos: fakedRepos, - }; - opt: { token: string }; - constructor(token: string) { - this.opt = { token }; - } -} - type GitFunc = 'cmd' | 'push' | 'pull' | 'fetch' | 'clone' | 'checkout'; class GitSpy { history: [GitFunc, unknown[]][];