Repository navigation
ci: consolidate PR CI, run every test target, build the shipped binary - #269
Conversation
MaidSafe-Hermes
left a comment
MaidSafe-Hermes
left a comment
There was a problem hiding this comment.
Reviewing WithAutonomi/ant-node@a8e4cb123bf096ccd4e9405f90738a63baf8c480. The review panel found one actionable CI-control defect; I confirmed the Bash behavior with a failing-pipeline reproduction.
The integration loop exits on its first failing target. With shell: bash, GitHub Actions runs Bash with -e -o pipefail; therefore, when this cargo test | tee pipeline fails, errexit terminates the step before the next status=${PIPESTATUS[0]} assignment. Later targets then do not run, despite the comment saying they all run after a failure, and Linux-only test steps are skipped. I reproduced this with bash --noprofile --norc -e -o pipefail: false | tee exits 1 before the status assignment.
Minimum fix: handle the pipeline in an if/else (or otherwise disable errexit for that command), capture the cargo and tee statuses, then continue the loop and fail at the end.
Panel coverage: 2/2 seats completed (sol-1 gpt-6.1-sol xhigh: CHANGES_REQUIRED; glm-1 z-ai/glm-5.3: UNCERTAIN). The seats differed on this shell behavior; I verified it locally as above.
CI at review: Ubuntu test job passed; macOS and Windows test jobs were still running. Other required jobs observed passed. Security Audit failed on the documented advisory and is outside the gate. PR #268 remains open, and this PR is draft.
|
|
||
| filesystems: | ||
| name: Storage on ${{ matrix.fs }} | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
The integration loop exits on its first failing target. With shell: bash, GitHub Actions runs Bash with -e -o pipefail; therefore, when this cargo test | tee pipeline fails, errexit terminates the step before the next status=${PIPESTATUS[0]} assignment. Later targets then do not run, despite the comment saying they all run after a failure, and Linux-only test steps are skipped. I reproduced this with bash --noprofile --norc -e -o pipefail: false | tee exits 1 before the status assignment.
Minimum fix: handle the pipeline in an if/else (or otherwise disable errexit for that command), capture the cargo and tee statuses, then continue the loop and fail at the end.
… every cargo run build.rs declared rerun-if-changed for .git/HEAD, .git/refs/heads and .git/packed-refs. Cargo treats a path that doesn't exist as changed on every build, so whenever one was missing the script reran and ant-node was recompiled from scratch on each cargo invocation. That happens in a git worktree (.git is a file) and in CI checkouts: on PR #269 every `cargo test --test X` spent ~2m20s recompiling ant-node for a test binary the previous step had already built, 10 times per job; the old per-target CI steps paid the same cost. The script now asks git for the real git dir and common dir, and only watches the paths that exist. ANT_GIT_COMMIT is still refreshed when HEAD or a branch ref moves. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The PR job set was 17 jobs (2 macOS, 2 Windows), about 200 job-minutes, and still left tests unrun and the shipped build untested. Now 12 jobs (1 macOS, 1 Windows) with more coverage: - Quick checks: fmt, clippy, the no-default-features lib check and docs in one job (were Format Check, Clippy, Documentation, plus a step of Test (no logging)). - Test (3 OS): lib and bin unit tests, then every integration target read from `cargo metadata`, each required to pass at least one test. This picks up the targets CI never ran (pointer_convergence, poc_d1_bounded_queues, poc_price_floor_live), the three webrtc_direct_devnet tests gated on test-utils that no step could reach, and the bin unit tests, and a new tests/*.rs runs without editing ci.yml. Linux also runs the doc tests (never run before), the ignored five-node devnet test (was its own job) and the lib tests without default features (was Test (no logging)). - Build release: replaces Build x3, which built --release --no-default-features, a configuration nobody ships. It now builds the linux-x64 binary the way release.yml does (default features, musl, cross), and the no-failpoint check runs on that binary. - CI Gate: needs every job but Security Audit and fails unless all succeeded; the one check to require (V2-1801). - Security Audit is unchanged and outside the gate until V2-1800. enforced_floor_rejects_cheapest_of_k_real_settlement is marked ignored with its reason: it has failed since the close-group floor rework (V2-1803) and would otherwise red every PR now that the target runs. Closes V2-1811 Closes V2-1804 Closes V2-1805 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Step timings from the last 15 green runs: every integration step except e2e took 2-5 minutes (ubuntu ~2, macOS ~3, windows ~4.5) for tests that finish in under a second. The time was each step building and linking its own test binary in turn. One `cargo test --test '*' --no-run` builds them all in parallel, and the per-target loop then only runs them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…n Linux only - GitHub runs `shell: bash` steps with -e, so the loop stopped at the first failing target instead of running the rest and listing them all. Turn -e off for the loop; the step still fails at the end. - storage_scale's gates compare the store scan against timed directory walks measured on Linux, and the old ci.yml only ran it there. Running it on Windows failed opening_a_large_store_stays_quick. Skip it off Linux, as before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
70a8359 to
ff61461
Compare
MaidSafe-Hermes
left a comment
MaidSafe-Hermes
left a comment
There was a problem hiding this comment.
APPROVE — the previous CI-loop finding is fixed. I verified that set +e -uo pipefail disables GitHub Actions Bash errexit, and a failing pipeline is recorded while the next target still runs. The new Linux-only storage_scale guard preserves its prior platform scope.
CI Gate and all required jobs passed for this head, including Ubuntu, macOS and Windows tests. The Security Audit failure is outside CI Gate and is not a code-review blocker; track it separately for merge readiness. The poc_price_floor_live enforcement case remains ignored under V2-1803 as disclosed.
Coverage: 2/2 seats reviewed (sol-1 gpt-6.1-sol xhigh; glm-1 z-ai/glm-5.3); both seats agree (two model families).
… every cargo run build.rs declared rerun-if-changed for .git/HEAD, .git/refs/heads and .git/packed-refs. Cargo treats a path that doesn't exist as changed on every build, so whenever one was missing the script reran and ant-node was recompiled from scratch on each cargo invocation. That happens in a git worktree (.git is a file) and in CI checkouts: on PR WithAutonomi#269 every `cargo test --test X` spent ~2m20s recompiling ant-node for a test binary the previous step had already built, and the per-target steps in today's ci.yml pay the same cost (2-5 min each for sub-second tests). The script now asks git for the real git dir and common dir, and only watches the paths that exist. ANT_GIT_COMMIT is still refreshed when HEAD or a branch ref moves. Closes V2-1828 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Main merged ADR-0020's chunk get-or-closer read (WithAutonomi#249: a node answers `GetOrCloserRequest` with the chunk or its closest peers, and says so in its user agent), the consolidated CI (WithAutonomi#269), and build.rs watching only git paths that exist (WithAutonomi#270). Conflicts: - Cargo.toml: main pinned saorsa-core and ant-protocol at ADR-0020 revisions in a second `[patch.crates-io]` table. This branch's table already patches both to their feat/peer-swarm branches, which have merged their mains and so carry those revisions, so the one table stays, with its comment saying so. Cargo.lock moves both to those branches' new heads. - src/node.rs: both sides added an import from `ant_protocol`. Beyond the conflicts: - ant-protocol's main put get-or-closer at wire discriminants 14 and 15, where this branch's swarm variants had been; they now follow it, at 16 to 21. The node tells a swarm request from its header alone (`is_swarm_request_bytes`), by those discriminants, so its constants move from 14, 16 and 18 to 16, 18 and 20. Left as they were, every get-or-closer request would have gone to the swarm rendezvous and stayed unanswered. The test that tells swarm requests apart now also checks that a get-or-closer request or response is not taken for one, and fails with the old constants. - ADR-0026 gives the new discriminants. `cargo test --lib` 1255 passed, 1194 without default features (its tests also build with `-D warnings`); fmt, clippy with all targets and features and lib-only on Rust 1.99, `cargo check --lib --no-default-features --locked` and `RUSTDOCFLAGS="-D warnings" cargo doc --no-deps`: clean.
Brings the branch up to main f37429a, which adds pressure-gated retention (WithAutonomi#271), get-or-closer chunk GETs (WithAutonomi#249), the WebRTC UPnP mapping (WithAutonomi#264) and listener summary (WithAutonomi#278), and the consolidated PR CI (WithAutonomi#269). The only conflict was the ADR index: ADR-0024 now sits before main's ADR-0027 and ADR-0028. None of main's changes touch the fetch scheduler.
Takes the per-source scheduler branch at f568f91, which merged upstream main f37429a, so this branch carries main's pressure-gated retention (WithAutonomi#271), get-or-closer chunk GETs (WithAutonomi#249), WebRTC changes (WithAutonomi#264, WithAutonomi#278) and the consolidated PR CI (WithAutonomi#269) through it. The only conflict was the ADR index: ADR-0025 now sits before main's ADR-0027 and ADR-0028. The resulting tree equals a direct merge of this branch with main apart from that index.
Linear issue
Closes V2-1811
Closes V2-1804
Closes V2-1805
Summary
From the ant-node CI audit (V2-1799). PR CI was 17 jobs (2 macOS, 2 Windows), about 200 job-minutes, and it still:
pointer_convergence,poc_d1_bounded_queuesandpoc_price_floor_live, threewebrtc_direct_devnettests gated ontest-utilsthat no step could reach, the bin unit tests, and the doc tests (V2-1804)--release --no-default-features, which nobody ships (V2-1805)After this PR: 12 jobs (1 macOS, 1 Windows), and every test target runs.
check --lib --no-default-features,cargo doccargo metadata, run one at a time, each required to pass ≥1 test; Linux also runs doc tests, the ignored five-node devnet test and the no-default-features lib testscross); the no-failpoint check now runs on that binaryWhere every removed job went
check --lib --no-default-featurestest --lib --no-default-features--ignored)--release --no-default-features)--test Xlist (7 of 10 targets)cargo metadataloop (10 of 10 targets)webrtc_direct_devnetwithouttest-utilstest-utils, so its 3 gated tests run tooCoverage deliberately not kept: a release-profile build on macOS and Windows (their debug builds of default features still run), and a release build of the aarch64 targets (neither was built on PRs before).
One test ignored:
poc_price_floor_live::enforced_floor_rejects_cheapest_of_k_real_settlementnow has#[ignore], with its reason. It has failed deterministically since the close-group floor rework, and fixing it is V2-1803. Without the ignore, running every target would red every PR. The file's other two tests run.build.rs: the fix for the crate recompiling on every cargo invocation, found while measuring this PR, landed separately as #270 (V2-1828). That's why every target can afford to run as its owncargo test --test Xhere.After the first review (
70a8359, nowff61461):shell: bashwith-e, so the loop stopped at the first failing target. It now turns-eoff, runs every target and fails at the end with the full list.storage_scaleis Linux-only again, as in the old ci.yml. Its timing gates were measured on Linux, and it failed on Windows.Measured on
70a8359(before the rebase; same ci.yml) against main's medians: Test ubuntu 24 min (was 37), macOS 18 min (was 45). Windows is still to come.Not changed:
pr-checks.yml,adr-governance.yml,release.yml, and theon:triggers (still PR-only, as V2-1353 set).Open question (V2-1811): most of the remaining cost is e2e on macOS and Windows (45 and 57 min). If we drop e2e from one of them we'd get under 100 job-minutes. That's a call for Nic and Chris; this PR keeps today's coverage.
Risk tier
Compatibility
Semver impact
Test evidence
pointer_convergence17/17,poc_d1_bounded_queues7/7, the 3 gatedwebrtc_direct_devnettests 3/3, bins 15/15, and the doc test (1 pass, 2 rustdoc-ignored).poc_price_floor_livehas 2 passes and 1 deterministic failure, now ignored (V2-1803).bash -n. Thecargo metadata+ jq filter lists all 10 integration targets locally. The gate's jq expression returns false when one need isskippedand true when all succeeded.New dependency
none (no new crates or actions;
crossis installed the way release.yml installs it)ADR
n/a
Mitigation / rollback
Revert the commit. #268 (concurrency + timeouts) stands on its own.
🤖 Generated with Claude Code