tests: assert on the mesh with real components, not on node logs - #460
Merged
Merged
Conversation
Integration tests learned a node's peer ID and listen address by asking its debug endpoints and decoding the JSON into structs kept in the test package, a copy of the node's unexported types that would drift with them. The information was never the node's to give: the test picks the API and libp2p ports, and the peer ID follows from the key in the node's store, which the test can generate with the same node.GetOrGenerateKey the binary uses. launchNode does that. It creates the key, closes the store again so the node can take the bbolt lock, appends the chosen --bind-addr and --listen, and hands back a backgroundNode whose apiAddr, peerID and p2pAddr are plain fields. waitForAPI polls /healthz, which the node only serves once Start succeeded, and fails with the log if the process exits first. The seventeen hand-rolled exec.Command blocks for nodes go, and so do the tests' own JSON mirrors of internal types.
The node lifecycle tests read the node's stdout for sentences: "Using
stored identity.", "SAM Node Online", "Successfully reconnected to
router via HTTP fallback". A reworded log line failed them while the
behaviour stood, and a node that printed the line and died a moment
later passed them. They also ran against mock routers that handed out
a biscuit to anyone, so the enrollment they exercised was never judged.
Every mesh now is a real control plane and a real router, and what the
tests assert is what those two record:
- /admin/status, decoded into storage.EnrolledNode and
storage.RouterLease, says who enrolled which peer and which router
each peer is connected to. Routers renew their lease every second
in tests so the second is current.
- The node's own store, opened after the node has stopped, says what
it will do on its next start: its biscuit, its control plane, its
router set.
- /healthz answers only after Start succeeded, which needs a router
to have authenticated the node; a node with no identity is known by
the token-less MCP session it accepts.
The mock OIDC provider gains the authorization code grant with PKCE so
`sam-node join` signs in through it and the control plane verifies the
result against the JWKS. The join, restart, mismatch, reset, no-identity
and self-healing tests are rewritten on that, and the failover test
watches the router leases on both control planes instead of the node's
AutoRelay log. The mock router that spoke OIDC goes.
Binaries are now built once per test process. Go caches compilation,
but every go build -o links, and fifty tests linking three binaries
each was a minute of nothing. The Help tests drop from ~2s to ~0.2s.
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the integration tests by introducing a robust helper framework (backgroundNode, launchNode, startControlPlane, and startRouter) to manage background nodes, control planes, and routers. This significantly reduces boilerplate code, removes fragile log-parsing logic, and consolidates test setup across multiple integration test files. Feedback on the changes highlights a violation of Rule 1.5 of the SAM Code Review Style Guide in tests/integration/minimal_helpers_test.go, where raw peer ID strings are compared directly for identity in enrolledNode and routerWith instead of being decoded using peer.Decode first.
Comment on lines
+552
to
+559
| func (s adminStatus) enrolledNode(peerID string) *storage.EnrolledNode { | ||
| for i := range s.EnrolledNodes { | ||
| if s.EnrolledNodes[i].PeerID == peerID { | ||
| return &s.EnrolledNodes[i] | ||
| } | ||
| } | ||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
Comparing raw peer ID strings directly for identity violates the SAM Code Review Style Guide (Rule 1.5). Since s.EnrolledNodes[i].PeerID is decoded from wire JSON, it is a raw string and should be decoded using peer.Decode before comparison to ensure canonical forms are compared correctly.
func (s adminStatus) enrolledNode(peerID string) *storage.EnrolledNode {
target, err := peer.Decode(peerID)
if err != nil {
return nil
}
for i := range s.EnrolledNodes {
p, err := peer.Decode(s.EnrolledNodes[i].PeerID)
if err == nil && p == target {
return &s.EnrolledNodes[i]
}
}
return nil
}References
- Raw string compared for identity. Decode both sides and compare peer.ID values, or compare .String() of two decoded IDs. (link)
Comment on lines
+562
to
+571
| func (s adminStatus) routerWith(peerID string) *storage.RouterLease { | ||
| for i := range s.ActiveRouters { | ||
| for _, p := range s.ActiveRouters[i].ConnectedPeers { | ||
| if p == peerID { | ||
| return &s.ActiveRouters[i] | ||
| } | ||
| } | ||
| } | ||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
Comparing raw peer ID strings directly for identity violates the SAM Code Review Style Guide (Rule 1.5). Since p is retrieved from ConnectedPeers (wire JSON), it is a raw string and should be decoded using peer.Decode before comparison to ensure canonical forms are compared correctly.
func (s adminStatus) routerWith(peerID string) *storage.RouterLease {
target, err := peer.Decode(peerID)
if err != nil {
return nil
}
for i := range s.ActiveRouters {
for _, p := range s.ActiveRouters[i].ConnectedPeers {
pid, err := peer.Decode(p)
if err == nil && pid == target {
return &s.ActiveRouters[i]
}
}
}
return nil
}References
- Raw string compared for identity. Decode both sides and compare peer.ID values, or compare .String() of two decoded IDs. (link)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Integration tests for the node lifecycle (join, restart on a stored identity, control-plane mismatch, reset, no identity, self-healing router lookup, failover) now run against a real
sam-control-planeandsam-routerand assert on what those components record, instead of grepping the node's stdout against mock routers. Also builds each binary once per test process.Why
Follow-up to #456, where
TestSelfHealingHTTPFallbackfailed because a log line moved while the behaviour it guarded was intact. The same pattern was everywhere: tests waited for"Using stored identity.","SAM Node Online","Yielding static relays to AutoRelay","Successfully reconnected to router via HTTP fallback". A reworded log breaks them; a node that prints the line and dies a moment later passes them. And the mock routers minted a biscuit for anyone, so the enrollment those tests exercised was never actually judged.How
Identity is known before the node starts (
node_helpers_test.go). The test picks the API and libp2p ports and generates the node's key in its store with the samenode.GetOrGenerateKeythe binary uses, sopeerIDandp2pAddrare plain fields onbackgroundNode. The tests' JSON mirrors of the node's unexported/debugtypes are gone. 17 hand-rolledexec.Commandblocks collapse intolaunchNode.What the tests observe:
/admin/status, decoded intostorage.EnrolledNode/storage.RouterLease: who enrolled which peer (ClaimsJSON.sub,Role, the issuedBiscuit), and which router each peer is connected to (ConnectedPeers). Test routers renew their lease every 1s so this view is current./healthz, which the node only serves onceStartsucceeded, i.e. once a router authenticated it. A node without identity is recognised by the token-less MCP session it accepts (get_login_instructions).Real login path. The mock OIDC provider gains the authorization-code grant with PKCE, so
sam-node joinprints its auth URL, the test "user" visits it, the provider redirects to the CLI's loopback callback, and the resulting RS256 JWT is verified by the real control plane against the JWKS.startMockRouterWithOIDCis deleted.Failover / self-healing.
TestFailoverUpdatesRelaywatches the node appear in Router A's lease on CP A, then in Router B's lease on CP B.TestSelfHealingHTTPFallbackenrolls through a real router, kills it, starts a new one (CP--lease-duration 2sso the dead one expires), restarts the node with no--control-plane, and checks the node is connected to the new router and has it stored for next time.Build cache.
buildBinarybuilt into a fresht.TempDir()at 50 call sites; Go caches compilation but everygo build -olinks again (1.5–3s per binary). Now once per test process viasync.Map, cleaned inTestMain.TestSamRouterHelp2.15s → 0.17s.Testing
go vet ./tests/integration/(the package isn't compiled bygo build ./...)go test ./tests/integration/ -count=1— full package passes locallyhack/lint.sh— 0 issuesFollow-ups (not in this PR)
/admin/statusserialisesinternal/storagetypes directly; AGENTS.md wantsapi/structs for the operator plane. The tests decode into the storage types today because that is what the wire carries.mesh_wait_for_log "SAM Node Online"and"Using stored identity."intests/e2ehave the same problem;mesh_wait_for_mcp_readyexists to replace the former.