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

ci: Parameterize BenchmarkId on output type in PR bench comments #829

Conversation

samuelburnham
Copy link
Member

@samuelburnham samuelburnham commented Nov 1, 2023

This PR adds the LURK_BENCH_OUTPUT env var, which enables customization of the fibonacci_lem BenchmarkId for differently formatted output types.

Problem: boa-dev/criterion-compare-action PR comment is printing incorrectly: #825 (comment). critcmp can't compare benchmarks with different BenchmarkId's, and our fibonacci_lem benchmark includes the commit hash in the ID.

Solution: Remove the commit hash for the pr-comment output type, and keep it for the other types for now.

@samuelburnham samuelburnham requested review from a team as code owners November 1, 2023 16:28
Copy link
Member

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

✨ ✨

Comment on lines +64 to +77
let output_type = bench_parameters_env().unwrap_or("stdout".into());
match output_type.as_ref() {
"pr-comment" => format!("num-{}", self.fib_n),
"commit-comment" => todo!(),
"gh-pages" => todo!(),
_ => format!("num-{}/{}-{}", self.fib_n, self.sha, self.date),
}
}
}

fn bench_parameters_env() -> anyhow::Result<String> {
std::env::var("LURK_BENCH_OUTPUT")
.map_err(|e| anyhow!("Noise threshold env var isn't set: {e}"))
}
Copy link
Member

Choose a reason for hiding this comment

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

At this stage, this probably indicates we want to document what folks are allowed to put in the variables defined in the env file.

@samuelburnham samuelburnham added this pull request to the merge queue Nov 1, 2023
Merged via the queue into argumentcomputer:master with commit 0a94149 Nov 1, 2023
15 checks passed
@samuelburnham samuelburnham deleted the ci-bench-comment-context branch November 1, 2023 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants