fastify / workflows

Reusable workflows for use in the Fastify organization
MIT License
9 stars 6 forks source link

fix(plugins-benchmark-pr): run comparison benchmark against target #120

Closed mweberxyz closed 6 months ago

mweberxyz commented 6 months ago

Closes fastify/workflows#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

Uzlopak commented 6 months ago

Did you test it somehow?

mweberxyz commented 6 months ago

@Uzlopak Take a look here: https://github.com/mweberxyz/point-of-view/pull/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@main

On 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.

CleanShot 2024-02-20 at 16 12 46@2x

Second run: mweberxyz:fastify-workflows@fix/benchmark-runs-against-merge-target

On 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.

CleanShot 2024-02-20 at 16 15 32@2x

mweberxyz commented 6 months ago

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.

Uzlopak commented 6 months ago

thank you for your support

mweberxyz commented 6 months ago

@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 target_sha and target_ref inputs to the current master sha.

See here for benchmark output: https://github.com/mweberxyz/point-of-view/pull/3#issuecomment-1955425966

Uzlopak commented 6 months ago

I would recommend to merge and release and see if it works like it should.

mweberxyz commented 6 months ago

Benchmark workflow run as of most recent commits: https://github.com/mweberxyz/point-of-view/actions/runs/8006413114/job/21868091703?pr=3

mweberxyz commented 6 months ago

Also need to push docs changes.