Prepare isolated integration harness for CI smoke - #213
Santiagocetran wants to merge 2 commits into
Conversation
|
Reviewed against Claimed: focused harness tests — 57 passed. Verified — exact match: Claimed: Python unit suite — 442 passed with integration excluded. Verified — exact match, and still green after merging into current 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:
After restoring: 57 passed, 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
Claimed: bootstrap uses the current demo-wallet commands. Verified: every flag that Claimed: 258 relative doc links checked; diff whitespace checks passed. Verified — exact match: Not independently re-verified:
Notes (non-blocking):
Every checkable claim in the PR held up, several of them confirmed against real processes rather than mocks. Everything new is opt-in behind |
Files changed (9) — as of
|
| 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.
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:
Validation:
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.