Harden benchmark script against pull request changes#185
Conversation
|
Temporarily switched the benchmark job to use the branch for this PR and will need to switch it back to Test CI: https://ci.nodejs.org/job/benchmark-node-micro-benchmarks/1690/ |
| if [[ ! "$(git ls-remote origin refs/pull/${!2}/head)" =~ ^${!1}.*refs/pull/${!2}/head$ ]]; then | ||
| echo "${1} (${!1}) does not match HEAD for pull request ${!2}" |
There was a problem hiding this comment.
Could we use named variable here instead of numbers for better readability?
Require a new `TARGET` variable specifying the expected SHA of the commit at the HEAD of the pull request when running the benchmark script for pull requests.
|
New test CI run: https://ci.nodejs.org/job/benchmark-node-micro-benchmarks/1691/ |
| optional MACHINE_THREADS $getMACHINE_THREADS | ||
| rm -rf node | ||
| git clone https://github.com/nodejs/node.git | ||
| git clone --depth=1 https://github.com/nodejs/node.git |
There was a problem hiding this comment.
| git clone --depth=1 https://github.com/nodejs/node.git | |
| git clone --depth=1 --branch "$BASE" https://github.com/nodejs/node.git |
There was a problem hiding this comment.
I'll leave this for a follow up. Note that I think the suggestion will also require adding a fetch for TARGET in use case 2 to allow git checkout TARGET to succeed when TARGET points to a different branch or commit not on BASE.
| 1) | ||
| # Validate TARGET and pull request HEAD are consistent | ||
| assertMatchesPullRequest TARGET PULL_ID | ||
| git checkout $BRANCH |
There was a problem hiding this comment.
The checkout is not necessary on a shallow clone, we can only get the only ref we've pulled during the clone
| git checkout $BRANCH |
aduh95
left a comment
aduh95
left a comment
There was a problem hiding this comment.
Feel free to ignore my comments, I can open a follow up
@aduh95 I'll rename I'll leave the single branch clone optimization and case statement simplification to you for a follow up -- I think with that change the script would need to add a fetch for TARGET for use case 2 to be able to check out another branch/commit (not on the BASE branch). |
For consistency, rename the BRANCH parameter from use case 1 in the benchmark script to BASE.
|
Test run ( |
5 participants
Require a new
TARGETvariable specifying the expected SHA of the commit at the HEAD of the pull request when running the benchmark script for pull requests.