ci: add a manual workflow for the frame-transaction measurement harnesses - #12738
ci: add a manual workflow for the frame-transaction measurement harnesses#12738AnkushinDaniil wants to merge 5 commits into
Conversation
|
Claude finished @AnkushinDaniil's task in 4m 58s —— View job Review:
|
| # | Severity | Location | Issue |
|---|---|---|---|
| 1 | Medium | L44-47 | -f "${project}/${fixture}.cs" assumes the harness sits at the project root under exactly that filename. Nethermind.Evm.Test has 31 .cs files in subdirectories — a nested or differently-named harness fails the run claiming the ref doesn't carry it. |
| 2 | Medium | L55-62 | With harness: both (default), set -e aborts after a failing verify harness, so mempool never runs. The artifact then holds partial results that look complete. Note the function currently returns the status of its trailing echo "::endgroup::", so it always reports success — that has to be fixed first for any ||-based collection to work. |
| 3 | Medium | L20 | No permissions: block. The job needs only contents: read; it executes arbitrary code from a dispatched ref on a persistent self-hosted runner, so bounding the token is cheap insurance. Both nethermind-tests.yml and the expb workflow declare theirs. |
| 4 | Low | L48-52 | ::endgroup:: is skipped when the test command fails, leaving the failing output collapsed. Also suggest MTP's --no-ansi so the captured RESULT lines land clean (per the ANSI caveat in AGENTS.md). |
| 5 | Low | L69-77 | Whole dotnet test log is appended to $GITHUB_STEP_SUMMARY (1 MiB cap). Overflow fails the step under set -e, worst exactly when a harness misbehaves. Filter to RESULT lines or bound with tail -c; the artifact keeps the full log. |
Also folded into #2: the output_dir step output is written but never read — the path is spelled three separate times, and the later steps can't use the output anyway since they are if: always().
} that duplicates line 53. Apply it over L48-53, or drop that brace. Flagged inline too.
I have not pushed any changes — the GitHub App token can't modify files under .github/workflows. Fix these →
· branch daniil/frame-measurement-workflow
4a840d6 to
5cc0d10
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
@claude re-review |
|
Claude finished @AnkushinDaniil's task in 2m 55s —— View job Re-review:
|
| # | Was | Now |
|---|---|---|
| 1 | -f "${project}/${fixture}.cs" missed nested harnesses |
find … -print -quit (L50) — layout-independent, keeps the friendly message |
| 2 | harness: both skipped the second harness after a failure |
set -e dropped, run_harness returns the captured rc (L59), call sites collect into status (L64-72). The ordering matters and it's right: ` |
| 3 | No permissions: |
contents: read (L20-21) |
| 4 | ::endgroup:: skipped on failure; ANSI in the report |
` |
| 5 | Whole log into a 1 MiB-capped summary | grep '^RESULT' with an explicit fallback (L85) |
| 6 | Unused output_dir step output, path spelled 3× |
Exported once to GITHUB_ENV (L35), read by both consumers |
On #6 — your reasoning is correct, not a workaround. Job-level env: can only see github, needs, strategy, matrix, vars, secrets, and inputs; runner is step-scope only, so GITHUB_ENV is the right mechanism. The comment on L32-33 records that, which is exactly the kind of why worth keeping.
Also verified: set -uo pipefail (L45) — pipefail is what makes dotnet test | tee || rc=$? report the test's status rather than tee's, so dropping -e doesn't create a silent-success path. Action pins still match repo convention (checkout@v6, setup-dotnet@v5, upload-artifact@v7). --project X/X.csproj -c release -- --filter is the form in AGENTS.md:124, and both Nethermind.Evm.Test and Nethermind.TxPool.Test resolve. --no-ansi is a Microsoft.Testing.Platform built-in, and tests.props:5 sets EnableNUnitRunner, so it's accepted.
New findings
| # | Severity | Location | Issue |
|---|---|---|---|
| 1 | Medium | L29-35 | Resolve the output directory now runs after checkout. If checkout fails it is skipped, and the if: always() upload at L95 interpolates an unset env.OUTPUT_DIR to the empty string — path: /, globbing the whole filesystem of a persistent self-hosted runner. Moving the step above the checkout closes it; it has no dependency on the checkout. |
| 2 | Low | L85 | ^RESULT bets on column 0, which NUnit-under-MTP output capture doesn't always give. Also L57 tees stdout only, so stderr is missing from the artifact. Both degrade gracefully thanks to the ` |
Finding 1 is a one-line reorder and is the only thing I'd hold the merge for.
One optional nit carried over: the deliberate concurrency deviation (no -${{ github.ref }}, cancel-in-progress: false, against the default in .agents/rules/github-workflows.md:8) is still uncommented. It's the correct choice for serialising dispatches on a shared measurement runner — a short comment would stop someone "fixing" it back.
I have not pushed any changes — the GitHub App token cannot modify files under .github/workflows.
· branch daniil/frame-measurement-workflow
|
@claude re-review |
|
Claude finished @AnkushinDaniil's task in 1m 49s —— View job Re-review:
|
| # | Was | Now |
|---|---|---|
| 1 (Medium) | Resolve the output directory ran after checkout; a checkout failure left OUTPUT_DIR unset and the if: always() upload globbed / |
Resolved first (L33-34), before anything that can fail. The comment on L29-32 records why the ordering matters, not just that it exists — that's the version worth keeping |
| 2 (Low) | ^RESULT anchored at column 0; the report captured stdout only |
grep 'RESULT' (L87) and 2>&1 | tee (L59) |
Re-verified from the round before, since the file moved again: find … -print -quit presence check (L52) is still layout-independent; run_harness still returns the captured rc (L61) so || status=1 at L68/L71 actually collects, and harness: both runs both harnesses before exiting non-zero; permissions: contents: read (L20-21) is intact; --no-ansi (L58) is intact. set -uo pipefail without -e is still the right combination — pipefail is what makes the | tee report the test's status rather than tee's, and dropping -e is what lets the second harness run.
Also checked: action pins match the repo majority (checkout@v6 ×88, upload-artifact@v7 ×33), bare setup-dotnet@v5 matches nethermind-tests.yml:142 and picks the SDK up from global.json, and runs-on: [self-hosted, reproducible-benchmarks] matches the label .agents/rules/github-workflows.md names for benchmark jobs.
mkdir -p "${OUTPUT_DIR}" without a prior clean is fine — the runner empties RUNNER_TEMP at the start of each job, so a verify-only dispatch cannot pick up a stale mempool report from a previous run.
Remaining
| # | Severity | Location | Issue |
|---|---|---|---|
| 1 | Low | L92-97 | if: always() fires on cancellation too, so a dispatch cancelled before L34 still leaves env.OUTPUT_DIR empty and uploads path: /. A few-second window on a 120-minute job; if: always() && env.OUTPUT_DIR != '' closes it and keeps the invariant next to the step that depends on it. |
| 2 | Nit | L16-18 | The deliberate concurrency deviation — no -${{ github.ref }}, cancel-in-progress: false, against the default in .agents/rules/github-workflows.md:8 — is still uncommented. It is the right call for serialising dispatches on a shared measurement runner; one line would stop someone "fixing" it back to the PR-workflow default and having two measurement runs contend for the box. |
Neither blocks the merge.
I have not pushed any changes — the GitHub App token cannot modify files under .github/workflows.
· branch daniil/frame-measurement-workflow
The concurrency group intentionally omits the ref and keeps in-progress runs alive so dispatches queue for the single shared measurement runner instead of cancelling each other.
|
Superseded by #12939, which re-bases this onto |
| return 1 | ||
| fi | ||
| echo "::group::${fixture}" | ||
| dotnet test --project "${project}/${project}.csproj" -c release -- \ |
There was a problem hiding this comment.
dotnet test builds in-job, which leaves MSBuild worker nodes and VBCSCompiler resident for their idle timeout — right through the measurement, on a box whose whole point is reproducible timing, and then into whatever job the runner takes next. Building first, running dotnet build-server shutdown, then testing with --no-build --no-restore keeps them out of that window. nethermind-tests.yml:161 already does the shutdown.
dotnet build "${project}/${project}.csproj" -c release || return 1
dotnet build-server shutdown || true
dotnet test --project "${project}/${project}.csproj" -c release --no-build --no-restore --output Detailed -- \
--no-ansi --filter "FullyQualifiedName~${fixture}" \
2>&1 | tee "${OUTPUT_DIR}/${fixture}.txt" || rc=$?| HARNESS: ${{ inputs.harness }} | ||
| run: | | ||
| set -uo pipefail | ||
| mkdir -p "${OUTPUT_DIR}" |
There was a problem hiding this comment.
The output directory is a fixed path on a persistent runner and is only mkdir -p'd, never cleaned. If a report file survives an earlier dispatch, a harness: mempool run republishes the previous verify report in its summary and artifact as if it had just been measured — the summary loop globs every *.txt and the upload takes the whole directory. An rm -rf before the mkdir -p removes the possibility entirely.
set -uo pipefail
rm -rf "${OUTPUT_DIR}"
mkdir -p "${OUTPUT_DIR}"2 participants
Changes
run-frame-tx-measurements.yml, a manual workflow that runs the frame-transaction measurement harnesses on thereproducible-benchmarksrunner and publishes what they reportThe two harnesses (
FrameTxVerifyDosMeasurementandFrameTxMempoolDosMeasurement) are[Explicit]measurement fixtures on the frame-transaction branches, so they never run in regular CI. SizingMAX_VERIFY_GASneeds their numbers from a machine that does not move underfoot, which is what the benchmark runner is for.The workflow is dispatch-only and takes no part in PR or push CI. It exists on
masteronly becauseworkflow_dispatchis not offered for a workflow that is absent from the default branch; the harnesses themselves are not onmasterand the run fails with a clear message if the dispatched ref does not carry them.Output goes to the run summary and to a
frame-tx-measurementsartifact, so a number quoted in a spec discussion has a run behind it.Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
The invocation was checked locally against the harness branch:
dotnet Nethermind.Evm.Test.dll --filter FullyQualifiedName~FrameTxVerifyDosMeasurementruns all 12 cases and prints theRESULTlines the workflow captures. A filter that matches nothing exits non-zero, so a bad ref fails the run rather than reporting an empty success.Documentation
Requires documentation update
Requires explanation in Release Notes