test: raise unit coverage to >=80% for every file and package

Son of Odinrequested to merge
unit-test-expansion into develop

🔎 Visual review of 5d023240: explainer, review flags and prompts to ask an AI about the change

Summary

Raises unit-test coverage so that every non-generated Go file and every package reaches ≥80% statement coverage from its own package's tests. Generated files (Code generated … DO NOT EDIT, *.pb.go, *.pulsar.go, openapi/gen) are out of scope.

Scope Before After Files <80%
Main module (-tags=mocknet, as CI runs it) 71.3% 90.0% 299/743 → 0
bifrost/tss/go-tss (-race) 62.8% 86.4% 19/40 → 0
test/simulation module 22.1% 92.9% 45/51 → 0
Network-variant files (mainnet/stagenet/chainnet/regtest), each under its own tag 2–70% 84–100% 0 (31 have no statements)

  • 1,361 new tests (831 func TestX + 530 gocheck methods) in 249 new *_test.go files.

  • Five existing tests only had timing deflakes. Their assertions are the same; only waits and setup changed so they hold up on loaded CI runners:

    • coinbase_test.go: stubs the REST poll during the websocket check and polls for the fixture prices instead of a fixed sleep.
    • monero startup-clamp churn recovery: deadline 5s → 30s.
    • XMR single-flight signer: 1s → 10s.
    • go-tss testProcessTaskDone: 20ms → 2s, and it now waits for the goroutine.
    • FROST early-result quorum: 20ms → 2s.

  • Tests whose fixtures or expectations are mocknet constants skip themselves on other networks (CI runs unit tests under -tags=mocknet). Every package was swept under mainnet, stagenet, chainnet and no tag against the base commit, with no new failures.

  • Tests are hermetic: httptest fakes for THORNode/chain RPCs, in-memory libp2p, and fixture preparams instead of safe-prime generation.

  • Tests for network-variant files carry the same build constraint as the file under test, so they never compile into the mocknet run. Instead, make test-network-specific (part of test-coverage/test-coverage-sum) now runs them under their own tag:

    • mainnet: ConfAdjustmentMainnetCov and MigrationsMainnetCov
    • stagenet: MigrationsStagenetCov
    • chainnet: MigrationsChainnetCov
    • regtest: RegtestCov in ./app and ./cmd/thornode, plus QuerierRegtestCov

    Before this, no CI job ran them.

Coverage badge (README)

The README badge shows GitLab's develop coverage. The unit-tests job's coverage: regex scrapes it from make test-coverage-sum. That target only stripped *.pb.go from the profile, so ~155k statements of generated *.pulsar.go plus openapi/gen (all 0%) stayed in the denominator. That held the badge at 21.2%, while hand-written code was at 71%.

scripts/coverage-strip-generated.sh now drops every file with Go's // Code generated … DO NOT EDIT. header from coverage.txt in both test-coverage targets; coverage.xml and coverage.html follow. make test-coverage-sum on this branch reports total: (statements) 90.0%. The badge updates after the first develop pipeline post-merge.

Test seams (behaviour-preserving)

16 files, where tests alone couldn't reach 80%:

  • Package-level vars defaulting to the original call:
    • cmd/bifrost/main.go: startP2P, newTss, loadChains, and legacyRecoveryRetryInitial (const → var)
    • bifrost/pkg/chainclients/loadchains.go: failedChainRetryInterval
    • go-tss cmd/tss and cmd/tss-benchgen
    • tools/pubkey2address: chainNetwork
  • main() → run() extraction: tools/gen-preparams, tools/gen-keyshare-fixtures.
  • Init-time embedded JSON decoding via a named helper (same Unmarshal+panic): common/tokenlist/*, common/tcyclaimlist, x/thorchain/xmr_genesis_delay_mocknet.go.

Equivalence was checked empirically:

  • HEAD vs branch binaries on 20 invocations: stdout, stderr and exit code are identical after normalising source paths in stack traces. The one diff is pre-existing goroutine ordering in gen-keyshare-fixtures, which HEAD also shows against itself.
  • SHA-256 of all init-loaded token/claim lists under mocknet and mainnet.
  • The XMR genesis-delay env initializer for several inputs.

Test-time impact

Measured per package, interleaving HEAD/branch runs on the same machine:

  • make test unit run: total package time 515s → 620s (+20%). The slowest package (bifrost/observer, ~49s) is unchanged, so parallel wall clock barely moves.
  • test-race-bifrost: 590s → 670s (+14%); slowest package unchanged.
  • go-tss: ≈ unchanged (~+5s in go-tss/tss).

Most of the added time comes from two sources:

  • Hard-coded sleeps and retry intervals in the code under test (e.g. test/regression/cmd +12s, tools/p2p-check +8s).
  • Test-binary links for packages that previously had no tests.

Production bugs found by these tests (fixed separately in !5194 (merged))

This MR is test-only: its production code changes are limited to the behaviour-preserving test seams and coverage tooling above. Bugs the new tests uncovered are fixed in !5194 (merged), which is stacked on this branch; there, the tests are flipped to assert the fixed behaviour, while here they document current behaviour. The CI image-mirror fallbacks that were briefly on this branch moved to !5195 (merged). The migrateFunds dust-burn atomicity gap is tracked in #3394.

Bugs fixed in !5194 (merged):

  • Bitfinex websocket/REST short-array panic (crashes bifrost)
  • KeySign.getVersion data race; evm getTxInOptimized shared-error race (could drop valid inbounds)
  • unstuckTx nil-TxOutItem panic (evm and ethereum)
  • ERC20 decimals() uint256 branch; x/denom mintTo validation order
  • redeemSynthAssetToReserve non-atomic transfer
  • reconstructVaultLiability requesting "latest" instead of "pending"
  • inactive-vault MaxGas budget rebalance (max_gas_deducted), with the ragnarok double-payment guard from review
  • LoadChains retry ticker never stopped

Suspected production bugs (documented by tests, NOT fixed here)

A test asserting current behaviour pins each of these, so fixing one means updating its test. Each is worth a follow-up issue:

  • Panic: castTxOutItem (x/thorchain/querier.go) dereferences AggregatorTargetLimit/CloutSpent without nil checks. The slasher sets AggregatorTargetLimit = nil on reassignment, so pending/scheduled outbound queries could panic.
  • SwapperClout.Valid() is inverted, so ValidateGenesis rejects any export containing clout. There is also a codec mismatch: DefaultGenesis uses protojson while InitGenesis uses amino.
  • Deferred blame handling overwrites err, so keysign failures return a nil error in xrp/gaia SignTx and bifrost/tss GenerateNewKey.
  • GetTHORName checks expiry before returning the decode error. A corrupt (undecodable) record at a positive height comes back as an expired name with an empty owner and a nil error, and handler_manage_thorname.go's ownership check then lets any signer take the name. Users can't reach this (keeper writes can't produce an undecodable record), but it would turn store corruption into a takeover, so it should be a priority follow-up.
  • Failures swallowed by log-and-continue, worth confirming they should not surface:
    • SetLastSignedHeight in handler_ragnarok.go
    • rolling pool liquidity fee in keeper_liquidity_fees.go
  • Smaller issues:
    • pollingProvider.Start calls wg.Add without Done.
    • CustomGRPCGatewayRouter's lower-case height-header check never fires.
    • x/scheduler GetQueryCmd returns the denom command tree.
    • monero EnqueueAuthenticatedInternalInboundReplay's BlockHeight <= 0 guard is unreachable (ObservedTx.Valid rejects it first).
    • p2p acknowledges a leader's join-party response twice (harmless).
  • Stagenet Migrate4to5 strands 1.8M TCY in the thorchain module (already executed on stagenet).
  • Broken dev tools:
    • tools/pubkey2address always panics.
    • recover-keyshare-backup panics ("Config is sealed").
    • tss-benchgen and gen-keyshare-fixtures fail against tss-lib v3.

Verification

Review follow-ups:

  • Third round:
    • The upgrade-handler reject runs in a discarded cache context, as production rolls back a failed msg.
    • The unbond-handler test fails the handler's own SetBondProviders/SetNodeAccount (call-counted) instead of refundBond's writes.
    • The savers-utilization test sets SaversEjectInterval explicitly instead of relying on the mocknet constant.
    • The ethereum unstuck test asserts that the signer cache records the cancel tx hash.
  • Second round:
    • monero asserts the real error text.
    • The solana floor clamp asserts the minContextSlot param it sends.
    • xrp labels the post-blame nil error as a known bug.
    • Affiliate liquidity tests verify each deposit's exact 90/10 unit split and that LP unit deltas sum to the pool's minted units, for both RUNE and BTC affiliate addresses.
    • The TCY claim test handles networks with no configured smart-contract addresses.
    • The missing-anchor test enables derived depth, stages the anchor, and verifies recovery when the anchor becomes available.
  • Regression checks: the strengthened pending-block and redemption tests fail against the old production code.

Checks run:

  • go test -tags=mocknet ./x/thorchain -count=1: pass.
  • Full Ethereum and EVM packages with -race -tags=mocknet: pass.
  • Affected THORChain suites under -tags=chainnet: pass.
  • make lint-ci script/Trunk checks plus GOTOOLCHAIN=go1.26.6 make lint-ci-golangci: pass.
  • go test -tags=mocknet ./... (excl. go-tss), go test -tags stagenet ./common, test-race-bifrost, and go-tss with -race: pass.
  • golangci-lint (repo config, plus --no-config for test/simulation) and gofumpt: clean.
  • Test files compile under mocknet, mainnet, stagenet, chainnet, regtest and no tag (go vet).
  • test/simulation: three pre-existing tests that need a live mocknet still fail locally, as on develop.

🤖 Generated with Claude Code

Edited by Huginn Ai

Merge request reports