Skip to content

Commit

Permalink
fix: 204 ratio is NaN when previous value is 0 (#222)
Browse files Browse the repository at this point in the history
* extract getRatio function
* print 1 when both values are 0
* print +-∞ when divisor is 0
  • Loading branch information
ktrz authored Jan 31, 2024
1 parent 785a741 commit e108afd
Show file tree
Hide file tree
Showing 7 changed files with 268 additions and 42 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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)

<a name="v1.19.2"></a>
# [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)
Expand Down
1 change: 1 addition & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
27 changes: 20 additions & 7 deletions src/write.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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}`,
'',
Expand All @@ -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 {
Expand Down Expand Up @@ -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
}
209 changes: 209 additions & 0 deletions test/buildComment.test.ts
Original file line number Diff line number Diff line change
@@ -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
<details>
| 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 | \`-∞\` |
</details>
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
<details>
| 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 | \`-∞\` |
</details>
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).
`);
});
});
34 changes: 34 additions & 0 deletions test/fakedOctokit.ts
Original file line number Diff line number Diff line change
@@ -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 };
}
}
36 changes: 1 addition & 35 deletions test/write.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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[]][];
Expand Down

0 comments on commit e108afd

Please sign in to comment.