Skip to content

test(app): add smoke test#541

Draft
iamquang95 wants to merge 2 commits into
mainfrom
feat/smoke-test
Draft

test(app): add smoke test#541
iamquang95 wants to merge 2 commits into
mainfrom
feat/smoke-test

Conversation

@iamquang95

Copy link
Copy Markdown
Collaborator

Add docker-compose smoke tests (charon parity + pluto/charon node mixing)

Ports charon v1.7.1's testutil/compose smoke-test framework to test-infra/compose, adding a per-node implementation axis so clusters can mix charon and pluto nodes. Each scenario builds a real cluster (keygen → run with simnet mocks → 60s under Prometheus alert gating); any firing alert fails the test.

Scenarios cover charon's full matrix plus pluto variants (all_pluto, mixed_2_charon_2_pluto, pluto_dkg, gated behind -pluto-run). all_pluto passes against feat/simnet-run: 4 nodes reach QBFT consensus, threshold-sign, and broadcast duties with zero errors/warnings.

Deviations from charon (each commented):

  • Alert gate matches firing, not active — charon's never fires (upstream no-op bug)
  • Prometheus scrape config generated per cluster size (charon scrapes a non-existent bootnode)
  • Pluto keygen gets flags, not CHARON_* env vars (pluto CLI gap)
  • Path-less relay URL, dkg --shutdown-delay (distroless image), loki/otlp only when monitoring enabled, dropped CI-only sudo chown

Run: PLUTO_REPO= go test ./smoke -v -integration [-pluto-run]

@iamquang95

Copy link
Copy Markdown
Collaborator Author

@claude review this PR

@claude

claude Bot commented Jul 13, 2026

Copy link
Copy Markdown

Claude finished @iamquang95's task in 4m 29s —— View job


Claude PR Review — test(app): add smoke test

I reviewed the full diff (8ddf273..HEAD), reading the ported Go framework against charon v1.7.1's testutil/compose at /home/runner/work/pluto/pluto/charon/testutil/compose. Overall this is a high-quality, unusually well-documented port — every deviation from charon is justified with a comment tying it to a concrete pluto/charon behavior, and the pure-Go logic (alert-rule generation, warmup gating, impl selection) is well covered by internal tests. I have no blocking findings; the notes below are observations and a couple of small gaps.

What's solid

  • getActiveAlerts firing-vs-active fix (alert.go:163-185) is the most important correctness change over charon and is exactly right — charon matches "active", a state Prometheus never emits, so its gate silently passes everything. Pinned by TestGetActiveAlertsFiringOnly.
  • Warmup gating (alert.go:76-119) correctly still fails if a cold-start transient is still firing after the window (inWarmup && startupTransientRules[...]continue, then re-checked once inWarmup is false). TestStartupTransientRulesScoped locks the allowlist to the 3 windowed rules.
  • writeAlertRules broadcast-liveness expression (define.go:545-550) — the or on (job) max by (job) (0 * up) injection so absent series (a node that never broadcasts) still alerts is a genuinely subtle fix and is well tested.
  • Config.Validate running on every write and load (config.go:188, exercised by TestConfigValidateRejectsUnknownImpl) closes the "typo silently selects charon image" hole nicely.
  • Golden coverage maps 1:1 to the 8 TestDockerCompose subtests + TestNewDefaultConfig.

Observations / minor

  1. No CI wiring — smoke tests are manual-only. No workflow under .github/ references test-infra/compose or smoke, and the module is a separate go.mod outside the Rust workspace, so cargo test won't touch it. Given it needs Docker + a source build of both repos, manual-only is reasonable, but please confirm that's intended and consider a nightly/opt-in workflow so the framework doesn't silently rot. (I could not execute go test in this review environment to confirm the unit tests pass — CI/local should verify.)

  2. flagsCommand comma-quoting is only exercised in gated integration tests. The single-quote-on-comma logic (config.go:286-288) matters most for create dkg's --operator-enrs=enr:...,enr:... and withdrawal/fee-recipient addresses, but there's no golden or unit test for the pluto keygen DKG define path — the TestDockerCompose matrix covers lock create pluto keygen but not a pluto define dkg. A small table-test on flagsCommand (comma value → single-quoted; false/empty → omitted; true → bare flag) would guard this cheaply and without Docker. Fix this →

  3. Pluto keygen container gets both flags and CHARON_* env vars (see testdata/TestDockerCompose_lock_create_pluto_keygen_yml.golden). Harmless since pluto ignores the env vars and reads the flags, but the redundancy is slightly confusing — worth a one-line comment in Lock/Define noting the env vars are dead weight for the pluto keygen path (kept only because they share the kvs used to build the flags).

  4. Alert reader only drains after the full AlertTimeout. In Auto (auto.go:131-146), execUp blocks until the context deadline, so the for alert := range alerts loop runs after the timeout — a real alert firing at t≈20s still takes the full 2 min to fail the scenario. This is inherited from charon and fine for a smoke suite, just flagging the wall-clock cost (12 non-pluto scenarios × up to 3 min).

  5. Alert channel (buffer 100) has no concurrent reader during the run (alert.go:56), and non-success status messages (alert.go:93) are not deduped unlike real alerts. Bounded well under 100 for a 2-min run and inherited from charon, so low risk — noting only for completeness.

None of these are blockers. Nice work — the deviation comments in particular make this easy to audit against upstream.
· branch feat/smoke-test

@iamquang95

Copy link
Copy Markdown
Collaborator Author

First, clone this branch and go to compose folder.

cd test-infra/compose
PLUTO_REPO=<your pluto repo want to run smoke test>  go test ./smoke -v -integration -timeout=35m

The 4 pluto scenarios vary two independent things — who generates keys vs who runs the cluster — plus DKG-vs-trusted-dealer:

Scenario Keygen impl Runtime impl Unique coverage
pluto_keygen_create pluto charon pluto artifacts → charon reads them
all_pluto pluto pluto pluto end-to-end, alone
mixed_2_charon_2_pluto charon charon + pluto live cross-impl duties + charon artifacts → pluto reads them
pluto_dkg pluto DKG pluto pluto's distributed keygen ceremony

What each one actually proves

pluto_keygen_create — artifact format, pluto→charon direction. Pluto runs create cluster (writes cluster-lock.json, key shares, deposit data, ENR keys); then charon nodes load and run them.
If pluto's lock format, share encoding, or lock signature deviated, charon rejects at startup or fails lock verification. Note the flip side: the runtime is all charon, so this exercises zero pluto runtime code — it's purely "can charon consume what pluto produced."

all_pluto — most pluto code, weakest proof. Keygen, run, QBFT consensus, parsigex, sigagg, broadcast — all pluto, 4 nodes. It exercises the widest pluto surface, but because every peer is pluto, a bug where pluto disagrees with the spec but agrees with itself passes silently (both sides compute the same wrong thing). Green here means "pluto is internally consistent and live," not "pluto is correct."

mixed_2_charon_2_pluto — the real interop test. Threshold is 3-of-4, so every duty needs ≥3 of the 4 nodes to sign → both charon and pluto must participate in every duty for consensus to decide.
This is the scenario that forced out every real interop bug:

  • signing-domain divergence (fork_schedule endpoint vs spec keys) — charon was rejecting 100% of pluto's partial signatures
  • mock head-root divergence (the Go math/rand port) — cross-impl sync-committee messages
  • QBFT reaching quorum across implementations, partial-sig exchange, aggregation across impls

It also covers the charon→pluto artifact direction (charon does keygen here), so the two artifact directions together are covered by mixed + keygen_create. This is the only scenario that proves pluto and charon can actually run a validator together.

pluto_dkg — the distributed keygen ceremony. The other three use create cluster (centralized/trusted-dealer keygen). This runs pluto's real DKG (frost) p2p ceremony across 4 nodes, then runs the cluster. Validates the ceremony end-to-end.

Gaps worth knowing

  • All simnet, mock beacon + mock VC. No real beacon-node or real-VC path is gated (real VCs can't sync against the v1.7.1 mock).
  • Liveness, not per-duty correctness. "Green" = nodes broadcasting, no error/warn floods, cross-impl consensus decides — it won't catch a duty that produces a wrong-but-non-erroring value.
  • Residual, currently masked in mixed: infosync isn't wired into pluto run (#402B), so mixed excludes the sched warn topic; and there's a rare relay-circuit hang seen once in all_pluto that the deadline bound now surfaces as a ConsensusTimeout rather than hiding.

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.

1 participant