Skip to content

feat(test-harness): one machine-written integration PR summary#531

Open
nebasuke wants to merge 5 commits into
mainfrom
integration-report-summary
Open

feat(test-harness): one machine-written integration PR summary#531
nebasuke wants to merge 5 commits into
mainfrom
integration-report-summary

Conversation

@nebasuke

@nebasuke nebasuke commented Jul 10, 2026

Copy link
Copy Markdown
Member

PR adding the status of an integration test run as a comment. My goal is that it should be clear if an integration test was successful (with the goals in mind). In particular, the comment states the following:

  • Whether it is output preserving -> good in case of a performance improvement that should not have impact on output
  • Whether we have introduced new test failures -> generally should be a fail.
  • Numbers on compile time, gas reports along with sensible defaults to allow for jitter.

The original reports are preserved, and are linked in the PR comment.

Claude Summary

Why

The integration workflow posted three link-only PR comments; all analysis of the XLSX reports happened offline (the local solx-perf-review skill's parse_reports.py). Nothing on the PR told you whether output changed, what failed, or how compile-time moved.

What

A single machine-written comment (integration-test-summary), rendered in-tree in Rust from the converter's native Benchmark model — no XLSX round-trip, no Python. Designed so a reviewer can call pass/fail from the comment alone, using the same decision procedure as the offline review.

  • New render_summary (solx-benchmark-converter/src/output/markdown.rs) + solx-benchmark-summary binary.
  • solx-dev now writes {foundry,hardhat}-benchmark.json next to each XLSX (one shared writer); the workflow emits solx-tester-benchmark.json. The summary reads those.
  • The three link comments collapse into one comment (reusing mshick/add-pr-comment via message-path), also mirrored to $GITHUB_STEP_SUMMARY.
  • XLSX artifacts are unchanged — still uploaded, still linked (by their real file names) from the summary table.

The comment

Three independent verdict lines, each with its numbers:

  • Output invariance — bytecode size (deterministic, all suites) + solx-tester gas (deterministic REVM). A size change informs, never hides: ⚠️ headline + the largest movers listed inline (a size change means semantics possibly changed — but intentional codegen PRs exist, so it is not a red ❌). Foundry/Hardhat gas is fuzz/CREATE-noisy → shown as jitter diff-count + median (never the max, which is always a huge meaningless CREATE-deploy outlier), never gated.
  • New failures — PR vs main per paired run; pre-existing baselines are stated, only regressions alarm (❌), and the regressed projects are listed inline. A PR run with no main counterpart is surfaced as unbaselined (⚪/⚠️), not counted as regressions against an imaginary zero.
  • Harness health — a suite that errored, produced a corrupt report, or whose data matches no recognized toolchain naming is an explicit ❌ line + row; one bad suite never suppresses the healthy suites' results, and the comment never shows a green checkmark over empty data.

Every percentage states its direction (positive = PR larger / PR slower); no internal toolchain names leak (00.solc → "vs solc", mode keys humanized, report links named per suite).

Example (output-preserving, full matrix):

Output-preserving — bytecode size identical (54,480 comparisons), solx-tester gas identical (63,720).
No new failures — Foundry's 685 / Hardhat's 40 failures already present on main.

Suite New failures Size Δ Gas Δ Report
solx-tester ✅ 0 ✅ 0 of 49,508 ✅ 0 of 63,720 solx-tester-report.xlsx ↓
Foundry · 25 proj ✅ 0 (685 pre-existing) ✅ 0 of 4,972 ⚪ jitter 90 of 2,486, median 0.4% (not gated) foundry-report.xlsx ↓
Hardhat · 3 proj ✅ 0 (40 pre-existing) ⚪ not collected ⚪ not collected hardhat-report.xlsx ↓

Plus: a compile-time tripwire table (aggregate / median per pipeline, columns from the pipelines present in the data, ⚠️ at ≥5%, project outliers at ≥15%, "within noise" line when quiet — authoritative deltas stay with ci:compile-benchmark), and an always-expanded full-matrix table "Bytecode size — PR vs baselines" with vs solc / vs released solx columns, summed over contracts built by both toolchains only so a baseline that failed some builds is excluded rather than skewing the percentage.

Failure behaviour

  • A suite that runs but regresses vs main writes its report first, so it's included (with the regressed projects listed) and turns the verdict red.
  • A suite that hard-errors (no *-benchmark.json, or a corrupt one) gets an explicit "❌ Suite errored" verdict line and table row; the healthy suites still post.
  • A suite skipped by an earlier hard failure is omitted (its flag isn't passed), not mislabeled as errored; a missing main-side tester baseline renders the PR data as unbaselined instead of "suite errored".
  • Losing the summary itself fails the job visibly (no continue-on-error on the generation step).
  • Benchmark data matching no recognized toolchain naming is a loud "❌ Harness error" — the guard against naming drift silently disabling the gate (the fix(benchmark): solx-tester skips E codegen; Hardhat ignores the configured solx #497 class).

Verification

  • 16 converter unit tests (three-verdict split, new-vs-main failure pairing incl. pre-existing-only and unbaselined cases, unrecognized-naming guard, common-contracts baseline sums, gas-jitter median, compile outlier / within-noise / median, errored-suite row, named links, commas); clippy + fmt clean.
  • Binary exercised end-to-end on synthetic full-matrix pass data, a combined output-changed + new-failures + errored-suite scenario, and a corrupt-suite-JSON scenario (healthy suites still render).
  • actionlint + shellcheck clean (also fixed a pre-existing useless-cat).
  • No Cargo.lock change (deps already present; --frozen safe).

Real-data proof is this PR's own CI run — apply ci:integration (+ ci:integration-benchmark-full for the full matrix). Supersedes the offline parse_reports.py.

@nebasuke nebasuke added ci:integration Trigger integration tests workflow on PR ci:integration-benchmark-full Run the full benchmark matrix (all compilers + comparisons) in integration tests labels Jul 10, 2026
@github-actions

github-actions Bot commented Jul 10, 2026

Copy link
Copy Markdown

🧪 Integration tests — full matrix · PR vs main

⚠️ Output changed — 1 of 63,720 solx-tester gas comparisons differ. If this PR is meant to be output-preserving, investigate before merging.
No new failures — Foundry's 685 / Hardhat's 40 failures already present on main.

Suite New failures Size Δ Gas Δ Report
solx-tester ✅ 0 ✅ 0 of 49,508 ⚠️ 1 of 63,720 solx-tester-report.xlsx ↓
Foundry · 25 proj ✅ 0 (685 pre-existing) ✅ 0 of 4,972 ⚪ jitter 80 of 5,214, median <0.1% (not gated) foundry-report.xlsx ↓
Hardhat · 3 proj ✅ 0 (40 pre-existing) ⚪ not collected ⚪ not collected hardhat-report.xlsx ↓

solx-tester — largest gas changes:

  • solx-solidity/test/libsolidity/semanticTests/structs/delete_struct.sol [Yul MzB3 0.8.34] 85,899 → 85,902 (+0.0%)

Compile time — wall-clock tripwire, positive = PR slower (authoritative Δ in ci:compile-benchmark)

Suite legacy (agg / median) viaIR (agg / median)
Foundry · 25 proj +0.1% / +0.2% -0.1% / +0.2%
Hardhat · 3 proj +0.2% / +0.3% +0.7% / +0.7%

Within noise — no suite ≥ 5%, no project ≥ 15%.

Bytecode size — PR vs baselines (positive = PR larger; contracts built by both only)

Suite Pipeline vs solc vs released solx
Foundry · 25 proj legacy -13.0% -0.0%
Foundry · 25 proj viaIR -1.6% +0.0%

Suites run the release solx binary. Foundry/Hardhat gas jitters run-to-run (fuzz/invariant tests, CREATE-context deploys), so it never gates.

Base automatically changed from integration-harness-fixes to main July 11, 2026 13:43
The integration workflow posted three link-only PR comments; all analysis
of the XLSX reports happened offline. Render the verdict in-tree instead.

New `solx-benchmark-summary` binary + `render_summary` in the converter
compute, from the merged `Benchmark` data model (no XLSX round-trip):

- correctness gate — bytecode size (deterministic, all suites) + solx-tester
  gas (deterministic REVM); foundry/hardhat gas is fuzz-noisy and excluded,
  with top movers listed when output actually changes;
- new build/test failures vs main;
- a non-gating compile-time tripwire: per-suite legacy/viaIR aggregates,
  highlighted at >=5%, with project outliers listed at >=15% (cap 5, +N more);
- full-matrix bytecode size vs solc / released solx.

solx-dev now writes `{foundry,hardhat}-benchmark.json` next to each XLSX and
the workflow emits `solx-tester-benchmark.json`, so the summary reads the
native model directly. The three link comments collapse into one
`integration-test-summary` comment (reusing mshick/add-pr-comment via
message-path) that also lands in the step summary.
@nebasuke nebasuke force-pushed the integration-report-summary branch from e3f3244 to 8ffc4d1 Compare July 11, 2026 18:51
Redesign the integration summary so a reviewer can call pass/fail from
the comment alone, matching the offline review procedure:

- Failures now compare PR vs main per paired run: only regressions
  alarm, pre-existing baselines are stated, and the regressed projects
  are listed inline. Previously the absolute PR-side counts tripped the
  verdict, turning every Foundry run red on its long-standing failures.
- The single conflated headline becomes three independent verdicts:
  output invariance, new failures, harness health. A size change now
  informs (⚠️, movers inline, never folded) rather than alarms — only
  new failures and errored suites are ❌.
- Internal vocabulary no longer leaks: report links are named
  (foundry.xlsx), baselines read "vs solc / vs released solx" instead
  of `00.solc`, mode keys are humanized (E → EVMLA, Y → Yul), and every
  percentage states its direction (positive = PR larger / PR slower).
- Non-gated gas shows its jitter magnitude; absent metrics read "not
  collected"; compile-time gains a per-project median; the footer notes
  the suites run the release binary.
@nebasuke nebasuke force-pushed the integration-report-summary branch from 8ffc4d1 to 27880aa Compare July 11, 2026 22:11
nebasuke added 3 commits July 13, 2026 08:08
…uietly

Review fixes to the summary renderer and binary:

- A present-but-corrupt suite JSON no longer aborts the whole summary:
  the suite renders as errored and the healthy suites still post.
- Benchmark data that matches no recognized toolchain naming is a loud
  "Harness error" verdict + row, and zero collected comparisons render
  as "⚪ No output data" — never a green checkmark over empty data.
- PR runs without a main counterpart are surfaced as unbaselined
  (verdict line + cell marker) instead of counting every pre-existing
  failure as a regression against an imaginary zero baseline.
- Baseline comparisons (vs solc / vs released solx) sum only contracts
  built by both toolchains, so a baseline that failed some builds is
  excluded from the percentage rather than skewing it.
- Report links and "+N more" pointers use the real artifact file names
  (foundry-report.xlsx), passed in by the binary instead of fabricated
  from the display label.
- median() averages the two middle elements for even-length input —
  previously n=2 reported the maximum as the median.
- Compile-time table columns come from the pipelines present in the
  data instead of a hardcoded legacy/viaIR list, so a new codegen shows
  up instead of silently vanishing from the tripwire.
- Each suite's summary flag is passed only when its step actually ran,
  so suites skipped by an earlier hard failure are omitted instead of
  publicly reported as "Suite errored".
- The solx-tester converter runs off the candidate benchmark alone when
  the main-side baseline is missing: the summary then shows the PR data
  as unbaselined instead of a bogus "suite errored" (the XLSX comparison
  still requires both sides).
- Dropped continue-on-error from the summary generation step: losing
  the report must fail the job visibly, not skip the comment silently.
The verbatim-duplicated emit blocks in the foundry and hardhat runners
collapse into one helper next to toolchain_name; it pretty-prints and
path-annotates write errors, matching the converter's own JSON output
for the solx-tester suite.
@nebasuke

nebasuke commented Jul 13, 2026

Copy link
Copy Markdown
Member Author

Currently there is a small gas change. I've deliberately NOT rebased, to show how it looks in the case of a change.

Claude Summary

The gas diff is a stale-base artifact, not a change from this PR

The integration summary flags structs/delete_struct.sol [Yul MzB3 0.8.34] 85,899 → 85,902 (+3 gas). It's a base-drift artifact — nothing in this PR moves compiler output:

  1. This PR can't change codegen. The diff only touches reporting/harness code — integration-tests.yaml, solx-benchmark-converter/*, and solx-dev/src/test/*. No compiler crate, no solx-solidity submodule, no LLVM. Both sides compile identical bytecode.

  2. The branch is 12 commits behind main, including a revm major bump (fix(deps): update rust crate revm to v41 #541, revm 40 → 41):

    revm pin Cargo.lock
    merge-base (fa8cf78d) 40.0 40.0.3
    main tip 41.0 41.0.0
  3. solx-tester measures gas by executing bytecode through revm (gas.tx_gas_used()). The harness builds the tester on each side, so the PR side runs the bytecode under revm 40.0.3 and the main side runs the same bytecode under 41.0.0. The +3 is a revm 40→41 gas-accounting change (delete_struct hits SSTORE/refund paths), applied to one side only because the branch predates the bump.

I also checked the solx-solidity submodule move (#530) — that's just a #include <cstdint> GCC 16 build fix, not codegen, so it's ruled out.

@nebasuke nebasuke marked this pull request as ready for review July 13, 2026 12:57
@nebasuke nebasuke requested a review from a team July 13, 2026 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci:integration Trigger integration tests workflow on PR ci:integration-benchmark-full Run the full benchmark matrix (all compilers + comparisons) in integration tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant