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

Compare against the merge-base if no vs is specified #134

Merged
merged 5 commits into from
Nov 24, 2022
Merged

Conversation

maleadt
Copy link
Member

@maleadt maleadt commented Nov 18, 2022

Currently, PkgEvalJob was comparing against the git ref as specified (which is bad because you could then be comparing against a branch that's much ahead, e.g., JuliaLang/julia#47369 (comment)), and BenchmarkJob was checking out the merge commit. I'd argue that the latter isn't ideal as well: sometimes the merge commit is unavailable (in case of merge conflicts) leading to inconsistent behavior; the merge commit isn't available directly so if you request runbenchmarks as part of the PR opening post you'll end up comparing against a different commit; and finally the merge base is a commit that you can't find in the GitHub UI, so the versioninfo() as reported in the benchmark reports isn't very useful.

As an alternative, this PR switches to using the merge-base, i.e., the commit on master where the PR forked. However, as it's not clear when to look up the merge-base (e.g., only when specifying vs=:master, or also when specifying vs=v1.8 or vs=SOME_SHA), I've implemented @KristofferC's suggestion here to default to the master branch when not specifying a vs at all from a Nanosoldier invocation on a PR. This also considerably simplifies the most common Nanosoldier invocation to: @nanosoldier runbenchmarks(ALL) or @nanosoldier runtests(ALL).

I'll be testing this on the PkgEval bot in the coming days.

Fixes #131

@maleadt
Copy link
Member Author

maleadt commented Nov 19, 2022

As suggested by @KristofferC: maybe ALL should be the default package selection so that in the common case you just do runtests().

@maleadt
Copy link
Member Author

maleadt commented Nov 22, 2022

@vtjnash Any thoughts, as this does change BenchmarkJob?

@vtjnash
Copy link
Member

vtjnash commented Nov 22, 2022

That rationale all seems reasonable

@maleadt maleadt merged commit ee54dd6 into master Nov 24, 2022
@maleadt maleadt deleted the tb/dev branch November 24, 2022 12:51
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.

When comparing against master: use base of target branch
2 participants