Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add optional commit SHA in config. #93

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ context) properties. Like this:
"unit": "Megabytes",
"value": 100,
"range": "3",
"extra": "Value for Tooltip: 25\nOptional Num #2: 100\nAnything Else!",
"extra": "Value for Tooltip: 25\nOptional Num #2: 100\nAnything Else!"
}
]
```
Expand Down Expand Up @@ -469,9 +469,16 @@ with `actions/cache` action. Please read 'Minimal setup' section above.

Max number of data points in a chart for avoiding too busy chart. This value must be unsigned integer
larger than zero. If the number of benchmark results for some benchmark suite exceeds this value,
the oldest one will be removed before storing the results to file. By default this value is empty
the oldest one will be removed before storing the results to file. By default, this value is empty
which means there is no limit.

#### `commit-sha` (Optional)

- Type: String
- Default: N/A

The commit SHA to tag the benchmark with. Uses this in place of the payload if provided.


### Action outputs

Expand Down
3 changes: 3 additions & 0 deletions action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,9 @@ inputs:
max-items-in-chart:
description: 'Max data points in a benchmark chart to avoid making the chart too busy. Value must be unsigned integer. No limit by default'
required: false
commit-sha:
description: 'The commit SHA to tag the benchmark with. If not provided, uses GitHub context.'
required: false

runs:
using: 'node12'
Expand Down
6 changes: 6 additions & 0 deletions src/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ export interface Config {
alertCommentCcUsers: string[];
externalDataJsonPath: string | undefined;
maxItemsInChart: number | null;
commitSha: string | undefined;
}

export const VALID_TOOLS: ToolType[] = [
Expand Down Expand Up @@ -240,6 +241,7 @@ export async function configFromJobInput(): Promise<Config> {
let externalDataJsonPath: undefined | string = core.getInput('external-data-json-path');
const maxItemsInChart = getUintInput('max-items-in-chart');
let failThreshold = getPercentageInput('fail-threshold');
const commitSha: string | undefined = core.getInput('commit-sha') || undefined;

validateToolType(tool);
outputFilePath = await validateOutputFilePath(outputFilePath);
Expand All @@ -255,6 +257,9 @@ export async function configFromJobInput(): Promise<Config> {
if (commentOnAlert) {
validateGitHubToken('comment-on-alert', githubToken, 'to send commit comment on alert');
}
if (commitSha) {
validateGitHubToken('commit-sha', githubToken, 'to retrieve the commit info');
}
validateAlertThreshold(alertThreshold, failThreshold);
validateAlertCommentCcUsers(alertCommentCcUsers);
externalDataJsonPath = await validateExternalDataJsonPath(externalDataJsonPath, autoPush);
Expand All @@ -281,5 +286,6 @@ export async function configFromJobInput(): Promise<Config> {
externalDataJsonPath,
maxItemsInChart,
failThreshold,
commitSha,
};
}
14 changes: 9 additions & 5 deletions src/extract.ts
Original file line number Diff line number Diff line change
Expand Up @@ -202,13 +202,13 @@ function getCommitFromPullRequestPayload(pr: PullRequest): Commit {
};
}

async function getCommitFromGitHubAPIRequest(githubToken: string): Promise<Commit> {
async function getCommitFromGitHubAPIRequest(githubToken: string, commitSha?: string): Promise<Commit> {
const octocat = new github.GitHub(githubToken);

const { status, data } = await octocat.repos.getCommit({
owner: github.context.repo.owner,
repo: github.context.repo.repo,
ref: github.context.ref,
ref: commitSha ?? github.context.ref,
});

if (!(status === 200 || status === 304)) {
Expand All @@ -235,7 +235,11 @@ async function getCommitFromGitHubAPIRequest(githubToken: string): Promise<Commi
};
}

async function getCommit(githubToken?: string): Promise<Commit> {
async function getCommit(githubToken?: string, commitSha?: string): Promise<Commit> {
if (commitSha && githubToken) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this line be placed after the line 253-261 ? Mostly I can't see any reason to let this success faster though.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, if you've provided the commit SHA, then your intention is for the action to use it. I don't see why it should even attempt to use the other options.

return getCommitFromGitHubAPIRequest(githubToken, commitSha);
}

if (github.context.payload.head_commit) {
return github.context.payload.head_commit;
}
Expand Down Expand Up @@ -564,7 +568,7 @@ function extractCustomBenchmarkResult(output: string): BenchmarkResult[] {

export async function extractResult(config: Config): Promise<Benchmark> {
const output = await fs.readFile(config.outputFilePath, 'utf8');
const { tool, githubToken } = config;
const { tool, githubToken, commitSha } = config;
let benches: BenchmarkResult[];

switch (tool) {
Expand Down Expand Up @@ -603,7 +607,7 @@ export async function extractResult(config: Config): Promise<Benchmark> {
throw new Error(`No benchmark result was found in ${config.outputFilePath}. Benchmark output was '${output}'`);
}

const commit = await getCommit(githubToken);
const commit = await getCommit(githubToken, commitSha);

return {
commit,
Expand Down
10 changes: 9 additions & 1 deletion test/config.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ describe('configFromJobInput()', function () {
'alert-comment-cc-users': '',
'external-data-json-path': '',
'max-items-in-chart': '',
'commit-sha': '',
};

const validation_tests = [
Expand Down Expand Up @@ -158,6 +159,11 @@ describe('configFromJobInput()', function () {
inputs: { ...defaultInputs, 'alert-threshold': '150%', 'fail-threshold': '120%' },
expected: /'alert-threshold' value must be smaller than 'fail-threshold' value but got 1.5 > 1.2/,
},
{
what: 'commit-sha is set but github-token is not set',
inputs: { ...defaultInputs, 'commit-sha': 'dummy sha', 'github-token': '' },
expected: /'commit-sha' is enabled but 'github-token' is not set/,
},
] as Array<{
what: string;
inputs: Inputs;
Expand Down Expand Up @@ -185,6 +191,7 @@ describe('configFromJobInput()', function () {
hasExternalDataJsonPath: boolean;
maxItemsInChart: null | number;
failThreshold: number | null;
commitSha: string | undefined;
}

const defaultExpected: ExpectedResult = {
Expand All @@ -201,6 +208,7 @@ describe('configFromJobInput()', function () {
hasExternalDataJsonPath: false,
maxItemsInChart: null,
failThreshold: null,
commitSha: undefined,
};

const returnedConfigTests = [
Expand Down Expand Up @@ -350,7 +358,7 @@ describe('configFromJobInput()', function () {
const absCwd = process.cwd();
if (!absCwd.startsWith(home)) {
// Test was not run under home directory so "~" in paths cannot be tested
fail('Test was not run under home directory so "~" in paths cannot be tested');
A.fail('Test was not run under home directory so "~" in paths cannot be tested');
}

const cwd = path.join('~', absCwd.slice(home.length));
Expand Down
2 changes: 1 addition & 1 deletion test/extract.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ const dummyWebhookPayload = {
let dummyCommitData = {};
class DummyGitHub {
repos = {
getCommit: () => {
getCommit: async () => {
return {
status: 200,
data: dummyCommitData,
Expand Down
2 changes: 2 additions & 0 deletions test/write.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,7 @@ describe('writeBenchmark()', function () {
externalDataJsonPath: dataJson,
maxItemsInChart: null,
failThreshold: 2.0,
commitSha: undefined,
};

const savedRepository = gitHubContext.payload.repository;
Expand Down Expand Up @@ -885,6 +886,7 @@ describe('writeBenchmark()', function () {
externalDataJsonPath: undefined,
maxItemsInChart: null,
failThreshold: 2.0,
commitSha: 'dummy sha',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should add specific tests for the use-case where we set the explicit commitSha rather than just adding this param here.

};

function gitHistory(
Expand Down