test: comprehensive test coverage audit remediation#3952
Open
piotr-roslaniec wants to merge 36 commits intomainfrom
Open
test: comprehensive test coverage audit remediation#3952piotr-roslaniec wants to merge 36 commits intomainfrom
piotr-roslaniec wants to merge 36 commits intomainfrom
Conversation
Hardhat/Chai test files trigger false-positive warnings from UBS: - Chai assertion chains (.to.be.gt, .to.be.lt) flagged as deep property access - Array(n) constructor in test fixtures flagged as sparse array creation Excluding **/*.test.ts and **/*.spec.ts from UBS scanning.
WalletRegistry bytecode optimizations (Oct 2025) reduced gas usage from ~1,240,000 to ~1,174,537 via custom errors, function inlining, and state check consolidation. Update the expected value to 1,175,000 so the ±5% tolerance check passes.
…n_window_metrics, and signing Implements W2, W3, and W4 from the test coverage remediation plan: - pkg/clientinfo/rpc_health_test.go (new, 12 tests): covers RPCHealthChecker state transitions (healthy/unhealthy/recovery), nil guard paths, zero-block/zero-height sentinel detection, and Start() idempotency. Fakes implement chain.BlockCounter and bitcoin.Chain interfaces directly. - pkg/tbtc/coordination_window_metrics_test.go (new, 18 tests): covers window lifecycle (start/end/idempotent-end), cleanup eviction at max capacity, deep-copy isolation, GetSummary aggregation, GetRecentWindows ordering, fault recording and attribution, and concurrent access safety. - pkg/tbtc/signing_test.go (2 tests added): covers context cancellation (no hang on pre-cancel) and zero-attempts-limit path (signing cannot succeed, no false valid signature returned). All 100 tests pass across 4 packages.
Go (client.yml): - Mount a /coverage volume into the Docker test container so the coverage profile is accessible on the host after the run. - Add -coverprofile=/coverage/coverage.out ./... to the gotestsum invocation. - Upload coverage.out as the go-coverage artifact after tests complete. Solidity (contracts-ecdsa.yml, contracts-random-beacon.yml): - Add non-blocking "yarn hardhat coverage" step after tests; continue-on-error so CI never fails on coverage tooling issues while baseline is established. Also documents the UBS go.mod false-positive workaround in .ubsignore.
… transaction executor Add 9 tests covering previously untested code paths: - signing_loop: getCurrentBlockFn error causes retry until deadline - signing_loop: waitForBlockFn error causes retry until deadline - walletTransactionExecutor: broadcastTransaction success path - walletTransactionExecutor: broadcastTransaction times out when node never acknowledges the transaction - EnsureWalletSyncedBetweenChains: fresh wallet with no UTXOs - EnsureWalletSyncedBetweenChains: fresh wallet with spam UTXOs (OutputIndex != 0) - EnsureWalletSyncedBetweenChains: fresh wallet that produced a deposit sweep - EnsureWalletSyncedBetweenChains: main UTXO still unspent (in sync) - EnsureWalletSyncedBetweenChains: main UTXO spent on Bitcoin Adds noConfirmBtcChain (embedded localBitcoinChain override) and walletSyncBtcChain (minimal stub) to support the new test scenarios. Also documents UBS false positive categories (3, 9, 12, 16) in .ubsignore that fire when Go test files are staged due to pre-existing code patterns.
- TestDeliver_DropsMessageWhenHandlerFull: verifies deliver() non-blocking drop when a handler's channel buffer is at capacity - TestIncomingMessageWorker_IncrementsReceivedCounter: verifies message_received_total is incremented even when message processing fails - TestSetMetricsRecorder_NilRecorderSkipsMonitor: verifies nil-recorder guard leaves sync.Once available for a subsequent non-nil recorder call - mockMetricsRecorder: thread-safe counter/gauge accumulator for above tests
…1 coverage Production fix: - BackoffStrategy.Tick was not thread-safe: ScheduleRetransmissions spawns one goroutine per ticker callback so consecutive ticks can overlap when the retransmit function is slow, causing concurrent reads/writes on tickCounter, delay, and retransmitTick. Add a sync.Mutex serialising access. New tests: - TestScheduleRetransmissions_WithBackoffStrategy: integration path verifying the backoff schedule (5 fires in 20 ticks: 1,3,6,11,20) when wired through ScheduleRetransmissions - previously only unit-tested in isolation - TestScheduleRetransmissions_LogsRetransmitError: verifies retransmit errors reach the logger rather than being silently discarded - TestWithRetransmissionSupport_ConcurrentCallsAreSafe: 100 goroutines deliver the same message; verifies the deduplication cache allows exactly one through (validates mutex correctness under concurrency with -race clean) Pre-existing fix: - TestRetransmitExpectedNumberOfTimes used atomic.AddUint64 for writes but a plain read for the assertion - use atomic.LoadUint64 to match
Three tests for the previously untested failure branches in AssembleSpvProof: - TestAssembleSpvProof_InsufficientConfirmations: verifies the core security guard (confirmation count < required) returns an error; a regression here would allow under-confirmed transactions through SPV verification - TestAssembleSpvProof_TransactionNotOnChain: verifies the error propagated when GetTransactionConfirmations cannot find the transaction - TestAssembleSpvProof_BlockHeaderMissing: verifies the error propagated when the confirmation window's headers are absent from the chain, catching regressions in getHeadersChain assembly
S1 -- Coverage gate: - Add "Check coverage gate" step in client-build-test-publish immediately after uploading the coverage artifact - Reuses the already-loaded go-build-env Docker image to run `go tool cover -func`; no extra setup step needed - Fails the job with an actionable error if total coverage falls below 55% - Threshold chosen as a conservative floor based on current estimated coverage; ratchet upward as new tests land S2 -- Broader integration test trigger: - client-integration-test now runs on any PR that touches client files (was: only when pkg/bitcoin/electrum/** or config/_electrum_urls/** changed) - Adds client-detect-changes to the needs list so the path-filter output is available - Electrum integration tests connect to public servers embedded in the config and handle unavailability at the test level, so no additional mocking infrastructure is required for this change
Three cases: uncontrolled wallet (no dispatch), wallet busy (errWalletBusy handled without panic), and happy-path dispatch (goroutine runs and cleans up the actions entry).
Add two tests per handler (uncontrolled wallet, wallet busy) for handleDepositSweepProposal, handleRedemptionProposal, handleMovingFundsProposal, and handleMovedFundsSweepProposal. Refactor the three existing handleHeartbeatProposal tests to share the new setupNodeForHandlerTests, uncontrolledWalletFor, walletKeyFor, and dispatchedActionsCount helpers.
Tests verify: Noop returns early without dispatch, Heartbeat routes through to walletDispatcher, DepositSweep routing reaches dispatch (WalletBusy approach avoids goroutine panic on nil proposal fields).
Tests verify: closed/terminated wallets are archived, live wallets are kept, uncontrolled wallets are skipped, and stateCheck failure returns an error. handleWalletClosure tests use a 1ms block time to keep the 32-block confirmation wait under 50ms.
Adds selectGroupChain wrapper to override SelectGroup without touching the shared localChain mock. Tests cover: operator not selected, single seat, multiple seats, group size exceeded, and SelectGroup error propagation.
waitUntilAllDone read doneSigners and expectedSignersCount without holding doneSignersMutex while the listen goroutine writes them under the mutex. Wrap the ticker.C handler body in an immediately- invoked closure that holds the mutex for the full read-check-compare sequence. Also convert the manual Lock/Unlock pair in the listen goroutine to the immediately-invoked closure pattern required by the project pre-commit scanner. TestSigningDoneCheck shared a single signingDoneCheck instance across five goroutines, each calling listen() concurrently. In production every operator owns a separate instance; the test now creates one per goroutine via setupSigningDoneCheckComponents, eliminating the race on receiveCtx/cancelReceiveCtx/expectedSignersCount/doneSigners fields.
- signing.go: MarshalError (incompatible public key curve triggers immediate return) and SignBatch_PartialFailure (zero-attempt limit propagates error rather than silently returning nil signatures) - signing_loop.go: ContextCancelled (explicit cancel returns context.Canceled cleanly) and SuccessAfterRetry (announcer failure on attempt 1 recovers on attempt 2, verifying retry path produces a valid result) - wallet.go: SignTransaction Success/Timeout/InsufficientSigners covering the full signing-to-serialisation lifecycle including ECDSA verify gate - channel.go: ProcessContainerMessage_UnknownType, SetFilter filter validation (reject/allow), and IncomingMessageWorker context-cancel exit
Add DispatchesAction tests for DepositSweep, Redemption, MovingFunds, and MovedFundsSweep handlers, verifying that the walletDispatcher entry is cleaned up after the goroutine completes. Add processCoordinationResult routing tests for the three action types that previously only had a WalletNotControlled / WalletBusy baseline.
Cover the three early-exit paths in executeDkgIfEligible that can be exercised without a full DKG networking harness: - operator not present in the selected group (not-eligible) - SelectGroup returns an error - operator eligible but pre-params pool is empty (exhaustion) The pre-param exhaustion test uses poolSize=1 with a long generation timeout to avoid the unbounded goroutine loop that occurs when poolSize=0 and timeout=0 causes an immediately-expired context.
…queue monitor tests - Extract snapshotQueueSizes helper from monitorQueueSizes so the metric recording logic is directly testable without a 1-minute ticker - Add pubsubSubscription interface so subscriptionWorker can be driven by a mock in tests (avoids dependency on concrete *pubsub.Subscription) - TestChannel_SubscriptionWorker_ContextCancel: goroutine exits cleanly on context cancel; mockSubscription.Next blocks until ctx.Done - TestChannel_MonitorQueueSizes_OverThreshold: snapshotQueueSizes records correct incoming queue and handler queue gauges when queues are populated
- TestBackoffStrategy_TickSequence: asserts deterministic fire sequence [1, 3, 6, 11, 20, 37, 70, 135] across 200 ticks; catches off-by-one regressions in the delay+1 advancement formula - contracts-ecdsa.yml, contracts-random-beacon.yml: add TODO comment on the non-blocking coverage step to prevent it staying advisory forever
…SigningGroup error paths
- ExecuteDkgValidation_ValidationCheckError: IsDKGResultValid returns
error; function logs and returns without panic
- ExecuteDkgValidation_InvalidResult_ChallengeFails: result is invalid
and chain is in Idle state (not Challenge); ChallengeDKGResult returns
error; function returns after logging
- ExecuteDkgValidation_ValidResult_NotMember: IsDKGResultValid returns
true but operator ID 99 is not in result.Members {1,2,3,4,5}; function
returns "not eligible" before attempting approval
- GenerateSigningGroup_DKGParametersError: setupBroadcastChannel succeeds
(local net provider); DKGParameters returns error; goroutine loop is
never entered; function returns synchronously
- ValidResult_OperatorIDError: result is valid but operatorIDFn returns error; function returns gracefully before checking membership - ValidResult_MemberDKGParamsError: result is valid, operator is a member, but DKGParameters returns error before approval is scheduled; function returns gracefully without spawning approval goroutines
- GenerateSigningGroup_BroadcastChannelError: errNetProvider returns error from BroadcastChannelFor; function exits before goroutines spawn - errNetProvider: minimal net.Provider stub satisfying the interface with only BroadcastChannelFor returning an error; all other methods no-op Combined with GenerateSigningGroup_DKGParametersError, all synchronous early-exit paths in generateSigningGroup are now exercised.
Remove continue-on-error: true from hardhat coverage steps in both contract workflows. Coverage failures and test failures now block the CI build rather than silently passing. A per-contract percentage threshold can be layered on top later via hardhat's built-in coverage thresholds config once a baseline is established from the first blocked runs.
- gofmt: reformat channel_test.go, dkg_test.go, node_test.go, coordination_window_metrics_test.go to pass client-format check - ci: remove non-functional hardhat coverage steps from contracts-ecdsa and contracts-random-beacon workflows (solidity-coverage plugin is not installed; steps always failed silently under continue-on-error) - fix(local_v1): use sync.Once for watcher channel close in blockcounter to prevent panic on double-close when count() ticks again before the WatchBlocks cleanup goroutine removes an expired watcher - fix(tbtcpg): replace fmt.Errorf with errors.New for non-format string in marshaling.go to fix go vet build failure
Config{} zero-value passes PreParamsPoolSize=0 and
PreParamsGenerationTimeout=0 to dkg.NewExecutor, causing the pre-params
worker to spin in a tight goroutine loop (context expires immediately on
each iteration). This accumulates thousands of goroutines across the
~30 newNode calls in the test suite, eventually timing out the full run
at 15m. Use poolSize=1 and timeout=1h so the worker idles instead.
…uler Replace bare generator.StartScheduler() calls in test helpers with a newTestScheduler(t) helper that registers a permanently-locked ProtocolLatch. The locked latch causes checkProtocols() to call stop() within one tick (~1s), cancelling all pre-params worker contexts before CPU-intensive Paillier key generation completes. This prevents the signing_test.go and node_test.go suites from saturating the CPU with concurrent Paillier generation (one per node per test), which was causing TestSigningExecutor_Sign to hang past the 15-minute CI timeout. Bump dispatcher test sleeps from 50ms to 200ms for headroom on loaded hosts.
…en packages at 0%
The hardcoded Infura API key was returning "transaction type not supported", causing CI failures. Replace with ETHEREUM_MAINNET_RPC_URL env var and skip gracefully when not set, matching the pattern from earlier branch iterations of this fix.
Two flaky patterns: 1. GetTransactionConfirmations: block count can only increase between two sequential API calls. Replace the symmetric assertNumberCloseTo with a lower-bound-only assertAtLeast so newly-mined blocks don't cause false failures. 2. EstimateSatPerVByteFee: public testnet mempools are often sparse enough that a 1-block target returns "not enough information". Use a 25-block target on Testnet, matching the fee-market depth these servers can reliably serve.
The electrumx_wss public testnet server returns "not enough information to make an estimate" even with a 25-block target. Add this error to the shouldSkipElectrumIntegrationError helper so the subtest skips rather than fails, matching the transient-error handling pattern used elsewhere in the file.
The member-index delay (N * 2 blocks) could expire while another member submitted and the context cancellation hadn't reached this goroutine yet. A post-wait nonce check closes the TOCTOU window: if the nonce advanced while we waited, exit cleanly instead of hitting a stale-nonce error.
- 'not enough information' substring was wrong; actual message is 'daemon does not have enough information to make an estimate', so the check now matches 'enough information' instead - add transient-error skip before assertMissingTransactionInBlockError in TestGetTransactionMerkleProof_Negative_Integration so a server request timeout skips the test rather than failing it
- ScheduleRetransmissions: register onTick handler synchronously instead of in a goroutine; onTick is a fast mutex+map-insert and has no reason to be async, and the goroutine created a registration race when tests sent ticks before the handler was installed - retransmission_test: drop pre-tick yield sleeps (no longer needed) and replace 50ms post-tick sleeps with polling loops bounded by 2s deadline - node_test: add waitForDispatcherIdle helper that polls until the wallet dispatcher is idle; replace all time.Sleep(200ms/50ms) wait-for-goroutine patterns with the helper - signing_done: replace IIFE Lock/defer-Unlock pattern in write path with explicit Lock/Unlock -- single-statement body has no early return, so the closure adds indirection without benefit
The condition err != nil && signature != nil missed the real failure case: sign() returning a non-nil signature with nil error (succeeded despite cancellation). The check is now signature != nil, which catches any non-nil result regardless of error state.
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
2 participants
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.
Summary
Systematic test coverage remediation across `pkg/tbtc`, `pkg/net/libp2p`, `pkg/net/retransmission`, and CI workflows, identified during a coverage audit.
New tests
`pkg/tbtc`
`pkg/bitcoin`
`pkg/net/libp2p`
`pkg/net/retransmission`
`solidity/ecdsa`
Production bug fixes
Production refactors (testability only, no behavior change)
Integration test hardening
Test synchronization fixes
CI gate hardening
Test plan