fix(ci): build dist before running benchmarks - #343
Merged
Conversation
The benchmark adapter loads lambda-api from the working tree via
require('../../'), which resolves through the root package's "main".
jeremydaly#326 changed that from index.js to ./dist/cjs/index.js, so the adapter
now needs a build that the workflow never performed — it only ran
npm ci inside benchmarks/. The job crashed on the first lambda-api
scenario with MODULE_NOT_FOUND.
Also honor LAMBDA_API_VERSION in the adapter. run.js already used it
for the caption, but the per-framework table rows read package.json
directly, so every row rendered as 0.0.0-development.
Collaborator
Author
|
Added a third fix (6bcb402): the final step now opens a PR instead of pushing to the default branch.
One thing to be aware of before merging: PRs opened with the default If that's a problem, the usual fix is to create the PR with a PAT or GitHub App token instead of |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The Benchmarks workflow crashed on the v1.5.0 release (run 31267507913):
Root cause
benchmarks/frameworks/lambda-api.jsloads the library from the working tree withrequire('../../'), which resolves through the root package'smain. #326 changedmainfromindex.jsto./dist/cjs/index.js, so the adapter now requires a build — but the workflow only rannpm ciinsidebenchmarks/, never installing or building the root package.The benchmark suite (#327) merged before the dual build (#326), and benchmarks only run on
release: published, so v1.5.0 was the first time the two met. Thebaselineframework has no lambda-api dependency and completed fine, which is why the log shows 10 green scenarios before the crash.Fixes
1. Build
dist/before benchmarking. Adds a rootnpm ci --ignore-scripts+npm run buildahead of the benchmark install, matching howrelease.ymlsequences the same steps.2. Honor
LAMBDA_API_VERSIONin the adapter. Surfaced while verifying the first fix.run.js:65already read the env var, but only for the run metadata that renders the caption — the per-framework table rows use each adapter's ownversionfield, and lambda-api's readpackage.jsondirectly. Since that ships a0.0.0-developmentplaceholder until publish time, every table row would have rendered as:while the caption directly above it correctly said
lambda-api v1.5.0. That would have been committed straight into the README by the workflow's final step. The adapter now uses the sameLAMBDA_API_VERSION || pkg.versionprecedence asrun.js.Verification
Reproduced locally by deleting
dist/— sameMODULE_NOT_FOUNDat the same adapter line; loads cleanly afternpm run build.Ran the exact workflow command end-to-end (
node run.js --md results/RESULTS.md --json results/raw.json --update-readmewithLAMBDA_API_VERSION=1.5.0). All 6 frameworks × 10 scenarios complete, and both the caption and every table row now read1.5.0. Generated results were reverted out of this branch — the committed numbers should come from the runner, not my laptop.The subsequent
Format READMEstep pinsprettier@2, which matches the repo's^2.3.2devDependency, so no version skew there.Note
The v1.5.0 README still has stale benchmark numbers, since the workflow never reached its commit step. Once this merges, re-running the workflow via
workflow_dispatchwith version1.5.0will refresh it.