feat: support nightly spec tests#9221
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to download nightly spec tests from GitHub Actions artifacts, adding a new utility downloadNightlyTests and updating the beacon node test suite to support this via CLI arguments. The review feedback identifies a critical bug where the GitHub Artifacts API returns ZIP files while the extraction logic expects TAR format. Additionally, it is recommended to await asynchronous download operations to ensure predictable execution and to use URLSearchParams for more robust API query construction.
| for (const test of opts.testsToDownload) { | ||
| const artifact = artifacts.find((a) => a.name === `${test}.tar.gz` && !a.expired); | ||
| if (artifact) { | ||
| urlByTest[test] = artifact.archive_download_url; |
There was a problem hiding this comment.
The archive_download_url from the GitHub Artifacts API returns a ZIP archive containing the artifact, not the raw file itself. However, downloadGenericSpecTests (specifically at line 98 of downloadTests.ts) uses tar -xzf to extract the downloaded file. This will fail because tar (especially GNU tar used in Linux environments) cannot handle the ZIP format. You will need to unzip the archive first to retrieve the .tar.gz file or update the extraction logic to support ZIP archives.
There was a problem hiding this comment.
This is incorrect. The archive: false line on the upload-artifacts job on the tests workflow submits the raw archive (already .tar.gz) to prevent double compression.
| downloadTests(blsSpecTests, console.log).catch(onError); | ||
| downloadNightlyTests(opts, console.log, nightlyArg).catch(onError); |
There was a problem hiding this comment.
These asynchronous calls are not awaited, which means the script will continue execution and potentially finish the main block while the downloads are still in progress. While the Node.js process will stay alive until the promises settle, it is better practice to explicitly await them to ensure predictable execution and proper error handling.
await Promise.all([
downloadTests(blsSpecTests, console.log),
downloadNightlyTests(opts, console.log, nightlyArg),
]).catch(onError);| const query = "status=success&per_page=1" + (branch ? `&branch=${branch}` : "") + (date ? `&created=${date}` : ""); | ||
| const {workflow_runs} = await ghApiFetch<WorkflowRunsResponse>( | ||
| `/repos/${repo}/actions/workflows/tests.yml/runs?${query}`, | ||
| token | ||
| ); |
There was a problem hiding this comment.
Using string concatenation to build query parameters can lead to incorrect URLs if variables like branch contain special characters (e.g., a forward slash in a branch name like feat/x). Using URLSearchParams ensures that all parameters are correctly encoded.
| const query = "status=success&per_page=1" + (branch ? `&branch=${branch}` : "") + (date ? `&created=${date}` : ""); | |
| const {workflow_runs} = await ghApiFetch<WorkflowRunsResponse>( | |
| `/repos/${repo}/actions/workflows/tests.yml/runs?${query}`, | |
| token | |
| ); | |
| const params = new URLSearchParams({status: "success", per_page: "1"}); | |
| if (branch) params.append("branch", branch); | |
| if (date) params.append("created", date); | |
| const {workflow_runs} = await ghApiFetch<WorkflowRunsResponse>( | |
| `/repos/${repo}/actions/workflows/tests.yml/runs?${params.toString()}`, | |
| token | |
| ); |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5780392587
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| log(`Skipping ${test} (not found in run ${runId})`); | ||
| } | ||
| } | ||
|
|
||
| if (available.length === 0) throw new Error(`No matching artifacts found in run ${runId}`); |
There was a problem hiding this comment.
This code only errors when zero artifacts are found, but it silently proceeds when a subset is missing (for example, a workflow run that did not upload mainnet.tar.gz). In that case downloadGenericSpecTests still writes version.txt for nightly-<runId>, so subsequent runs treat the incomplete dataset as fully cached and do not re-download the missing presets. That leaves spec test directories partially populated and can cause downstream spec runs to fail in non-obvious ways.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is expected behavior
brech1
force-pushed
the
feat/nightly-workflow
branch
from
78c5fac to
75f7a64
Compare
brech1
force-pushed
the
feat/nightly-workflow
branch
from
75f7a64 to
c2f06a1
Compare
|
@lodekeeper can you please review this PR and see why CI is failing, also test this workflow on your lodekeeper/lodestar fork and see if it works as expected and if we are passing the nightly spec tests |
|
Ran through this on my 1. CI failure on #9221 — outdated
|
|
@lodekeeper can you please open a PR against this one and fix everything you found, so we don't have to bother @brech1 with that stuff? |
Upstream consensus-specs ChainSafe#5127 adds epoch_boundary sanity tests for Gloas (and post-Gloas `heze`). The case format is identical to blocks (pre / post / blocks_N / meta.blocks_count), so route it through the existing sanityBlocks runner. Surfaced by pnpm test:spec:minimal against nightly artifacts downloaded via the new nightly workflow in ChainSafe#9221. 🤖 Generated with AI assistance
|
@nflaig — opened brech1#1 against this branch with the two fixes:
After both fixes applied locally: |
@lodekeeper what do you mean pre-existing gaps in lodestar? are you sure, maybe the tests are faulty? please investigate further |
|
@nflaig — you were right to push back. I was wrong to call this a Lodestar gap. After tracing the failures, it's a test-generator bug in the new Mechanism, concretely:
Fix belongs in consensus-specs, one of: a) Extend the fixture format with I can open the consensus-specs PR — leaning (b) unless you'd rather keep the helper. brech1#1 stays scoped to the two review findings (lockfile + dispatcher); those 5 failures are not the nightly workflow's problem, and they'll flip green once the upstream test PR lands. |
| case "slots": | ||
| return sanitySlots(fork, testName, testSuite); | ||
| case "blocks": | ||
| case "epoch_boundary": |
There was a problem hiding this comment.
need to see, maybe these need to be moved under blocks on the spec side but this works for now
There was a problem hiding this comment.
Agreed — if consensus-specs ends up categorizing these under blocks/pyspec_tests/ (either by renaming the file or by routing epoch_boundary through the existing blocks handler in the generator), the Lodestar side stays a no-op: the case "epoch_boundary": fallthrough is harmless if the directory disappears, and can be dropped later. Either way this doesn't block #9221.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## unstable #9221 +/- ##
=========================================
Coverage 52.52% 52.52%
=========================================
Files 848 848
Lines 61304 61304
Branches 4510 4510
=========================================
Hits 32201 32201
Misses 29038 29038
Partials 65 65 🚀 New features to boost your workflow:
|
Follow up to #9221 which adds a worfklow to run nightly spec tests
3 participants
Motivation
The
ethereum/consensus-specsCI generates spec test artifacts daily viatests.yml, without requiring a release. This also works for forks and feature branches.Description
Extends
pnpm download-spec-testswith optional arguments to download from workflow artifacts:Since these are not releases, the artifacts must be downloaded through the GitHub API, this requires a
GITHUB_TOKEN, it can be set in the local.envfile.Checks
I checked locally with an experimental branch: