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

Benchmark workflow is broken #5328

Closed
2 tasks done
mweberxyz opened this issue Feb 22, 2024 · 7 comments
Closed
2 tasks done

Benchmark workflow is broken #5328

mweberxyz opened this issue Feb 22, 2024 · 7 comments

Comments

@mweberxyz
Copy link
Contributor

mweberxyz commented Feb 22, 2024

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the issue has not already been raised

Issue

The PR benchmark tag workflow (https://github.com/fastify/fastify/blob/main/.github/workflows/benchmark.yml) is currently broken.

Dependencies are installed with npm install --only=production -- but the benchmark calls concurrently which is listed as a devDependency.

See: https://github.com/mweberxyz/fastify/actions/runs/8005536797/job/21865200203

@mweberxyz
Copy link
Contributor Author

There is also an on-going discussion for cleaning up the (also broken, but for a different reason) plugins-benchmark-pr workflow: fastify/workflows#120, so at the conclusion of that discussion, the same improvements to ensure the workflow runs correctly and is properly labeled cross-fork ought to be implemented here.

@mweberxyz
Copy link
Contributor Author

autocannon is also used by npm benchmark and listed in devDependencies

As for the fix, would it be preferable to:
a) remove the --only=production from the Install steps, or
b) explicitly install autocannon and concurrently via npm during the workflow Install steps, keeping the npm dependencies installed to a minimum during benchmark runs?

@Uzlopak
Copy link
Contributor

Uzlopak commented Feb 22, 2024

remove tthe --only=production is imho good

@climba03003
Copy link
Member

climba03003 commented Feb 23, 2024

The history is just remove and adding back, remove and adding it back.
Removed from #3167
Added from #5240

Why not just keep the devDependencies and use npx so it can either get from devDependencies when it exist or download when not exist.

@mweberxyz
Copy link
Contributor Author

I've got some potential improvements cooking over in fastify/workflows#121

If there's interest in moving in that direction, I will push forward to get the necessary changes made here which would resolve this issue, otherwise I'll just get a small PR up to fix this specific issue.

@Uzlopak
Copy link
Contributor

Uzlopak commented May 3, 2024

@mweberxyz

Is this resolved?

@gurgunday
Copy link
Member

I fixed it with 35a6b99

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

No branches or pull requests

4 participants