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/ |
| @@ -59,10 +81,12 @@ getMACHINE_THREADS=`cat /proc/cpuinfo |grep processor|tail -n1|awk {'print $3'}` | |||
| let getMACHINE_THREADS=getMACHINE_THREADS+1 #getting threads this way is 0 based. Add one | |||
| 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.
| cd node | ||
| case $USE_CASE in | ||
| 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 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 ( |
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.