Skip to content

docs: record the local-vs-CI split so local runs stop duplicating CI #899

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lklimek merged 3 commits into from
Jul 18, 2026
Merged

docs: record the local-vs-CI split so local runs stop duplicating CI #899

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c4e82c7
docs: record the local-vs-CI split so local runs stop duplicating CI
lklimek Jul 16, 2026
e909e68
docs: fix CI-policy path filters and drop unsupported workflow_dispat…
lklimek Jul 17, 2026
48e3c98
docs: fix nightly/pinned-toolchain fmt mismatch and other CI-doc gaps
lklimek Jul 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 28 additions & 1 deletion CLAUDE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,34 @@ Test locations:

Driving the actual compiled binary through a real display — for flows that need real navigation, real async/network timing, or visual verification beyond what `kittest` (no display) or `backend-e2e` (no UI) can cover. Read `docs/gui-testing/README.md` before running this kind of test — it has the safety rules (isolated data dir, credential handling, fund-movement caps) and the reusable scenario library under `docs/gui-testing/scenarios/`.

Always run `cargo clippy` and `cargo +nightly fmt` when finalizing your work.
Always run `cargo +nightly fmt` when finalizing your work. For `cargo clippy`, see the scope guidance below.

### Local vs CI — avoid duplicate test runs

CI is the full-suite backstop. Do not reproduce it locally.

On every **non-draft** PR that touches Rust code, and on pushes to `v*-dev`, GitHub Actions runs the complete gate:

@lklimek lklimek Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Low: "complete gate"/"full sweep" wording overlooks intentionally-#[ignore]d unit tests

The default cargo test --all-features --workspace skips every #[ignore] test. Beyond the acknowledged backend-E2E lane, current HEAD also has at least one intentionally-ignored unit test (src/model/secret.rs:492, test_drop_zeroes_full_capacity) that stays manual. "Complete gate" / "full sweep," plus the advice to check for a test's name in the log, reads as too absolute — a test name can appear in the log with ignored status without having actually run.

Suggest: qualify to "the full non-ignored suite," note intentionally-ignored tests remain manual, and recommend checking per-test pass/ignore status (or executed-test count) rather than only searching the log for a name.

🤖 Co-authored by Claudius the Magnificent AI Agent — automated grumpy-review (Claude trio-equivalent + Codex Sol)

@Claudius-Maginificent Claudius-Maginificent Jul 17, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 48e3c980 — qualified to "full non-ignored-test gate," and the log-check guidance now calls out a concrete example (test_drop_zeroes_full_capacity in src/model/secret.rs) and says to confirm pass status, not just name presence.

🤖 Co-authored by Claudius the Magnificent AI Agent


| Workflow | Runs |
|---|---|
| `tests.yml` | `cargo test --all-features --workspace` and `cargo test --doc --all-features --workspace` |
| `clippy.yml` | `cargo fmt --all -- --check` and `cargo clippy --all-features --all-targets -- -D warnings` |

The workflows are path-filtered independently, each on `**/*.rs` (which includes `build.rs`), `**/Cargo.toml`, `Cargo.lock`, `.cargo/config.toml`, and its own workflow file. `tests.yml` additionally watches `tests/backend-e2e/**`, so a documentation-only change under that directory (e.g. `tests/backend-e2e/README.md`) still triggers the test workflow; other documentation-only changes run neither workflow.

Because CI always runs the full sweep, locally you should:

- Run only the **narrowest scope covering your change** — `cargo test <test_name> --all-features`, or `cargo test --test kittest --all-features` for a UI change. Running the whole workspace suite locally only duplicates the run CI is about to do anyway.

@lklimek lklimek Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Low: new "run bare cargo locally" guidance doesn't disambiguate from the safe-cargo.sh mandate

CLAUDE.md's existing "CI: Safe Cargo Wrapper" section mandates scripts/safe-cargo.sh "In GitHub Actions (Claude Code workflow)" instead of bare cargo. This new section's "locally"-framed bare-cargo instructions could plausibly be misread by an AI agent running inside the Claude Code review GitHub Action (itself a form of CI, but neither tests.yml nor clippy.yml) as applying to its own environment. Soft ambiguity, not a confirmed misread — but cheap to close.

Suggest: one clause cross-referencing the safe-cargo.sh rule so the two sections can't be read as conflicting for an agent running inside that workflow.

🤖 Co-authored by Claudius the Magnificent AI Agent — automated grumpy-review (Claude trio-equivalent + Codex Sol)

@Claudius-Maginificent Claudius-Maginificent Jul 17, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 48e3c980 — added a cross-reference clause pointing an agent running inside the Claude Code review GitHub Action to the "CI: Safe Cargo Wrapper" section instead.

🤖 Co-authored by Claudius the Magnificent AI Agent

- Always run `cargo +nightly fmt --all` before committing. It needs no compile, and `clippy.yml` fails the build on unformatted code.

@lklimek lklimek Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium: documented local formatter doesn't match CI's formatter toolchain

This mandates cargo +nightly fmt --all, while clippy.yml installs Rust 1.92 (per rust-toolchain.toml) and runs cargo fmt --all -- --check on that toolchain. Nightly and 1.92 rustfmt can disagree on formatting, so the locally-mandated command isn't actually the one CI validates — the same local/CI mismatch this section exists to close. (Predates this PR — it echoes the repo's existing top-level "Always run cargo +nightly fmt" instruction — but this PR is the place restating it, so worth fixing here.)

Suggest: cargo fmt --all (honoring rust-toolchain.toml) or explicit cargo +1.92 fmt --all, unless nightly-only rustfmt options are actually in use — if so, say why nightly is required.

🤖 Co-authored by Claudius the Magnificent AI Agent — automated grumpy-review (Claude trio-equivalent + Codex Sol)

@Claudius-Maginificent Claudius-Maginificent Jul 17, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 48e3c980 — switched to plain cargo fmt --all (honors the rust-toolchain.toml 1.92 pin) in both this bullet and the earlier "finalizing your work" sentence, matching what clippy.yml actually checks. Left the pre-existing "CI: Safe Cargo Wrapper" section's +nightly fmt untouched — that's a separate, pre-existing concern outside this docs-only PR's stated scope, but flagging it here in case it's worth its own follow-up.

🤖 Co-authored by Claudius the Magnificent AI Agent

- Run `cargo clippy` locally only for the scope you touched (`-p <crate>`), or when you expect lint fallout. CI owns the `--all-features --all-targets` sweep.

@lklimek lklimek Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Low: -p <crate> clippy-scoping advice doesn't apply to this repo

This repo has no [workspace] — root Cargo.toml is a single [package] name = "dash-evo-tool". -p <crate> has exactly one possible value here (the whole crate), so it provides zero scoping benefit over omitting -p entirely. Reads like carried-over multi-crate-workspace boilerplate.

Suggest: replace with something that actually narrows a clippy run in a single-package repo (e.g. --bin <name>), or drop the parenthetical if no real narrowing exists.

🤖 Co-authored by Claudius the Magnificent AI Agent — automated grumpy-review (Claude trio-equivalent + Codex Sol)

@Claudius-Maginificent Claudius-Maginificent Jul 17, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 48e3c980 — dropped the -p <crate> parenthetical (confirmed: no [workspace], single package) and replaced with target-based scoping guidance (--bin dash-evo-tool).

🤖 Co-authored by Claudius the Magnificent AI Agent

- After pushing, watch the PR checks instead of re-running the suite locally.

Two gaps where CI will **not** cover you:

- **Draft PRs run no automatic CI.** Both workflows are gated on `github.event.pull_request.draft != true`, so a draft PR's `pull_request` runs are suppressed. Neither workflow declares a `workflow_dispatch` trigger, so there is no way to run them by hand against a draft branch — mark the PR ready for review (`ready_for_review` triggers the full run) to get CI coverage.
- **Backend E2E tests are not in CI.** The step is commented out in `tests.yml`, and the tests are `#[ignore]`d. If a change touches backend behaviour that only `tests/backend-e2e/` covers, run those locally; CI will not.

A green CI run is only meaningful if it actually executed your tests. `cargo test <filter>` exits 0 and prints `test result: ok` even when the filter matches nothing — when checking a run, confirm your new test names appear in the log rather than trusting the exit code.

### User stories catalog

Expand Down