fix(plugins-benchmark-pr): run comparison benchmark against target #120
Conversation
…R base Closes fastify#93 Previously, the second benchmark ran against the default branch of the (potentially forked) repo, which might not be in sync with the PR target repo.
|
Did you test it somehow? |
|
@Uzlopak Take a look here: mweberxyz/point-of-view#3 The sample PR merges fastify/point-of-view@master into mweber/point-of-view@master, so you would expect the initial benchmark run to be against fastify/pov, and the comparison benchmark run to be against mweberxyz/pov. Note that because all the testing is happening in a fork, everything is "backwards". The benchmark tag was added, which triggered the workflow at fastify/workflows@main was used. Then, I updated the workflow configuration, added the tag, and the workflow at mweberxyz:fix/benchmark-runs-against-merge-target was used -- https://github.com/mweberxyz/point-of-view/actions/runs/7979681524/job/21787636566 Initial run: fastify/workflows@mainOn the initial run, the first benchmark ran against fastify/point-of-view@master as expected, but then the second benchmark ran against fastify/point-of-view@master again.
Second run: mweberxyz:fastify-workflows@fix/benchmark-runs-against-merge-targetOn the second run, the first benchmark ran against fastify/point-of-view@master as expected, and then the second benchmark ran against mweberxyz/point-of-view@master -- which is expected because that is what the PR is trying to merge into.
|
|
I suppose the PR comments created by the workflow could be more clear as well. If you look at the sample PR, it just says "master" and "master" -- not indicating which master we are talking about. I'll take a peek at fixing that. |
|
thank you for your support |
mweberxyz
force-pushed
the
fix/benchmark-runs-against-merge-target
branch
3 times, most recently
from
ada03e3
to
9c0cccd
Compare
mweberxyz
force-pushed
the
fix/benchmark-runs-against-merge-target
branch
from
9c0cccd
to
a9911bc
Compare
|
@Uzlopak Ready for review! I accidentally added some functionality. Previously, the workflow always ran the second benchmark against the default branch, but now it runs against the target of the PR. If any user of this workflow wants to keep the current functionality, they can specify See here for benchmark output: mweberxyz/point-of-view#3 (comment) |
|
I would recommend to merge and release and see if it works like it should. |
For consistency
|
Benchmark workflow run as of most recent commits: https://github.com/mweberxyz/point-of-view/actions/runs/8006413114/job/21868091703?pr=3 |
|
Also need to push docs changes. |
In my fork, the benchmark label was not removed with a 403 after the benchmark run, because it was trying to remove the label from PR in fastify repo — not the PR in forked repo.
Closes #93
Previously, the second benchmark run ran against the default branch of the (potentially forked) repo, which might not be in sync with the PR target repo.
Checklist
and the Code of conduct