Skip to content

Prepare isolated integration harness for CI smoke - #213

Open
Santiagocetran wants to merge 2 commits into
InfiniteZeroFoundation:developfrom
Santiagocetran:ci/smoke-harness-foundation
Open

Santiagocetran wants to merge 2 commits into
InfiniteZeroFoundation:developfrom
Santiagocetran:ci/smoke-harness-foundation

Conversation

@Santiagocetran

@Santiagocetran Santiagocetran commented Oct 2, 2026 •

Copy link
Copy Markdown
Collaborator

The integration harness uses obsolete demo-wallet commands, and service startup failures can leave processes running or lose the evidence needed to diagnose a failure. This PR updates demo bootstrap and adds an opt-in isolated harness mode as preparation for integration coverage in CI.

Changes:

  • Validate and reuse the checkout's public Anvil demo accounts without changing the file. If absent, generate the two required accounts and remove only that generated file afterward. Use the current demo-wallet CLI commands.
  • Give isolated runs explicit scratch and results directories, local endpoints, and a filtered environment.
  • Track only services launched by the harness, check readiness, and clean them up even when startup fails or is interrupted. Keep service logs outside disposable state.
  • Document the mode and cover bootstrap, account-file preservation, ownership, readiness, environment filtering, and cleanup with 57 focused tests.

Validation:

  • Python unit suite: 442 passed, with integration tests excluded.
  • Focused harness tests: 57 passed.
  • Local integration validation: 2 tests passed using the real harness in a disposable checkout. The existing wallet-reconnection test passed after Hardhat/Foundry compilation, Anvil and offline Kubo startup, and CLI demo bootstrap. A separate validation-only test confirmed chain ID 1337, funded demo accounts, and an IPFS upload/download byte comparison.
  • After the successful run, both service ports were released, scratch state was removed, the checked-in accounts file was preserved, and external logs remained. A deliberate invalid-account run also failed as expected and cleaned up both live services while retaining logs and preserving its input.
  • Documentation: 258 relative links checked successfully; diff whitespace checks passed.

The local Python environment reused installed dependencies and supplied the declared PyNaCl dependency separately. The additional live service-check test was used only in the disposable validation checkout and is not included in this PR.

Scope:
This PR prepares the integration test harness for future CI coverage. Existing GitHub Actions workflows remain unchanged. Local validation covers bootstrap, live blockchain/IPFS use and cleanup; it does not cover platform deployment, the full integration lifecycle suite, a Docker runner, or a hosted integration job.

@Santiagocetran
Santiagocetran marked this pull request as ready for review October 2, 2026 21:56
@umeradl

umeradl commented Oct 3, 2026

Copy link
Copy Markdown
Member

Reviewed against develop in an isolated worktree at PR head 2af68f3 (2 commits, merge-base 420d48d). The branch applies cleanly on top of current develop tip 4ec42d4: git merge-tree --write-tree exits 0 locally, and GitHub reports mergeable: MERGEABLE, mergeStateStatus: CLEAN. I ran the actual harness code against real Anvil/Kubo processes as well as the PR's own unit tests.

Claimed: focused harness tests — 57 passed.

Verified — exact match: python -m pytest tests/test_integration_demo.py tests/test_integration_services.py tests/test_integration_harness.py -q → 57 passed in 1.31s.

Claimed: Python unit suite — 442 passed with integration excluded.

Verified — exact match, and still green after merging into current develop: pytest -m "not integration" at PR head → 442 passed, 133 deselected. Trial-merged into origin/develop (4ec42d4) in a scratch worktree → 446 passed, 133 deselected (the +4 are tests/test_auditor_commit_hash.py from PR No. 212). GitHub CI on the PR head: Solidity / Python / Docs / CI OK all pass.

Claimed: the tests cover ownership, cleanup, account preservation and readiness — i.e. they would catch a regression.

Verified — break-then-fix, 6/6 regressions caught: I removed each guarded mechanism one at a time, ran the 57 focused tests, then restored the file:

Removed mechanism Result
resources.callback(chain.close) in managed_services (chain left running when IPFS init fails) 2 failed
SIGKILL escalation in OwnedService.close 11 failed
symlink refusal in prepare_demo_accounts 1 failed
if generated_accounts: → unconditional unlink (deletes checked-in accounts.json) 2 failed
private-key ↔ address check in prepare_demo_accounts 1 failed
_check_port before spawning 1 failed

After restoring: 57 passed, git status clean.

Claimed: occupied ports are rejected without terminating their processes; only newly started process groups are stopped; logs are retained; ports are released.

Verified — live, beyond the mocked tests: I drove tests/dincli/services.py directly against real processes on this machine:

  1. A pre-existing, unrelated anvil was already listening on 127.0.0.1:8545. start_service(..., port=8545) raised Loopback port 8545 is unavailable, created no log, and the pre-existing anvil PID was unchanged before and after.
  2. A real anvil with the harness's isolated flags on port 8546: rpc_ready passed (chain ID 1337), and os.getpgid(pid) == pid, so the service runs in its own process group.
  3. A real ipfs daemon --offline (Kubo 0.42.0) on a fresh IPFS_PATH, using the harness's exact init / Bootstrap [] / Discovery.MDNS.Enabled false sequence: ipfs_ready passed.
  4. After close() on both services: both processes were reaped, ports 8546 and 5001 bound free again, both logs were non-empty and retained, and the unrelated 8545 anvil was still alive.

Claimed: bootstrap uses the current demo-wallet commands.

Verified: every flag that bootstrap_demo passes exists on develop's CLI: configure-demo --mode, and connect-demo-wallet <name> --account/-a --yes/-y. The old bootstrap could not work on develop: register-wallet now refuses to run while demo mode is on, and the old bootstrap had just turned demo mode on. So this change is a fix, not just a refresh. The checked-in dincli/config/accounts.json holds 70 accounts, and accounts 0/1 are the expected Anvil addresses, so prepare_demo_accounts takes the reuse path (returns False, file left untouched) in any normal checkout.

Claimed: 258 relative doc links checked; diff whitespace checks passed.

Verified — exact match: .github/scripts/check_doc_links.py Documentation → checked 258 inline relative link(s), all resolve, at PR head and on the merged tree. git diff --check 420d48d 2af68f3 is clean.

Not independently re-verified:

  • A full DIN_TEST_ISOLATED=1 session (compile → Anvil → Kubo → bootstrap → test_connect_wallet_dinrep). The review machine's port 8545 is held by an unrelated anvil, so the harness would correctly refuse to start. The pieces are covered by the live checks above.
  • The validation-only live test mentioned in the PR body. It isn't included in the PR, so there was nothing to run.

Notes (non-blocking):

  • Later phases still use the old command (as the PR says). test_02_task_contracts.py (3), test_03_registration.py (5) and test_04_gi.py (8) still call system connect-wallet <name>, 16 call sites in total. connect-wallet passes require_demo=False and so rejects the demo wallets that bootstrap now creates. A run past test_01 will fail at the first of these. That matches the PR's stated scope; the call sites need migrating in the follow-up before the smoke job runs beyond test_01.
  • The isolated Anvil flags duplicate foundry/anvil.sh. The isolated branch inlines the same flags (--accounts 70 --balance 10000 --chain-id 1337 --code-size-limit 4294967295 --block-time 2 --mnemonic …) rather than reusing foundry/anvil.sh. Today they match, but a future edit to one will not reach the other. It may be worth a shared constant, or a comment in anvil.sh pointing at the copy.
  • Isolated compiles may re-download compilers every run. In isolated mode, compilation runs with a fresh HOME/XDG_CACHE_HOME under scratch, so Hardhat's and svm's compiler caches start empty on each run. That is fine for correctness, but it needs network access and will cost time in the future CI job. The Docker runner can pre-seed or mount that cache.

Every checkable claim in the PR held up, several of them confirmed against real processes rather than mocks. Everything new is opt-in behind DIN_TEST_ISOLATED=1. On the default (non-isolated) path, the only changes are: din_tmp becomes a generator fixture with the same setup; there is an optional DIN_TEST_RESULTS_DIR, which defaults to the same <DIN_TEST_TMPDIR>/results; and the bootstrap and test_01 wallet commands are corrected. Looks good to merge as-is; the notes above are follow-up items, not blockers.

@umeradl

umeradl commented Oct 3, 2026

Copy link
Copy Markdown
Member

Files changed (9) — as of 2af68f3 (PR head)

Diffed against merge-base 420d48d (develop). develop has moved 3 commits since (9fb6507 PR No. 212 merge, 7785acf docs nit, 4ec42d4 skills), touching 20 files: auditor contracts, foundry tests, dincli/cli/auditor.py, tests/test_auditor_commit_hash.py, contract docs and .claude/skills. None of them overlap with this PR's 9 files. GitHub agrees: mergeable: MERGEABLE, mergeStateStatus: CLEAN. A local git merge-tree --write-tree dry run is clean (exit 0).

tests/dincli/services.py

Field Value
Change New
Lines +154/-0
Diff (what exactly is in this PR) OwnedService (idempotent close(): SIGTERM the process group → wait 5s → SIGKILL the group → reap → close log; also a context manager). _check_port (bind-probe on 127.0.0.1). start_service (port check → Popen(start_new_session=True) with stdout/stderr to a log → readiness loop with deadline and child-exit checks on both sides of the probe → on any BaseException, close what was started and re-raise; KeyboardInterrupt/SystemExit are re-raised unwrapped). rpc_ready (strict JSON-RPC eth_chainId == 1337). ipfs_ready (non-empty Version).
Functionality — how & why How: each service is spawned as its own session leader, so its PID is also its process-group ID. Cleanup signals only that group, never matching processes by name. A probe that returns True after the child exited, or after the deadline, still counts as a failure. Why: the existing manual path kills by name (pkill -f anvil) and can leave orphans when startup fails. A CI harness has to own exactly what it started, leave anything already on the host untouched, and keep the log when startup fails. Verified live: a pre-existing anvil on 8545 was refused and left alive; a real anvil (8546) and an offline Kubo (5001) were started, reaped, ports freed, logs retained.
Diff vs current develop HEAD None — new file
Recommended merge proposal Merge as-is.
Actual merge proposal Soon
Pending proposal None
Local merge conflict No
GitHub merge conflict No

tests/dincli/demo.py

Field Value
Change New
Lines +79/-0
Diff (what exactly is in this PR) generate_demo_accounts (derives Anvil accounts 0/1 from the public mnemonic, asserts the expected addresses, writes with open("x") and refuses existing files and symlinks). prepare_demo_accounts (refuses a symlink; generates the file if it is missing and returns True = caller owns it; otherwise validates accounts 0/1 address and private key and returns False, leaving the file untouched). bootstrap_demo (init → configure-demo --mode yes → configure-network local → connect-demo-wallet dinrep/modelowner --account 0/1 --yes).
Functionality — how & why How: the conftest's bootstrap fixture calls prepare_demo_accounts only in isolated mode, then bootstrap_demo(run), and unlinks accounts.json only if this run created it. Why: the old bootstrap called register-wallet --account N, which develop now refuses while demo mode is on (the old bootstrap had just enabled it). connect-demo-wallet is the supported path, and it reads the packaged dincli/config/accounts.json. The ownership flag keeps the tracked file (70 accounts) from being deleted. Break-then-fix: forcing the unlink fails 2 tests.
Diff vs current develop HEAD None — new file
Recommended merge proposal Merge as-is.
Actual merge proposal Soon
Pending proposal None
Local merge conflict No
GitHub merge conflict No

tests/dincli/conftest.py

Field Value
Change Modified
Lines +88/-13
Diff (what exactly is in this PR) Adds an isolated branch to din_tmp (refuses checkout .env* other than .env.example; scratch created with exist_ok=False; removed in finally), to managed_services (owned Anvil/Hardhat + fresh offline Kubo via ExitStack, logs in RESULTS_DIR), to din_env (_isolated_env: allow-listed local-only env), to workdir (no .env copy), and to compile calls (env=_isolated_env(...)). din_tmp becomes a generator. bootstrap now delegates to demo.py and cleans up only a generated accounts file.
Functionality — how & why How: everything new is gated on ISOLATED_MODE. In the default path, the services, .env copy and wallet layout are unchanged except for the corrected bootstrap commands. results_dir reads RESULTS_DIR, which defaults to the same DIN_TEMP/results. Why: a CI smoke job must not read a developer's dotenv credentials, reuse a running chain or IPFS repo, or lose logs when scratch is wiped. The isolated env drops host variables; the unit tests confirm an injected PINATA_JWT doesn't reach the subprocess. Note (non-blocking): the isolated Anvil flags duplicate foundry/anvil.sh, and isolated compiles start with empty compiler caches. Both are covered in the verification comment.
Diff vs current develop HEAD None — untouched by develop since merge-base
Recommended merge proposal Merge as-is. The anvil-flag duplication is a follow-up option, not a blocker.
Actual merge proposal Soon
Pending proposal Optional follow-up: single source of truth for the Anvil flags (foundry/anvil.sh vs the isolated inline command).
Local merge conflict No
GitHub merge conflict No

tests/dincli/constants.py

Field Value
Change Modified
Lines +22/-2
Diff (what exactly is in this PR) ISOLATED_MODE (DIN_TEST_ISOLATED == "1"); skips the repo-root .env load, ~/my_venvs Python and nvm npx fallbacks when isolated; new ANVIL_BIN; new RESULTS_DIR (DIN_TEST_RESULTS_DIR or DIN_TEMP/results). When isolated, raises at import time unless both dirs are set via the real environment, both are absolute, and results is not inside scratch.
Functionality — how & why How: these values are resolved once at import time and read by the conftest. The guards run before any fixture, so a misconfigured isolated run fails before touching the filesystem. Why: if results sat inside scratch, the scratch cleanup would delete the evidence. Host-specific fallbacks would make the isolated run depend on the developer's machine layout.
Diff vs current develop HEAD None — untouched by develop since merge-base
Recommended merge proposal Merge as-is.
Actual merge proposal Soon
Pending proposal None
Local merge conflict No
GitHub merge conflict No

tests/dincli/test_01_platform.py

Field Value
Change Modified
Lines +2/-2
Diff (what exactly is in this PR) test_connect_wallet_dinrep calls system connect-demo-wallet dinrep instead of system connect-wallet dinrep.
Functionality — how & why How: reconnects the demo wallet that bootstrap created. Why: connect-wallet passes require_demo=False and rejects demo wallets. test_02–test_04 still have 16 connect-wallet call sites (3 / 5 / 8) that need the same migration. The PR states this is out of scope.
Diff vs current develop HEAD None — untouched by develop since merge-base
Recommended merge proposal Merge as-is.
Actual merge proposal Soon
Pending proposal Follow-up: migrate the 16 connect-wallet call sites in test_02–test_04 before the smoke job runs past test_01.
Local merge conflict No
GitHub merge conflict No

tests/test_integration_demo.py, tests/test_integration_harness.py, tests/test_integration_services.py

Field Value
Change New (3 files)
Lines +125/-0, +216/-0, +214/-0
Diff (what exactly is in this PR) 57 unmarked unit tests (so they run in CI's not integration job). They cover account generation/validation/preservation, bootstrap command order, fixture teardown ordering, failure cleanup, env filtering, scratch/results handling, readiness-probe parsing, and one real child-process reap. Services, subprocess.run and requests.post are stubbed to raise if called unexpectedly.
Functionality — how & why How: imports tests.dincli.conftest with isolated env vars, monkeypatches module paths into tmp_path, and drives fixtures through __wrapped__. Why: this lets CI exercise the harness's failure paths without Anvil, IPFS or compilers. Break-then-fix confirmed that 6 of 6 removed mechanisms are caught. 57 passed locally; 446 passed in the not integration suite on the trial merge with current develop.
Diff vs current develop HEAD None — new files
Recommended merge proposal Merge as-is.
Actual merge proposal Soon
Pending proposal None
Local merge conflict No
GitHub merge conflict No

Documentation/technical/testing/dincli-testing-guide.md

Field Value
Change Modified
Lines +46/-3
Diff (what exactly is in this PR) Quick-start corrected: it no longer calls the harness "self-contained"; it lists the .env + packaged accounts.json prerequisites. New "CI smoke groundwork" section: isolated-mode requirements, account-file behaviour, the focused-test command, and scope limits (no Docker runner or CI job yet).
Functionality — how & why How: documentation only. Why: it keeps Documentation/technical/ describing what exists on develop, including what isolated mode does not guarantee. Link check: 258 relative links, all resolve.
Diff vs current develop HEAD None — untouched by develop since merge-base
Recommended merge proposal Merge as-is.
Actual merge proposal Soon
Pending proposal None
Local merge conflict No
GitHub merge conflict No

Verification

Full detail is in the verification comment above. In short: 57 of 57 focused tests pass. pytest -m "not integration" gives 442 passed at PR head and 446 passed on a trial merge with develop 4ec42d4. Break-then-fix caught 6 of 6 regressions. Live Anvil/Kubo ownership checks passed, including the refusal of an occupied port. Doc links: 258/258 resolve. git diff --check is clean. GitHub CI is green.

Local vs. GitHub agree: both report a clean merge; no file overlaps with the 3 commits develop gained since the merge-base.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants