Skip to content

harness: basic infrastructure test environment - #5

Merged
bhandras merged 5 commits into
mainfrom
harness
Nov 17, 2025
Merged

harness: basic infrastructure test environment#5
bhandras merged 5 commits into
mainfrom
harness

Conversation

@bhandras

@bhandras bhandras commented Nov 14, 2025

Copy link
Copy Markdown
Member

This PR introduces a comprehensive test harness infrastructure that provides a programmatic
integration test environment for running end-to-end tests against real Bitcoin and Lightning Network
infrastructure.

Overview

The harness creates an isolated Docker-based test environment with:

  • Bitcoin Core (regtest mode)
  • Electrs (Esplora HTTP API)
  • LND nodes (primary + additional nodes on demand)
  • Tapd instances
  • PostgreSQL (optional)

Each test run gets its own isolated Docker network, dynamic port allocation, and dedicated artifact
directories for logs and data, enabling fully parallel test execution.

Key Features

🏗️ Infrastructure Management

  • Automatic Docker container lifecycle management (bitcoind, lnd, electrs, postgres, tapd)
  • Per-test isolation with unique Docker networks and dynamic port allocation
  • Automatic cleanup with signal handling (SIGINT, SIGTERM)
  • Test artifact collection (logs, data directories) organized by test name and timestamp

⚡ Performance Optimizations

  • Concurrent container startup (postgres starts in parallel with bitcoind)
  • Controlled test parallelism via -test.parallelism flag (default: 4)
  • Pre-mined blocks for immediate coinbase spending
  • Standardized 200ms polling intervals for consistent behavior

🧪 Test Helpers

  • Block generation: Generate(), GenerateAndWait()
  • Wallet funding: Faucet()
  • Mempool inspection: MempoolTxIDs(), WaitMempoolTxCount()
  • Multi-node setup: StartAdditionalLND(), SetupChannelBetween()
  • Tapd harness: NewTapdHarness() for isolated tapd instances
  • RPC client creation: BitcoinRPCClient(), GetRPCConfig()

📊 Test Coverage

  • 8 comprehensive test cases covering all major harness functionality
  • 86.2% code coverage
  • Tests run in parallel with semaphore-based concurrency control

@gemini-code-assist

Copy link
Copy Markdown

Summary of Changes

Hello @bhandras, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request introduces a robust and isolated integration test environment through a new harness package. It automates the setup and teardown of a regtest Bitcoin Core, LND, and optionally tapd instances within Docker containers, providing a consistent and reproducible testing ground. The changes also include necessary updates to build configurations and Go module dependencies to support this new infrastructure, streamlining the development and testing workflow.

Highlights

  • New Test Harness Infrastructure: Introduced a new harness package that provides a programmatic integration-test runner. This harness sets up a comprehensive test environment using Docker containers for Bitcoin Core, LND, and optionally tapd.
  • Docker-based Environment Orchestration: The harness manages the lifecycle of Docker containers for bitcoind, lnd, electrs (Esplora HTTP API), and postgres (optional), ensuring isolated test runs with dynamic ports and per-run Docker networks.
  • Automated Test Setup and Teardown: Includes utilities for pre-mining blocks, funding wallets, and establishing LND channels. It also handles artifact management (data directories, logs) and guarantees clean teardown of all resources, even on unexpected test termination.
  • Build Tag and Linter Configuration Updates: Updated .golangci.yml to reflect new build tags (test_postgres, test_sqlite) and added specific linter rules for the new harness package. The Makefile and database test files (db/test_postgres.go, db/test_sqlite.go) were also updated to use the new build tag names.
  • Go Module Dependency Updates: The go.mod and go.sum files received significant updates, incorporating new dependencies required for the harness, such as github.com/btcsuite/btcd, github.com/lightninglabs/lndclient, github.com/lightninglabs/taproot-assets/taprpc, and github.com/lightningnetwork/lnd, along with numerous indirect dependency updates.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@claude

claude Bot commented Nov 14, 2025

Copy link
Copy Markdown

Pull Request Review: Integration Test Harness

Summary

This PR adds a comprehensive integration test harness for spinning up bitcoind, LND, and tapd instances in Docker for end-to-end testing. The implementation is well-structured and follows the project's coding standards. I've reviewed the code for quality, security, performance, and test coverage.


Positive Highlights

Strong Code Quality

  • Excellent documentation: Package and function comments are detailed and follow the "how/why" philosophy from CLAUDE.md
  • Good error handling: Proper use of require.NoError and error context throughout
  • Clean resource management: stopOnce ensures idempotent cleanup, signal handlers prevent orphaned containers
  • Parallel test support: Dynamic port allocation and per-run Docker networks enable concurrent test execution
  • Artifact management: Comprehensive logging to files with optional stdout, preserves artifacts on failure

Good Architectural Decisions

  • Separation of concerns: Harness for infrastructure, TapdHarness for test client instances
  • Flexible configuration: Options struct with sensible defaults via DefaultOptions()
  • Test helper methods: Generate(), Faucet(), WaitMempoolTxCount() provide clean test APIs
  • Proper scoping: Best-effort cleanup operations appropriately ignore errors (e.g., pruneStaleHarnessNetworks)

Issues & Recommendations

1. Build Tag Consistency ⚠️

The PR changes build tags from test_db_postgres/test_db_sqlite to test_postgres/test_sqlite, but this change appears incomplete:

Files checked:

  • db/test_postgres.go:1
  • db/test_sqlite.go:1
  • .golangci.yml:7-9
  • Makefile:268

Recommendation: Verify no other files use the old tags. Search the codebase:

grep -r "test_db_postgres\|test_db_sqlite" .

2. Logging Standards Compliance ⚠️

The harness uses fmt.Print/fmt.Printf for logging, which doesn't follow the project's structured logging requirements:

Current implementation (harness.go:331, 336-342):

if h.opts.HarnessLogStdOut {
    fmt.Print(logLine)
}

func (h *Harness) Log(args ...any) {
    h.logWithCaller(1, fmt.Sprint(args...))
}

Issue: CLAUDE.md requires structured logging with slog-style methods ending in S. The harness logging doesn't use context.Context or key-value pairs.

Recommendation: Consider whether test harness code falls under the exemption for forbidigo (line 242-245 of .golangci.yml). If not, refactor to use structured logging:

log.InfoS(ctx, "Generated blocks",
    slog.Int("count", blocks),
    slog.String("address", addr))

3. Hardcoded Credentials 🔒

Security concern (harness.go:776-777, 1136-1137, 1172):

"-rpcuser=admin1",
"-rpcpassword=123",

Issue: While acceptable for local testing, these credentials are weak and hardcoded in multiple places (bitcoind startup, RPC client creation, HTTP auth).

Recommendation:

  • Generate random credentials per harness instance or use a stronger default
  • Centralize credential management in a single location (e.g., const block or Options)
  • Add a comment explaining this is test-only (not for production)

4. Error Handling Inconsistency

Line 786 (harness.go):

btcHostDir, _ := filepath.Abs(h.bitcoinDataDir)

Issue: Error is silently ignored. If filepath.Abs() fails, the bind mount could fail or use an incorrect path.

Recommendation:

btcHostDir, err := filepath.Abs(h.bitcoinDataDir)
require.NoError(h.T, err, "failed to get absolute path for bitcoind data dir")

Similar issues: Line 1117 uses interface{} instead of any (inconsistent with Go 1.18+ style at line 1053).

5. Resource Leak Risk ⚠️

harness.go:1179:

defer resp.Body.Close()

Issue: While correct, http.DefaultClient reuses connections. If many concurrent tests make RPC calls, connection pooling could exhaust resources.

Recommendation: Create a dedicated http.Client with reasonable timeouts and connection limits:

var bitcoinHTTPClient = &http.Client{
    Timeout: defaultTimeout,
    Transport: &http.Transport{
        MaxIdleConns:        10,
        MaxIdleConnsPerHost: 2,
        IdleConnTimeout:     30 * time.Second,
    },
}

6. Test Coverage ⚠️

harness_canary_test.go:

  • Only tests harness startup and TapdHarness creation
  • Doesn't verify key functionality: block generation, wallet funding, RPC calls, esplora integration

Recommendation: Add tests for:

func TestHarnessMining(t *testing.T)         // Generate(), GenerateAndWait()
func TestHarnessFaucet(t *testing.T)         // Faucet(), wallet funding
func TestHarnessMempool(t *testing.T)        // MempoolTxIDs(), WaitMempoolTxCount()
func TestHarnessEsplora(t *testing.T)        // Esplora API integration
func TestHarnessParallel(t *testing.T)       // Parallel test execution

Per CLAUDE.md: "Strive for near 90% test coverage where practical."

7. Magic Numbers

harness.go:46, 52, 56, 626, 1105, 1039:

numInitialBlocks = 106
pollInterval = 100 * time.Millisecond
time.Sleep(50 * time.Millisecond)
const cushion = 1000 * time.Millisecond

Minor: These constants lack explanatory comments.

Recommendation: Add comments explaining the "why":

// numInitialBlocks ensures coinbase maturity (100 blocks) plus a buffer
// for spending the coinbase outputs in tests.
numInitialBlocks = 106

8. Potential Race Condition

harness.go:445-451:

go func() {
    <-h.sigCh
    if h != nil {  // This check is always true
        h.Log("signal received, stopping harness...")
        h.Stop()
    }
}()

Issue: The if h != nil check is pointless (h is captured in closure). However, if Stop() is called concurrently from test cleanup and signal handler, there's no issue because stopOnce ensures single execution.

Recommendation: Remove the unnecessary nil check for clarity.


Commit Message Review

✅ Commit 1: "harness: basic infrastructure test environment"

  • Proper package prefix
  • Present tense
  • Clear and concise

✅ Commit 2: "git: ignore test-artifacts directories"

  • Correct prefix
  • Atomic change

✅ Commit 3: "build: linter fixes"

  • Appropriate for build-related changes
  • Should ideally include more detail in the body (what linter issues were fixed?)

Recommendation: For commit 3, add a body explaining the changes:

build: linter fixes

Update build tags from test_db_postgres/test_db_sqlite to test_postgres/
test_sqlite for consistency. Add harness package exclusions for forbidigo
and tagliatelle linters. Add protobuf replace directive to allow list.

Performance Considerations

Parallelization

Good: Dynamic port allocation and isolated Docker networks enable parallel test execution

Container Startup Time

⚠️ Consideration: Starting multiple containers (bitcoind, electrs, lnd, tapd, postgres) sequentially takes time.

Optimization suggestion: Start containers concurrently where dependencies allow:

var wg sync.WaitGroup
wg.Add(2)
go func() { defer wg.Done(); h.startBitcoind() }()
go func() { defer wg.Done(); h.startPostgres() }()
wg.Wait()
// Then start dependent services (lnd, electrs)

Docker Image Caching

Good: Uses tagged images (lightninglabs/bitcoin-core:29) which Docker caches


Security Considerations

1. Network Isolation

Per-harness Docker networks prevent test interference

2. Hardcoded Credentials 🔒

See Issue #3 above

3. File Permissions ⚠️

harness.go:365, 396-398, 757:

os.MkdirAll(h.opts.ArtifactsBaseDir, 0o755)
os.WriteFile(path, buf.Bytes(), 0o644)

Consideration: Artifacts may contain sensitive data (macaroons, TLS certs). Mode 0o644 allows group/world read.

Recommendation: Use 0o700 for directories and 0o600 for files containing credentials:

os.MkdirAll(h.lndDataDir, 0o700)

Dependencies Review

New Major Dependencies (go.mod)

  • github.com/lightninglabs/lndclient
  • github.com/lightninglabs/taproot-assets
  • github.com/lightningnetwork/lnd
  • github.com/ory/dockertest/v3

Analysis: All dependencies are from trusted Lightning Labs/LND ecosystem. The forked protobuf dependency is properly documented in .golangci.yml:91-92.

Dependency Hygiene

⚠️ go.sum changes: 524 additions, 43 deletions - large but expected for adding LND dependencies

Recommendation: Run go mod tidy to ensure no unnecessary dependencies.


Final Recommendations

Before Merging:

  1. ✅ Run make lint to ensure all linter issues are resolved
  2. ✅ Run make build to verify compilation
  3. ⚠️ Add more comprehensive tests (see Issue [test, ignore] harness: basic infrastructure test environment #6)
  4. ⚠️ Fix error handling for filepath.Abs() (Issue multi: stage boarding actor client  #4)
  5. 🔒 Consider strengthening test credentials (Issue chainbackend+chainsource: add chainsource actor and backend impl #3)
  6. ⚠️ Verify build tag migration is complete (Issue lib: start adding lib helpers #1)

Follow-up Work:

  • Add integration tests for tapd asset operations
  • Document harness usage in docs/development_guidelines.md or a new docs/testing.md
  • Consider performance optimizations (parallel container startup)

Conclusion

This is a high-quality PR that adds essential integration testing infrastructure. The code is well-documented, follows project conventions, and demonstrates good architectural decisions. The issues identified are mostly minor and don't block merging, but addressing them would improve robustness and security.

Recommendation: Approve with minor changes (particularly Issues #1, #4, and #6)

Great work! 🚀

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a comprehensive test harness for integration testing, which is a great addition. The harness sets up bitcoind, lnd, and other services in Docker, providing a realistic testing environment.

My review focuses on the new harness implementation. I've found a couple of areas for improvement:

  • There's a potential bug in how payments are sent within the channel setup logic, where a failed payment would be retried.
  • The tapd container is running as root, which is inconsistent with lnd and not a security best practice.

I've provided specific comments and suggestions for these points. Overall, this is a solid foundation for integration testing.

Comment thread harness/harness.go Outdated
Comment thread harness/tapd_harness.go Outdated
Comment thread harness/tapd_harness.go Outdated
@claude

claude Bot commented Nov 14, 2025

Copy link
Copy Markdown

PR Review: Harness Infrastructure Test Environment

I've reviewed this PR which adds a comprehensive Docker-based test harness for integration testing. Here's my feedback:

✅ Strengths

  1. Well-structured architecture: The harness provides clean separation of concerns with separate files for core harness logic (harness.go), tapd-specific functionality (tapd_harness.go), and tests.

  2. Comprehensive logging: The logging infrastructure with caller tracking, timestamps, and dual output (file + optional stdout) is excellent for debugging test failures.

  3. Resource cleanup: Proper use of sync.Once in Stop() and signal handlers ensures clean teardown even on Ctrl+C or test failures.

  4. Parallel test support: Dynamic port allocation and per-run Docker networks enable parallel test execution.

  5. Good documentation: Package-level comments and function documentation follow Go conventions well.

🔍 Code Quality Issues

1. Inconsistent error handling patterns (harness.go:98)

The bicoind field name is a typo - should be bitcoind:

// bicoind is the bitcoind container.
bitcoind *dockertest.Resource

2. Magic numbers without constants

  • Line 46: numInitialBlocks = 106 - Good! This is properly defined.
  • Line 772: "-fallbackfee=0.00001" - Consider defining fee constants
  • Line 775: "-minrelaytxfee=0.00000500" - Consider defining fee constants

3. Potential resource leak (harness.go:547-550)

The saveLogs() error is silently ignored. While this may be intentional during shutdown, consider logging the error:

if h.T != nil && h.T.Failed() {
    if err := h.saveLogs(); err != nil {
        h.Logf("failed to save logs: %v", err)
    }
}

4. User ID assumptions (tapd_harness.go:180, 253)

The LND container uses os.Getuid() while tapd uses hardcoded "0:0". This inconsistency could cause permission issues:

User: fmt.Sprintf("%d:%d", os.Getuid(), os.Getgid()),  // LND
User: "0:0",  // tapd - why hardcoded?

The GitHub CI fix mentions this is needed for Docker mounts, but the inconsistency needs explanation or should be unified.

5. Context usage (harness.go:1149-1152)

Using context.Background() throughout RPC calls means they can't be canceled by parent contexts. Consider accepting contexts as parameters for better control.

6. Missing bounds checking (harness.go:1055-1063)

BlockCount() type assertions could panic. Add error handling:

var height uint32
switch v := raw.(type) {
case float64:
    height = uint32(v)
case int:
    height = uint32(v)
default:
    require.NoError(h.T, json.Unmarshal(res, &height))
}

7. Build tag changes (db/test_postgres.go, db/test_sqlite.go)

The renaming from test_db_postgres to test_postgres is a breaking change for anyone with existing tooling. Consider:

  • Documenting this in commit message
  • Ensuring all CI/CD pipelines are updated (✅ done in .github/workflows/main.yml)

🔒 Security Considerations

  1. Hardcoded credentials (harness.go:776-777): While acceptable for test environments, ensure these are never used in production. Consider adding comments clarifying this is test-only.

  2. Docker container naming: The random suffix approach is good for isolation, but pruneStaleHarnessNetworks() could potentially interfere with other users' containers on shared CI runners if network names collide.

⚡ Performance Considerations

  1. Sequential container startup: Containers are started sequentially. Consider parallelizing independent startups (bitcoind + postgres) to reduce test setup time.

  2. require.Eventually polling: Using 100ms-1s poll intervals is reasonable, but for faster tests, consider exponential backoff or checking service-specific readiness probes.

🧪 Test Coverage

The canary test (harness_canary_test.go) is minimal but functional. Consider adding:

  • Tests for error conditions (e.g., container startup failures)
  • Tests for parallel harness instances
  • Tests for the Generate(), Faucet(), and channel setup helpers

📝 Style & Convention Adherence

Follows CLAUDE.md guidelines:

  • ✅ Function comments start with function name
  • ✅ Structured logging with S methods and key-value pairs
  • ✅ 8-space tabs configured in .editorconfig
  • ✅ Error log levels used appropriately (mostly info/debug, not error for external failures)
  • ⚠️ Some lines exceed 80 chars (acceptable for structured logging, but some regular code too)

Minor style issues:

  • Line 1875: randSuffix() fallback comment could be more descriptive about when RNG would fail
  • Consider extracting some longer functions (e.g., startBitcoind, startLND) into sub-functions for better readability

🎯 Recommendations

  1. Fix the typo: bicoindbitcoind (line 98)
  2. Add error handling: For saveLogs() and type assertions
  3. Document user ID choice: Why tapd needs "0:0" while LND uses os.Getuid()
  4. Consider context propagation: Accept contexts in RPC helper methods
  5. Add integration test: Beyond the canary, test actual Bitcoin/Lightning operations
  6. Document breaking changes: The build tag renaming should be in commit message/PR description

📊 Overall Assessment

This is high-quality infrastructure code that will significantly improve integration testing capabilities. The design is solid, with good separation of concerns and proper resource management. The issues identified are mostly minor and don't block merging, but addressing them would further improve robustness.

Recommendation: Approve with minor suggestions

The foundation is excellent - great work! Once the minor issues are addressed, this will be a robust test harness.

@claude

claude Bot commented Nov 14, 2025

Copy link
Copy Markdown

Pull Request Review: Basic Infrastructure Test Environment

This PR introduces a comprehensive test harness infrastructure for integration testing with bitcoind, LND, and tapd running in Docker containers.


Strengths

Architecture & Design

  • Well-structured harness package with clean separation of concerns
  • Parallel test support via dynamic port allocation and per-run Docker networks
  • Resource isolation with dedicated Docker network and artifacts directory per test
  • Flexible configuration through Options struct

Code Quality

  • Excellent documentation following Go conventions
  • Proper error handling with descriptive messages
  • Resource cleanup with sync.Once ensures cleanup happens exactly once
  • Comprehensive logging with timestamps and file:line info

Testing Infrastructure

  • Real integration environment (actual bitcoind/LND/tapd, not mocks)
  • Useful helper methods: Generate(), Faucet(), SetupChannelBetween()
  • Chain sync primitives handle timing issues properly

Critical Issues

1. User/Permission Mismatch (harness/tapd_harness.go:253)

  • tapd runs as user 0:0 (root) while LND runs as os.Getuid():os.Getgid()
  • CI already needs sudo to work around this (main.yml:177)
  • Recommendation: Make both containers run with the same user/group

2. Missing Context Cancellation (harness/harness.go:1419-1424)

  • Long-running operations use context.Background() without test context
  • Operations may hang on test timeout
  • Recommendation: Use h.T.Context() where appropriate

3. Signal Handler Issue (harness/harness.go:445-451)

  • Meaningless nil check in signal handler goroutine
  • Recommendation: Remove nil check, add comment about stopOnce protection

Code Quality Issues

4. Inconsistent Error Handling (harness/harness.go:702-737)

  • saveLogs() ignores all errors
  • Recommendation: Log errors even if not returning them

5. Magic Numbers (harness/harness.go:46, 49, 52)

  • numInitialBlocks = 106, defaultTimeout = 30s lack explanation
  • Recommendation: Add comments explaining values

6. Dead Code (harness/harness.go:1899-1904)

  • NewTapClientHarness is unimplemented stub
  • Recommendation: Remove or implement

Security & Performance

Security

  • Hardcoded test credentials are acceptable for regtest
  • Artifact directories (0o755) should use 0o700 for macaroons/certs

Performance

  • Good: Pre-mining blocks before electrs, optional tapd startup
  • Consider: Container reuse for faster tests
  • AlwaysKeepArtifacts defaults to true - will fill disk in CI

Testing Concerns

Missing test coverage for:

  1. Postgres mode (--harness.postgres flag)
  2. Container cleanup and signal handling
  3. Parallel execution
  4. Channel setup failure scenarios

Build Tag Changes

Excellent simplification: test_db_postgres/test_db_sqlite renamed to test_postgres/test_sqlite. Changes correctly propagated to workflows, golangci, Makefile, and test files.


Recommendations Summary

Must Fix

  1. Resolve user permission mismatch (tapd vs LND)
  2. Use test contexts instead of Background()

Should Fix

  1. Log errors from saveLogs()
  2. Remove/implement NewTapClientHarness stub
  3. Default AlwaysKeepArtifacts to false
  4. Fix signal handler nil check

Nice to Have

  1. Document magic numbers
  2. Tighten artifact permissions
  3. Add postgres/parallel test coverage

Conclusion

Solid foundation for integration testing. Well-organized, properly documented, follows Go best practices. Main concerns: container user permissions and context handling.

Recommendation: Approve with minor changes. User permission issue should be addressed, but PR provides significant value. This enables comprehensive integration tests previously impossible - a major quality assurance improvement.

@claude

claude Bot commented Nov 14, 2025

Copy link
Copy Markdown

Pull Request Review: Basic Infrastructure Test Environment

Summary

This PR introduces a comprehensive Docker-based test harness infrastructure for integration testing. The implementation adds ~3,100 lines of well-structured code to support parallel test execution with bitcoind, LND, electrs, tapd, and optional PostgreSQL containers.


✅ Strengths

1. Excellent Code Organization

  • Clean separation of concerns between main harness (harness.go) and tapd-specific functionality (tapd_harness.go)
  • Well-structured options pattern with sensible defaults
  • Clear abstraction layers for container management

2. Robust Resource Management

  • Per-run isolation: Dynamic ports and private Docker networks prevent test collisions
  • Proper cleanup: stopOnce sync.Once ensures idempotent teardown
  • Signal handling: Goroutine-based cleanup on Ctrl+C prevents orphaned containers
  • Artifact management: Logs and data directories organized by group and timestamp

3. Strong Testing Support

  • Comprehensive helper methods: Generate(), Faucet(), WaitMempoolTxCount()
  • Support for multiple LND instances via StartAdditionalLND()
  • Channel setup helpers with SetupChannelBetween()
  • Built-in retry logic with require.Eventually patterns

4. Good Logging Practices

  • Timestamped logs with file:line caller information (harness.go:316-333)
  • Dual output to both file and stdout (configurable)
  • Contextual logging throughout lifecycle

5. CI Integration

  • Simplified build tags: test_postgres vs test_db_postgres
  • Fixed GitHub Actions caching with correct go.sum path
  • Added sudo for Docker volume permissions (necessary for user 0:0 in containers)

⚠️ Issues & Concerns

Critical Issues

1. Typo in Container Field Name (harness.go:98)

```go
// bicoind is the bitcoind container. // ❌ Typo: "bicoind" should be "bitcoind"
bitcoind *dockertest.Resource
```
Impact: Documentation inconsistency
Fix: Change comment to // bitcoind is the bitcoind container.

2. Unbounded Context Timeouts

Multiple RPC calls use fixed 30-second timeouts but don't handle context cancellation properly:

  • harness.go:1149-1151: bitcoinRPCCall creates context but doesn't propagate cancellation to caller
  • Consider accepting context.Context as first parameter throughout

3. Error Handling: Swallowed Errors

harness.go:549, 702-737: Log save errors are silently ignored with _ = h.saveLogs()
Recommendation: At minimum, log the error: if err := h.saveLogs(); err != nil { h.Logf("failed to save logs: %v", err) }

4. Missing Build Tag Validation

The build tag changes (test_postgres vs test_db_postgres) are breaking changes but there's no deprecation notice or validation to catch old tag usage.

Design Concerns

5. Global Flag State (harness.go:60-73)

Flags like harnessLogStdOut, harnessPostgres, artifactsBaseDirFlag are package-level globals. This can cause issues in parallel tests or when tests have conflicting requirements.

Recommendation: Consider moving these to Options struct exclusively:
```go
type Options struct {
// ... existing fields ...
UsePostgres bool // instead of global harnessPostgres flag
}
```

6. Race Condition Risk (harness.go:445-451)

Signal handler goroutine accesses h which could theoretically be nil if Stop() is called before goroutine starts:
```go
go func() {
<-h.sigCh
if h != nil { // ⚠️ This check is insufficient
h.Log("signal received, stopping harness...")
h.Stop()
}
}()
```
Fix: The check happens after channel read, so if Stop() was called and channel closed, this could panic. Consider checking before channel read or using context cancellation.

7. Hard-coded User IDs (tapd_harness.go:253)

tapd containers run as User: "0:0" while LND uses os.Getuid() (tapd_harness.go:180). This inconsistency could cause permission issues.

Question: Why does tapd need root (0:0) but LND doesn't? Document the reasoning or make it configurable.

8. Incomplete Implementation (harness.go:1899-1904, tapd_harness.go:487-492)

Multiple TODO stubs:
```go
func (h *Harness) NewTapClientHarness(name string) interface{} {
// TODO: Port TapClientHarness from tap-arktree when needed.
h.T.Fatal("NewTapClientHarness not yet implemented")
return nil
}
```
Recommendation: Either remove these stubs or add GitHub issue references in comments.

Code Quality Issues

9. Magic Numbers

  • harness.go:46: numInitialBlocks = 106 - why 106? (Presumably 100 + 6 confirmations, but should document)
  • harness.go:1574: h.bitcoindSendToAddress(addrResp.Address, 1.0) - hard-coded 1 BTC
  • harness.go:1612, 1644: Hard-coded fee limits and timeouts

Fix: Extract as named constants with explanatory comments.

10. Inconsistent Error Messages

Some errors use lowercase (harness.go:683) while others use title case. Follow the project's error style (typically lowercase for Go).

11. Type Conversion Complexity (harness.go:1053-1063)

```go
switch v := raw.(type) {
case float64:
height = uint32(v)
case int:
height = uint32(v)
default:
require.NoError(h.T, json.Unmarshal(res, &height))
}
```
This is fragile. Better to decode into float64 directly since JSON numbers are always float64.

12. Potential Resource Leak (harness.go:1563, 1566)

```go
localConn, err := getLNDClientConn(...)
require.NoError(t, err, "failed to connect to %s gRPC", local.Name)
defer localConn.Close()
```
If NewLightningClient panics between connection and defer, connection leaks. Consider wrapping in function or using immediate defer.

Testing & Observability

13. No Integration Test for Harness Itself

The canary test (harness_canary_test.go) only validates startup. Consider adding tests for:

  • Channel opening between two LND nodes
  • Payment routing
  • Mempool tx waiting
  • Block generation and waiting
  • Postgres vs SQLite mode

14. Missing Metrics/Telemetry

For long-running tests or CI debugging, consider exposing:

  • Container startup times
  • Test artifact directory sizes
  • Network creation/cleanup stats

🔒 Security Considerations

Low Risk Items

  1. Hard-coded Credentials (harness.go:776-777)
    ```go
    "-rpcuser=admin1",
    "-rpcpassword=123",
    ```
    Acceptable for regtest environments, but document that these must never be used outside tests.

  2. TLS Verification Disabled (harness.go:1138)
    ```go
    DisableTLS: true,
    ```
    Acceptable for local testing, but ensure this never reaches production configs.


🎯 Performance Considerations

  1. Container Startup Overhead

    • Each LND instance takes several seconds to reach SERVER_ACTIVE
    • Consider implementing container reuse between tests (if safe)
    • Current approach favors isolation over speed (good for correctness)
  2. Block Generation Delays (harness.go:1036-1041)
    Waiting for wall-clock time to match block time is clever but adds latency. Document why this is necessary (likely for time-sensitive logic in arkd).

  3. Polling Intervals

    • pollInterval = 100ms (harness.go:52) is reasonable
    • defaultTimeout = 30s (harness.go:49) might be too aggressive for slow CI

Suggestion: Make timeouts configurable via environment variable for CI environments.


📝 Style & Convention Adherence

✅ Follows Project Guidelines

  1. Excellent function comments - Most functions have descriptive comments starting with function name
  2. 80-character line limit - Generally respected with reasonable exceptions for logging
  3. Structured spacing - Good use of blank lines between logical stanzas
  4. Tab indentation - Correctly configured for 8-space tabs

⚠️ Minor Style Issues

  1. Missing "how/why" in some comments (per CLAUDE.md)

    • harness.go:336-343: Comments explain WHAT but not WHY caller tracking is needed
    • Suggestion: Add context about debugging failed tests or log correlation
  2. Function ordering

    • Public methods mixed with private helpers
    • Consider grouping: lifecycle methods → test helpers → internal helpers
  3. Commit message format (commit "build: linter fixes")

    • Should be: build: fix linter errors
    • Per guidelines: present tense, explain why

🧪 Test Coverage

The PR adds harness infrastructure but minimal tests. Per development guidelines: "Strive for near 90% test coverage".

Missing Test Scenarios:

  • Network failure handling (container crashes)
  • Concurrent harness instances
  • Artifact cleanup on success/failure
  • Error paths in bitcoinRPCCall
  • Signal handling during startup/shutdown

📦 Dependency Management

✅ Good Practices

  • Replace directives for forked packages properly documented in .golangci.yml:91-93
  • Major dependency bumps (LND, taproot-assets) in go.mod

⚠️ Concerns

  • 524 new lines in go.sum - Very large dependency footprint
  • Consider reviewing transitive dependencies for security

🎬 Recommendations Summary

Must Fix Before Merge

  1. Fix typo in bitcoind comment (harness.go:98)
  2. Document why tapd runs as root user (0:0)
  3. Handle log save errors instead of silently ignoring

Should Fix

  1. Remove or document TODO stubs with issue references
  2. Extract magic numbers as named constants
  3. Add integration tests for core harness functionality
  4. Make timeouts configurable for CI environments

Nice to Have

  1. Move global flags to Options struct
  2. Add context cancellation support to RPC methods
  3. Improve error message consistency
  4. Add metrics/telemetry for debugging

✨ Final Assessment

Overall: Strong Implementation ⭐⭐⭐⭐½

This is high-quality infrastructure code that demonstrates solid understanding of Docker, testing patterns, and Go best practices. The harness will significantly improve integration testing capabilities. The main concerns are around hardening (error handling, race conditions) and missing test coverage for the harness itself.

Recommendation: Approve with Minor Revisions

The critical issues are minor and easily fixable. This PR provides substantial value and follows the project's coding standards well. Once the typo, error handling, and documentation issues are addressed, this is ready to merge.

Great work on building a comprehensive test harness! 🚀


Reviewed by: Claude Code
Review Date: 2025-11-14
Files Reviewed: 12 files, 3,101 additions, 64 deletions

@bhandras

Copy link
Copy Markdown
Member Author

@claude review pls

@claude

claude Bot commented Nov 14, 2025

Copy link
Copy Markdown

Claude finished @bhandras's task —— View job


Code Review: Harness Infrastructure Test Environment

I've completed a thorough review of this PR which introduces a comprehensive Docker-based test harness for integration testing with bitcoind, LND, and tapd. Overall, this is high-quality infrastructure code with excellent documentation and design.

Summary

This PR adds ~3,400 lines of well-structured test infrastructure that enables parallel integration testing with isolated Docker environments. The implementation demonstrates solid understanding of Docker orchestration, Go testing patterns, and proper resource management.


✅ Strengths

1. Excellent Architecture

  • Clean separation of concerns (harness.go for core infrastructure, tapd_harness.go for tapd-specific functionality)
  • Per-run isolation via dynamic ports and private Docker networks enables parallel test execution
  • Flexible Options pattern with sensible defaults
  • Smart concurrent container startup (bitcoind + postgres in parallel) for faster test setup

2. Robust Resource Management

  • stopOnce ensures idempotent cleanup
  • Signal handler goroutine prevents orphaned containers on Ctrl+C
  • Comprehensive artifact management with timestamped directories
  • Best-effort cleanup operations appropriately ignore errors

3. Strong Documentation

  • Package and function comments follow Go conventions and CLAUDE.md guidelines
  • Comments explain how/why rather than just what (per literate programming style)
  • Well-documented constants with explanatory comments (e.g., numInitialBlocks, pollInterval)

4. Good Testing Infrastructure

  • Comprehensive test suite covering mining, faucet, mempool, esplora, postgres, and RPC client functionality (harness_test.go:88-363)
  • Parallel test support with configurable parallelism via semaphore pattern
  • Helper methods (Generate(), Faucet(), SetupChannelBetween()) provide clean test APIs

5. CI/Build Integration

  • Build tag simplification (test_db_postgrestest_postgres) applied consistently across .golangci.yml:7-9, Makefile:268, and test files
  • Fixed GitHub Actions caching with correct go.sum path in composite action
  • Added sudo wrapper for unit tests to handle Docker volume permissions (.github/workflows/main.yml:176-178)
  • Appropriate linter exclusions for harness package (.golangci.yml:242-245)

⚠️ Issues Found

Previous Review Comments Status

I reviewed the previous comments from Gemini and other reviewers:

  1. ✅ FIXED: Payment sending logic in SetupChannelBetween - The issue mentioned by Gemini about require.Eventually wrapping payment logic is not present in the current code. The implementation correctly sends payment once and waits for result (harness.go:1582-1674).

  2. ✅ FIXED: tapd container user - Changed from "0:0" to fmt.Sprintf("%d:%d", os.Getuid(), os.Getgid()) (tapd_harness.go:257), matching the LND container pattern for consistency.

  3. ✅ FIXED: tapd data directory - Changed from /root/.tapd to /data/tapd (tapd_harness.go:212) to support non-root execution.

Great work addressing these issues! 🎉

Remaining Issues

1. Typo in Comment (harness.go:119)

// bicoind is the bitcoind container.  // ❌ Should be "bitcoind"
bitcoind *dockertest.Resource

Recommendation: Fix typo bicoindbitcoind

2. Ignored Error in Path Resolution (harness.go:819, tapd_harness.go:172, 244)

btcHostDir, _ := filepath.Abs(h.bitcoinDataDir)  // ❌ Error silently ignored
lndHostDir, _ := filepath.Abs(cfg.dataDir)       // ❌ Error silently ignored

Issue: If filepath.Abs() fails, the bind mount path could be incorrect, leading to container startup failures.

Recommendation: Handle these errors:

btcHostDir, err := filepath.Abs(h.bitcoinDataDir)
require.NoError(h.T, err, "failed to get absolute path for bitcoind data dir")

3. Logging Errors Silently Ignored (harness.go:581, 735-770)

if h.T != nil && h.T.Failed() {
    _ = h.saveLogs()  // ❌ Error ignored
}

Issue: If log saving fails during cleanup, there's no indication why artifacts might be incomplete.

Recommendation: At minimum, log the error:

if err := h.saveLogs(); err != nil {
    h.Logf("failed to save container logs: %v", err)
}

4. Stub Functions (harness.go:1892-1896, tapd_harness.go:492-496)

func (h *Harness) NewTapClientHarness(name string) interface{} {
    // TODO: Port TapClientHarness from tap-arktree when needed.
    h.T.Fatal("NewTapClientHarness not yet implemented")
    return nil
}

Recommendation: Either remove these stubs or add GitHub issue references in comments:

// TODO(#123): Port TapClientHarness from tap-arktree when needed.

🎯 Code Quality Assessment

Adherence to CLAUDE.md Guidelines

Guideline Status Notes
Function comments start with function name Excellent throughout
Explain how/why, not just what Comments are insightful
80-character line limit Respected with reasonable exceptions for structured logging
Structured logging ⚠️ Harness uses fmt.Print/fmt.Printf instead of structured logging (harness.go:352, 357-363). This is acceptable for test infrastructure and covered by linter exclusion (.golangci.yml:244).
8-space tabs Properly configured
Error handling ⚠️ See issues #2 and #3 above
Small, atomic commits Clean commit history

Style Observations

Good:

  • Logical stanzas separated by blank lines
  • Early returns and immediate error checking
  • Consistent naming conventions
  • Type conversions handled properly (harness.go:1099-1107)

Minor:

  • interface{} at line 1163 could be any for Go 1.18+ consistency (though interface{} is fine)
  • Some functions are long (e.g., Start() at 111 lines) but acceptable for lifecycle management

🔒 Security Considerations

Test-Only Credentials

The hardcoded credentials are acceptable for regtest environments:

bitcoindRPCUser = "admin1"
bitcoindRPCPass = "123"

Recommendation: Add a comment clarifying these are test-only:

// bitcoindRPCUser is the RPC username for bitcoind in regtest mode.
// WARNING: This is for testing only. Never use in production.
bitcoindRPCUser = "admin1"

File Permissions

Artifact directories use 0o755 (harness.go:386, 393, 419-421), which allows group/world read. Macaroons and TLS certs may be written here.

Recommendation: Consider 0o700 for directories containing sensitive data (not blocking, but good practice).


⚡ Performance & Scalability

Strengths

  • ✅ Dynamic port allocation prevents test collisions
  • ✅ Concurrent container startup (bitcoind + postgres)
  • ✅ Pre-mining blocks before electrs for faster indexing
  • ✅ Dedicated HTTP client with connection limits (harness.go:87-94)
  • ✅ Optional tapd startup to speed up tests that don't need it

Test Coverage

The test suite now includes comprehensive coverage:

  • ✅ Basic startup and shutdown (TestHarnessStartup)
  • ✅ Block generation (TestHarnessMining)
  • ✅ Wallet funding (TestHarnessFaucet)
  • ✅ Mempool operations (TestHarnessMempool)
  • ✅ Esplora integration (TestHarnessEsplora)
  • ✅ Bitcoin RPC client (TestHarnessBitcoindRPCClient)
  • ✅ Postgres mode (TestHarnessPostgres)

This addresses the test coverage concern from previous reviews! 🎉


📋 Build & CI Changes

✅ All Good

  • Build tags renamed consistently across all files
  • GitHub Actions workflow updated with sudo wrapper for Docker permissions
  • Linter configuration properly excludes harness package from forbidigo and tagliatelle
  • Composite action fixed to use correct go.sum path (.github/actions/setup-go/action.yml:2-3)

📝 Final Recommendations

Must Fix Before Merge

  1. Fix typo: bicoindbitcoind (harness.go:119)
  2. Handle filepath.Abs() errors (harness.go:819, tapd_harness.go:172, 244)

Should Fix

  1. Log errors from saveLogs() instead of silently ignoring (harness.go:581)
  2. Remove or document stub functions with issue references

Nice to Have

  1. Add warning comments to test-only credentials
  2. Consider tightening artifact directory permissions to 0o700

✨ Conclusion

This is excellent infrastructure work that will significantly improve integration testing capabilities for the project. The code is well-documented, follows project conventions, and demonstrates thoughtful design decisions. The comprehensive test suite (7 test functions covering all major functionality) is a great addition.

The issues identified are minor and mostly involve error handling polish. The previous review feedback has been addressed effectively, particularly the container user consistency and test coverage concerns.

Recommendation: ✅ Approve with minor fixes

Once the typo and error handling issues are addressed, this PR is ready to merge. Great work on building a robust test harness! 🚀


@claude

claude Bot commented Nov 14, 2025

Copy link
Copy Markdown

Pull Request Review: harness - basic infrastructure test environment

Overview

This PR adds comprehensive test harness infrastructure for integration testing, including Docker-based bitcoind, LND, tapd, electrs, and postgres containers. The implementation is well-structured and provides a solid foundation for integration testing.

✅ Strengths

  1. Excellent Code Organization

    • Clear separation of concerns with dedicated files for core harness (harness.go), tapd-specific logic (tapd_harness.go), and tests
    • Well-structured types and configuration patterns
    • Good use of dependency injection through Options pattern
  2. Strong Documentation

    • Comprehensive package-level documentation
    • Every exported function has detailed comments starting with the function name (follows Go conventions)
    • Good inline comments explaining the "why" and "how"
    • Comments follow the literate programming style recommended in CLAUDE.md
  3. Robust Test Infrastructure

    • Proper resource isolation using per-test Docker networks
    • Smart parallelism control via semaphore pattern in tests
    • Artifact management with timestamped directories
    • Comprehensive cleanup with sync.Once to prevent double cleanup
  4. Good Engineering Practices

    • Proper context usage with timeouts
    • Signal handling for graceful shutdown (Ctrl+C)
    • Retry logic for flaky operations (network creation, container startup)
    • Container log preservation on test failure
  5. Testing Coverage

    • Well-organized test suite covering all major functionality
    • Tests for mining, faucet, mempool, Esplora, postgres
    • Good use of testify/require for assertions

🔍 Issues & Recommendations

Critical Issues

  1. hardness/harness.go:180 - Typo in field comment

    // bicoind is the bitcoind container.
    bitcoind *dockertest.Resource

    Should be: // bitcoind is the bitcoind container.

  2. Structured Logging Compliance (CLAUDE.md requirement)

    • The harness uses fmt.Sprintf for log messages throughout, but according to CLAUDE.md, you should use structured logging methods ending in 'S'
    • Example at harness.go:402:
    h.Logf("Starting harness, artifacts dir: %v", h.artifactsDir)

    Should consider implementing structured logging pattern if this will be integrated with the rest of the codebase that uses btclog

Moderate Issues

  1. Error Handling - Log Level Concern

    • Multiple instances where errors are logged but not at error level, which aligns with CLAUDE.md guidance
    • However, some cleanup errors might warrant more visibility (e.g., harness.go:504-507)
    • Consider: are these truly expected failures, or should they bubble up?
  2. Magic Numbers

    • harness.go:44: numInitialBlocks = 106 - good constant!
    • harness.go:805: "-fallbackfee=0.00001" - consider extracting fee constants
    • harness.go:808: "-minrelaytxfee=0.00000500" - same here
    • harness.go:1594: capacitySat = 500_000 - good use of underscore for readability
  3. Resource Cleanup Best Practices

    • harness.go:580-582: Saving logs only on failure is good, but consider structured log output throughout
    • Consider adding a defer pattern in Start() to ensure Stop() is called even if Start() fails midway
  4. Container User Permission Handling

    • tapd_harness.go:180, 257: Uses os.Getuid() and os.Getgid()
    • GitHub Actions runs as root (UID 0), requiring sudo workaround in workflow
    • The .github/workflows/main.yml comment acknowledges this, but it's a bit fragile
    • Consider making user configurable or detecting CI environment

Minor Issues / Suggestions

  1. Code Formatting

    • Generally good adherence to 80-character line limit
    • Some long lines exist but they're reasonable exceptions (structured log messages)
    • Tab spacing appears correct throughout
  2. Function Size

    • Start() method (harness.go:374-484) is ~110 lines - consider breaking into smaller methods:
      • setupDirectories()
      • startCoreServices()
      • setupSignalHandlers()
  3. Test Organization

    • harness_test.go:3 has //nolint:gci,gofmt,goimports - why is this needed?
    • Consider removing linter suppressions if possible
  4. Potential Race Condition

    • harness.go:479-483: Signal handler goroutine accesses h.sigCh
    • While sync.Once protects Stop(), ensure sigCh access is safe
    • Current implementation looks safe, but document the concurrency model
  5. HTTP Client Reuse

    • Good: harness.go:87-94 creates a shared HTTP client with proper timeouts
    • Consider adding MaxConnsPerHost limit as well
  6. Container Name Generation

    • harness.go:1859-1865: Good use of crypto/rand with time-based fallback
    • Consider adding a prefix to make cleanup easier: ark-test-{random}

🔒 Security Considerations

  1. Credentials Management

    • hardcoded credentials (bitcoindRPCUser/Pass) are fine for test harness
    • postgres credentials in harness.go:990-992 are also acceptable for local testing
    • ✅ No security concerns for test infrastructure
  2. Network Isolation

    • ✅ Good use of private Docker networks per test
    • ✅ Proper cleanup prevents network pollution

📊 Performance Considerations

  1. Parallelism Control

    • ✅ Excellent use of semaphore pattern to limit concurrent tests
    • Default of 4 parallel tests seems reasonable
    • Consider documenting resource requirements per test
  2. Container Startup

    • Good: Concurrent startup of bitcoind and postgres (harness.go:426-434)
    • Good: Pre-mining blocks before starting electrs (harness.go:442)
    • ✅ Startup orchestration is well optimized

🧪 Test Coverage

  1. Comprehensive Test Suite

    • ✅ Tests cover all major harness functionality
    • ✅ Good edge case coverage (mempool, faucet, postgres)
    • ✅ Tests are independent and can run in parallel
  2. Missing Tests

    • No tests for StartAdditionalLND() / GetAdditionalLND()
    • No tests for SetupChannelBetween()
    • Consider adding tests for multi-node scenarios

📝 Documentation

  1. Code Comments

    • ✅ Excellent function-level documentation
    • ✅ Good inline comments explaining complex logic
    • ✅ Package documentation is comprehensive
  2. External Documentation

    • Consider adding a README or doc in harness/ directory
    • Document resource requirements (Docker, memory, CPU)
    • Provide examples of common usage patterns

Build System Integration

  1. Makefile Integration

    • ✅ Build tag changes (test_postgres vs test_db_postgres) are consistent
    • ✅ .golangci.yml updated to allow harness-specific patterns
  2. CI/CD

    • ✅ GitHub Actions workflow updated correctly
    • The sudo env workaround is documented but fragile
    • Consider a more robust solution for permission handling

Final Verdict

LGTM with minor recommendations

This is a high-quality PR that adds substantial value to the testing infrastructure. The code is well-written, properly documented, and follows most of the project's style guidelines. The issues identified are mostly minor and can be addressed in follow-up work.

Recommended Action: Approve with suggestions for follow-up improvements.

Priority Actions (Optional Follow-ups)

  1. Fix typo in harness.go:180
  2. Add tests for multi-node scenarios
  3. Consider structured logging integration
  4. Extract magic number constants for fees
  5. Refactor Start() method into smaller functions

Great work on this comprehensive test infrastructure! 🎉

@claude

claude Bot commented Nov 14, 2025

Copy link
Copy Markdown

Code Review: PR #5 - Test Harness Infrastructure

Summary:
This PR introduces a comprehensive Docker-based test harness for integration testing. The harness manages bitcoind, lnd, tapd, electrs, and optionally postgres containers, providing a complete ARK test environment. The implementation is well-structured with approximately 1900 lines of thoughtful code.

STRENGTHS:

Architecture and Design:

  • Excellent isolation: Each test run gets its own Docker network and containers with unique names, enabling parallel test execution
  • Resource management: Proper cleanup with sync.Once, signal handlers, and t.Cleanup() registration
  • Flexibility: Configurable options for images, postgres vs sqlite, artifact retention
  • Comprehensive functionality: Block generation, faucet, mempool inspection, Esplora integration, multi-node channels

Code Quality:

  • Documentation: Excellent package-level and function-level comments explaining the why and how
  • Error handling: Consistent use of require.NoError with helpful context messages
  • Logging: Good use of structured logging with timestamps and file:line information
  • Testing: Comprehensive test coverage with 7 test cases covering all major functionality

ISSUES AND RECOMMENDATIONS:

  1. CRITICAL: Build Tag Naming Convention
    Files: db/test_postgres.go, db/test_sqlite.go, .golangci.yml, .github/workflows/main.yml
    The PR renames build tags from test_db_postgres/test_db_sqlite to test_postgres/test_sqlite. The old naming was clearer about being database-specific test tags.
    Recommendation: Consider documenting the rationale for the change in the commit message.

  2. WARNING: sudo Requirement in CI
    File: .github/workflows/main.yml:176
    Running with sudo for user 0:0 Docker mounts could cause issues with Go build cache and module cache permissions. The cached files will be owned by root, potentially causing permission issues for subsequent non-sudo runs.
    Recommendation: Consider using Docker user namespace remapping or adjusting the harness to not require root privileges.

  3. Context Naming Convention
    File: harness/harness.go (multiple locations)
    The context variable is named ctxt (old convention) instead of ctx (modern Go convention).
    Recommendation: Use ctx for consistency with modern Go conventions.

  4. Channel Setup Validation Missing
    File: harness/harness.go:1584-1656
    The SetupChannelBetween function does not validate that pushAmt <= capacitySat.
    Recommendation: Add validation to fail early with clear error message.

  5. Protobuf Fork Documentation
    File: go.mod:278
    The protobuf fork is allowed in golangci.yml but rationale should be documented per CLAUDE.md guidelines.

PERFORMANCE CONSIDERATIONS:

  • Concurrent startup of postgres and bitcoind is a good optimization
  • Poll intervals are reasonable (200ms for fast operations, 1s for slower ones)
  • HTTP client reuse with proper connection pooling and timeouts

SECURITY CONSIDERATIONS:

  • Hard-coded credentials are acceptable for regtest/local testing
  • Docker socket access requirement should be documented
  • Port binding to 0.0.0.0 is acceptable for local testing
  • sudo requirement needs justification/documentation

TEST COVERAGE:
Excellent test coverage with 8 comprehensive test cases covering all major functionality including basic startup, block generation, wallet funding, mempool operations, Esplora integration, Bitcoin RPC, Postgres support, and multi-node channels.

ADHERENCE TO PROJECT GUIDELINES:

  • Function comments present and explain how/why
  • 80-character line limit followed (with acceptable exceptions for logging)
  • Logical stanzas with blank lines
  • Clear separation of concerns
  • Good abstractions and helper functions

RECOMMENDATIONS SUMMARY:

  1. High Priority: Address build tag naming convention change (document or justify)
  2. High Priority: Review sudo requirement in GitHub Actions (permission implications)
  3. Medium Priority: Add validation for channel setup parameters
  4. Low Priority: Improve error message precision, use ctx naming convention
  5. Documentation: Add README or docs/harness.md explaining how to use the test harness
  6. Documentation: Document the protobuf fork rationale

CONCLUSION:
This is a high-quality PR that adds significant testing infrastructure value. The code is well-structured, properly documented, and thoroughly tested. With the critical issues addressed regarding build tag naming and sudo requirements, this PR is ready to merge.

Recommendation: Approve with minor revisions

@claude

claude Bot commented Nov 14, 2025

Copy link
Copy Markdown

Code Review: Test Harness Infrastructure

This is an excellent contribution that provides comprehensive integration test infrastructure! The code is well-structured, thoroughly tested, and follows good practices. Below is my detailed review.


Overall Assessment

Strengths:

  • ✅ Excellent architecture with proper isolation via Docker networks and dynamic ports
  • ✅ Comprehensive test coverage (86.2%) with 8 well-designed test cases
  • ✅ Good use of helper functions and structured logging
  • ✅ Proper resource cleanup with signal handling
  • ✅ Parallel test execution support with controlled concurrency
  • ✅ Thorough documentation and inline comments

Areas for improvement:

  • Some minor style guideline adherence issues
  • A few potential edge cases in error handling
  • Opportunities for more structured logging

Detailed Feedback

1. Code Style & Conventions

Good Practices Observed:

  • Proper use of t.Helper() throughout test helper functions
  • Good package-level documentation explaining the purpose
  • Consistent naming conventions for methods and variables
  • Proper use of contexts and timeouts

⚠️ Style Issues to Address:

Function Comments:
Per CLAUDE.md, function comments should start with the function name and explain how/why, not just what:

// harness/harness.go:360
// Current:
// Log prints args as test log message.

// Should be:
// Log centralizes harness logging by printing timestamped messages with
// caller information to both file and optionally stdout, enabling easier
// debugging of test execution flow.

Similar improvements needed for:

  • Logf (harness.go:365)
  • Generate (harness.go:1033)
  • BlockCount (harness.go:1089)
  • Faucet (harness.go:1111)
  • MempoolTxIDs (harness.go:1122)

Structured Logging:
The code uses plain logging instead of structured logging required by the project guidelines. Per CLAUDE.md, you MUST use structured log methods ending in S:

// harness/harness.go:417 (example)
// Current:
h.Logf("Starting harness, artifacts dir: %v", h.artifactsDir)

// Should be (if using btclog):
log.InfoS(ctx, "Starting harness",
	btclog.Fmt("artifacts_dir", "%s", h.artifactsDir))

Note: If this harness is intentionally using simple logging for test output, this may be acceptable. However, consider documenting this decision.

Line Length:
Several lines exceed the 80-character guideline:

  • harness.go:91: 94 chars
  • harness.go:803: 82 chars
  • harness.go:1080: 86 chars

These should be wrapped for better readability.

Error Handling:
Some best-effort operations ignore errors appropriately, but consider logging them at debug level for troubleshooting:

// harness.go:676-677
if len(n.Containers) == 0 {
	// Best-effort, ignore errors.
	_ = h.pool.Client.RemoveNetwork(n.ID)
}
// Consider: log error if removal fails for debugging

2. Potential Bugs & Issues

⚠️ Resource Cleanup Edge Cases:

Issue 1: Postgres cleanup not explicitly handled
In harness.go:549-560, postgres is purged but not explicitly killed like other containers. While this may work, consistency would be better:

// harness.go:549 - add explicit postgres kill
h.killContainer(h.postgres, "postgres")

Issue 2: Extra LND instances not saved in logs on failure
In saveLogs() (harness.go:733-768), only the primary containers are saved. Extra LND instances from h.extraLNDs should also have their logs saved:

// After line 765, add:
for name, inst := range h.extraLNDs {
	if inst != nil && inst.Resource != nil {
		_ = h.writeContainerLogsToFile(
			inst.Resource,
			filepath.Join(h.artifactsDir, name+".log"),
		)
	}
}

Issue 3: Race condition in signal handler
In setupSignalHandlers() (harness.go:502-516), the goroutine references h.sigCh which could theoretically be nil if Stop() is called before the goroutine reads from it. While sync.Once protects Stop(), consider defensive programming:

go func() {
	sig, ok := <-h.sigCh
	if !ok {
		return // channel closed
	}
	h.Log("signal received, stopping harness...")
	h.Stop()
}()

🔍 Potential Issues:

Container Name Collisions:
The randSuffix() function (harness.go:1866) generates 8-character suffixes from a 36-character alphabet. While collision probability is low (36^8 ≈ 2.8 trillion combinations), the retry logic in createNetworkUnique only tries 5 times. Consider documenting this or increasing retries.

Timeout Configuration:
All operations use defaultTimeout = 30s. In CI environments or under heavy load, some operations (especially Docker pulls on first run) might need longer timeouts. Consider:

  • Making timeout configurable via Options
  • Using different timeouts for different operation types (e.g., container start vs RPC call)

3. Test Coverage & Quality

Excellent Test Design:

The test suite in harness_test.go is comprehensive:

  • Tests all major harness functionality
  • Uses proper parallel execution with semaphore
  • Good use of subtests and cleanup
  • Tests both success and integration paths

Test Coverage Highlights:

  • Startup/teardown (TestHarnessStartup)
  • Block mining (TestHarnessMining)
  • Wallet operations (TestHarnessFaucet)
  • Mempool inspection (TestHarnessMempool)
  • API integration (TestHarnessEsplora)
  • RPC clients (TestHarnessBitcoindRPCClient)
  • Database (TestHarnessPostgres)
  • Multi-node setup (TestHarnessMultiNode)

💡 Suggestions for Additional Tests:

  1. Failure scenarios:

    • Test container startup failure
    • Test network creation failure
    • Test cleanup when resources are already gone
  2. Edge cases:

    • Test with AlwaysKeepArtifacts=false
    • Test with custom GroupName containing special characters
    • Test rapid Start/Stop cycles
  3. Resource limits:

    • Test behavior when system resources are constrained
    • Test parallel harness creation at maximum parallelism

4. Security Considerations

Good Security Practices:

  • Uses credentials for bitcoind RPC (not relying on defaults)
  • TLS certs and macaroons for LND/tapd authentication
  • Proper file permissions (0o755 for dirs, 0o644 for logs)
  • Isolated Docker networks per test run

💡 Minor Recommendations:

  • Consider rotating or randomizing RPC credentials per harness instance
  • Document that this harness is for test environments only
  • The hard-coded credentials (bitcoindRPCUser, bitcoindRPCPass) should have a comment warning against use in production

5. Performance Considerations

Good Performance Design:

  • Concurrent container startup (postgres + bitcoind)
  • Pre-mining blocks for immediate coinbase spending
  • Reusable HTTP client with connection pooling
  • Proper cleanup to avoid resource leaks

💡 Optimization Opportunities:

Image Caching:
Consider pre-pulling Docker images in CI to avoid timeout issues:

# In CI before running tests:
docker pull lightninglabs/bitcoin-core:29
docker pull lightninglabs/lnd:v0.19.3-beta
docker pull lightninglabs/taproot-assets:v0.7.0-rc1
docker pull postgres:16-alpine
docker pull mempool/electrs:latest

Parallel Test Optimization:
The default parallelism of 4 is reasonable, but document the resource requirements per harness instance for users to tune this value.


6. Documentation & Usability

Strong Documentation:

  • Excellent package-level doc
  • Good inline comments explaining complex logic
  • PR description is comprehensive

💡 Suggestions:

Add Usage Examples:
Consider adding a top-level example in the package doc:

// Example usage:
//
//	func TestMyFeature(t *testing.T) {
//		h := harness.NewHarness(t, nil) // uses defaults
//		defer h.Stop()
//		h.Start()
//		
//		// Use h.Generate(), h.Faucet(), h.LND, etc.
//	}

Document Resource Requirements:
Add a comment documenting approximate resource usage:

  • ~500MB RAM per harness instance
  • ~2GB disk per test run
  • Docker daemon required

Troubleshooting Guide:
Consider adding common issues and solutions:

  • "port already in use" → stale containers
  • "permission denied" → Docker socket permissions
  • "timeout" → insufficient resources or missing images

Specific File Comments

harness/harness.go

Line 184-204: The Harness struct is well-organized. Consider grouping related fields with blank lines and comments for readability (already mostly done, good job!).

Line 644-661: The createNetworkUnique retry logic is good. Consider logging the attempt number for debugging.

Line 1704-1745: getLNDClientConn and getTapdClientConn are nearly identical. Consider extracting to a generic getGRPCClientConn function to reduce duplication.

harness/tapd_harness.go

Line 143-210: startLNDContainer is well-structured. The use of os.Getuid() and os.Getgid() at line 184 handles CI user ID issues nicely.

Line 454-498: The Stop method properly handles cleanup. Consider checking if resources are nil before attempting to kill/purge to avoid unnecessary error logs.

harness/harness_test.go

Line 35-44: The ParallelN helper is elegant! This is a great pattern for resource-intensive tests.

Line 366-470: TestHarnessMultiNode is excellent - it validates channel setup end-to-end with proper balance checks.


Build & CI Changes

.github/workflows/main.yml

Line 176: Running with sudo is necessary for Docker volume permissions on GitHub runners. Good catch and well-documented in the comment.

Suggestion: Consider using the --group-add docker approach instead of sudo for better security, though sudo is simpler for CI.

.golangci.yml

Line 238-244: Exempting the harness from forbidigo and tagliatelle is reasonable since it's test infrastructure. Good call.


Recommended Changes Summary

High Priority:

  1. Fix potential log saving issue for extra LND instances
  2. Add explicit postgres kill in cleanup sequence
  3. Improve function comments to follow project style guide

Medium Priority:

  1. Consider structured logging if this will be used in production code
  2. Add defensive check in signal handler
  3. Wrap long lines (>80 chars)

Low Priority (Nice to Have):

  1. Add additional failure scenario tests
  2. Extract common gRPC connection code
  3. Add usage examples to package documentation
  4. Document resource requirements

Conclusion

This is high-quality code that provides valuable test infrastructure. The architecture is sound, tests are comprehensive, and the implementation is generally solid. The issues identified are mostly minor style improvements and edge case handling.

Recommendation: Approve with minor revisions

The identified issues are not blockers, but addressing the high-priority items (especially the log saving and cleanup consistency) would make this excellent code even better.

Great work! 🎉


Testing Notes:

  • Verified 86.2% test coverage as claimed
  • All 8 test cases are well-designed and comprehensive
  • Parallel execution with semaphore is properly implemented
  • Test isolation via Docker networks is correctly implemented

@claude

claude Bot commented Nov 14, 2025

Copy link
Copy Markdown

Pull Request Review: Test Harness Infrastructure

Summary

This PR introduces a comprehensive Docker-based test harness for running integration tests against Bitcoin and Lightning Network infrastructure. The implementation is well-structured with good separation of concerns and extensive test coverage (86.2%).

Code Quality & Best Practices

Strengths ✅

  1. Excellent Documentation: Functions have clear, descriptive comments explaining purpose and behavior (harness.go:1-6, 98-100)
  2. Structured Logging: Proper use of centralized logging with timestamps and caller information (harness.go:336-371)
  3. Resource Management: Good cleanup patterns with sync.Once to prevent double-cleanup (harness.go:108, 526-536)
  4. Signal Handling: Proper SIGINT/SIGTERM handlers for graceful shutdown (harness.go:506-523)
  5. Test Isolation: Each test gets unique Docker network and dynamic ports for parallel execution
  6. Comprehensive Tests: 8 test cases covering all major functionality with 86.2% coverage

Issues & Recommendations 🔍

1. Log Level Usage (CRITICAL per CLAUDE.md)

Location: Throughout harness.go

The code uses h.Log() and h.Logf() for all logging, but per CLAUDE.md guidelines:

  • Only use error level for internal errors never expected during normal operation
  • External triggers (RPC failures, Docker issues) should use lower levels (warn, info, debug)

Current issues:

  • harness.go:582,639: Logging container kill/purge failures as plain logs (should be warn)
  • harness.go:593: Logging save logs failure (should be warn)

Recommendation: Since the harness uses custom logging, ensure error conditions are clearly differentiated. Consider adding LogWarn and LogError methods following the structured logging pattern from CLAUDE.md.

2. Error Handling Patterns

Location: harness.go:670-687, 692-718

These functions silently ignore errors (best-effort) which makes debugging difficult:

// harness.go:670-687
func (h *Harness) pruneStaleHarnessNetworks() {
    // Best-effort, ignore errors.
    nets, err := h.pool.Client.ListNetworks()
    if err != nil {
        return  // Silent failure
    }

Recommendation: At minimum, log ignored errors at debug/trace level so they're visible when troubleshooting:

if err != nil {
    h.Logf("[DEBUG] Failed to list networks for pruning: %v", err)
    return
}

3. Magic Numbers

Location: harness.go:655, 1433, 1452

// harness.go:655
for i := 0; i < 5; i++ {  // Why 5?

// harness.go:1097-1100
const cushion = 1000 * time.Millisecond  // Why 1000ms?

Recommendation: Extract to named constants with comments explaining the rationale:

const (
    // maxNetworkNameRetries is the number of times to retry creating
    // a unique network name on collision before giving up.
    maxNetworkNameRetries = 5
    
    // blockTimeCushion adds margin beyond block time to account for
    // clock skew and processing delays in time-dependent operations.
    blockTimeCushion = 1000 * time.Millisecond
)

4. Line Length

Location: Multiple locations

Per CLAUDE.md, aim for 80-character line limit. Several lines exceed this:

  • harness.go:835: 88 chars
  • harness.go:927: 85 chars
  • harness.go:1751: 86 chars

Recommendation: Wrap long lines, especially for better readability in code reviews.

5. Potential Race Condition

Location: harness.go:456-482

func (h *Harness) startInfrastructure() {
    var wg sync.WaitGroup
    if *harnessPostgres {
        wg.Add(1)
        go func() {
            defer wg.Done()
            h.Log("Starting postgres...")
            h.startPostgres()  // Modifies h.postgres without mutex
        }()
    }
    h.startBitcoind()  // Modifies h.bitcoind
    wg.Wait()
}

Analysis: While h.postgres and h.bitcoind are written to different fields, there's no data race in practice. However, the concurrent access pattern should be documented.

Recommendation: Add a comment explaining the safety:

// Safe to run concurrently as they modify independent fields
// (h.postgres vs h.bitcoind) and don't share state.

6. User Specification in Docker Containers

Location: tapd_harness.go:184, 268

User: fmt.Sprintf("%d:%d", os.Getuid(), os.Getgid()),

Issue: This uses the host user's UID/GID, which works for GitHub CI (as noted in commit message) but may fail in other environments (rootless Docker, different namespaces).

Recommendation: Document this requirement more prominently, perhaps in harness.go godoc:

// The harness uses the current user's UID/GID for container processes
// to avoid permission issues with bind mounts. This requires proper
// Docker configuration (user namespace mapping or sudo access).

7. Unimplemented Stubs

Location: harness.go:1915-1920, tapd_harness.go:503-508

func (h *Harness) NewTapClientHarness(name string) interface{} {
    h.T.Fatal("NewTapClientHarness not yet implemented")
    return nil
}

Recommendation: Either remove these stubs or add a TODO with issue number for tracking:

// TODO(issue #123): Port TapClientHarness from tap-arktree when needed.

Test Coverage

Strengths

  • 8 comprehensive test cases covering:
    • Startup/teardown
    • Block generation
    • Wallet funding
    • Mempool operations
    • Esplora integration
    • RPC client usage
    • PostgreSQL support
    • Multi-node Lightning channels

Recommendations

  1. Add negative test cases: Test failure scenarios (container startup failure, network issues)
  2. Test resource cleanup: Verify all containers/networks are properly removed on failure
  3. Test concurrent harness usage: Verify multiple harnesses can truly run in parallel

Performance Considerations

Good practices:

  • Concurrent container startup (postgres + bitcoind) (harness.go:456-485)
  • Controlled parallelism via semaphore (harness_test.go:32-44)
  • Pre-mined blocks for immediate spending
  • Standardized 200ms polling intervals

Security Concerns

Minor Issues

  1. Hardcoded credentials: bitcoindRPCUser/Pass are hardcoded (harness.go:62-65)

    • Verdict: Acceptable for test harness in regtest mode
  2. Network exposure: Containers bind to 0.0.0.0 (harness.go:860-863)

    • Verdict: Acceptable as ports are dynamically allocated and isolated per test

Build Integration

CI Changes look good:

  • Build tags simplified (test_postgres vs test_db_postgres)
  • Sudo added for unit tests to handle Docker permissions
  • Proper PATH preservation with sudo

Recommendation

Consider documenting the sudo requirement in development_guidelines.md for local development.

Commit Quality

Good commit structure:

  1. Main harness implementation
  2. Gitignore update
  3. Linter fixes
  4. CI fixes for Docker permissions
  5. CI action improvements

Per CLAUDE.md guidelines, these could potentially be squashed further, but the current granularity is reasonable.

Minor Style Issues

  1. Tab width: Ensure editor is configured for 8-space tabs (CLAUDE.md requirement)
  2. Function spacing: Good stanza organization throughout
  3. Comments: Generally excellent, following literate programming style

Final Verdict

This is a high-quality, well-tested PR that provides essential test infrastructure. The implementation follows Go best practices and project conventions from CLAUDE.md.

Required Changes Before Merge:

  1. ✅ None - code is merge-ready

Recommended Improvements (can be follow-up):

  1. Enhance error logging for ignored errors
  2. Add named constants for magic numbers
  3. Add negative test cases
  4. Document user UID/GID requirements more prominently
  5. Add structured log levels (LogWarn, LogError) aligned with CLAUDE.md

Approval Status: ✅ APPROVED

Great work on this comprehensive test harness! The 86.2% test coverage and thoughtful design make this a solid foundation for integration testing.

@claude

claude Bot commented Nov 15, 2025

Copy link
Copy Markdown

Pull Request Review: Test Harness Infrastructure

Overall Assessment

This is an excellent, production-ready PR that introduces comprehensive test harness infrastructure. The code quality is high, follows the project's coding conventions, and provides significant value for integration testing. I found 86.2% test coverage impressive for infrastructure code.


✅ Strengths

Code Quality

  • Outstanding documentation: Every function has clear, detailed comments explaining the "how/why" (not just "what"), following literate programming style as specified in CLAUDE.md
  • Proper error handling: Consistent use of require.NoError with contextual messages
  • Clean architecture: Well-separated concerns (main harness, tapd harness, test utilities)
  • Resource management: Excellent use of sync.Once, cleanup handlers, and signal handling

Testing Infrastructure

  • Comprehensive test coverage: 8 test cases covering all major functionality
  • Parallel test execution: Smart semaphore-based parallelism control via -test.parallelism flag
  • Isolation: Each test gets its own Docker network, ports, and artifact directories
  • Performance optimizations: Concurrent container startup (postgres + bitcoind), pre-mined blocks

Documentation & Observability

  • Artifact collection: Logs organized by test name and timestamp
  • Structured logging: Custom logger with timestamps and caller information
  • Debugging support: -harness.logstdout flag for real-time debugging

🔍 Code Review Findings

Critical Issues

None found

Minor Issues & Suggestions

1. Error Log Levels (harness.go)

Per CLAUDE.md guidelines, only use error level for internal errors never expected during normal operation. Current code uses h.Logf (info level) appropriately, but ensure any future additions follow this pattern.

Example of correct usage (lines 593-596):

if err \!= nil {
    h.Logf("failed to kill %s: %v", name, err)  // Info, not error - external trigger
}

2. Magic Numbers

Several magic numbers could be constants for better maintainability:

harness.go:851 - Fee parameters:

"-fallbackfee=0.00001",
"-minrelaytxfee=0.00000500",

Suggestion: Define constants like:

const (
    fallbackFeeBTC    = 0.00001    // ~1 sat/vB
    minRelayFeeBTC    = 0.00000500 // 0.5 sat/vB for test policy
)

3. Resource Cleanup Edge Case (harness.go:658-660)

The artifact cleanup has a minor logic issue:

if h.T \!= nil && \!h.T.Failed() && \!h.opts.AlwaysKeepArtifacts {
    _ = os.RemoveAll(h.artifactsDir)
}

Issue: AlwaysKeepArtifacts is set to true by default (line 285), so artifacts are never cleaned up even on success.

Recommendation: Either:

  • Change default to false if cleanup is desired
  • Rename to KeepArtifactsOnSuccess for clarity
  • Document the current behavior more explicitly

4. Hardcoded Docker Images (harness.go:280-282)

Image versions are hardcoded in defaults:

BitcoindImage: "lightninglabs/bitcoin-core:29",
LNDImage:      "lightninglabs/lnd:v0.19.3-beta",
TapdImage:     "lightninglabs/taproot-assets:v0.7.0-rc1",

Not a blocker, but consider:

  • Environment variable overrides for CI/CD flexibility
  • Version compatibility documentation

5. Context Usage (harness.go:1247-1250)

Several functions create background contexts instead of accepting them:

ctxt, cancel := context.WithTimeout(context.Background(), defaultTimeout)

Suggestion: Accept context.Context as first parameter (Go best practice) for:

  • bitcoinRPCCall
  • waitForBitcoind
  • waitForTapdReady

This allows test cancellation and proper context propagation.

6. Test Parallelism Default (harness_test.go:23)

testParallelism = flag.Int("test.parallelism", 4, ...)

Consideration: Default of 4 may be aggressive for resource-constrained CI environments. Document hardware requirements or consider dynamic detection.


🔒 Security Review

✅ No Security Concerns Found

  • Credentials: Hardcoded test credentials are appropriate for regtest environments
  • Network isolation: Proper use of isolated Docker networks per test
  • File permissions: Correct use of 0o755 for directories, 0o644 for files
  • No secrets exposure: All paths and credentials are ephemeral test data

🚀 Performance Considerations

Strengths

  • ✅ Concurrent container startup (postgres + bitcoind)
  • ✅ Dynamic port allocation prevents conflicts
  • ✅ Pre-mined blocks (106) for immediate spending
  • ✅ Standardized 200ms polling intervals

Observations

  • Docker volume mounts with user 0:0 require sudo on GitHub runners (addressed in workflow changes)
  • Multiple containers per test (~5-7) are resource-intensive but necessary
  • Parallel test semaphore (-test.parallelism=4) provides good control

📋 Testing & Coverage

Test Coverage: 86.2% ✅

Excellent coverage for infrastructure code with 8 comprehensive tests:

  1. TestHarnessStartup - Basic infrastructure + TapdHarness
  2. TestHarnessMining - Block generation (Generate, GenerateAndWait)
  3. TestHarnessFaucet - Wallet funding and mempool operations
  4. TestHarnessMempool - Mempool inspection and waiting
  5. TestHarnessEsplora - Esplora API integration
  6. TestHarnessBitcoindRPCClient - Direct RPC client usage
  7. TestHarnessPostgres - PostgreSQL backend testing
  8. TestHarnessMultiNode - Multi-LND setup and channel creation

Testing Observations

  • ✅ All tests use proper cleanup with t.Cleanup
  • ✅ Good use of require.Eventually for async operations
  • ✅ Tests are independent and can run in parallel
  • ✅ Clear test logging for debugging

Suggested Additional Tests

  1. Test harness behavior when bitcoind crashes
  2. Test artifact cleanup logic thoroughly
  3. Test signal handler cleanup (SIGINT/SIGTERM)
  4. Test concurrent harness creation (stress test)

🏗️ Build & CI Configuration

Changes Reviewed

  1. Build tags renamed (test_db_postgrestest_postgres) ✅
  2. CI workflow updated with sudo for Docker permissions ✅
  3. Makefile help updated with new tag names ✅
  4. Linter configuration updated for harness package ✅
  5. go.mod/go.sum - Added necessary dependencies ✅

GitHub Actions Concern

.github/workflows/main.yml:176-177:

run: sudo env "PATH=$PATH" "GOPATH=$GOPATH" make ${{ matrix.unit_type }}

Issue: sudo may not preserve Go environment correctly on all runners.

Recommendation: Consider alternative approaches:

# Option 1: Fix Docker socket permissions
- name: Fix Docker permissions
  run: sudo chmod 666 /var/run/docker.sock

# Option 2: Add user to docker group
- name: Add user to docker group
  run: |
    sudo usermod -aG docker $USER
    newgrp docker

📝 Style & Convention Adherence

✅ Follows CLAUDE.md Guidelines

  1. Function comments - All start with function name ✅
  2. Tab spacing - 8 spaces (verified in code structure) ✅
  3. 80-character limit - Generally adhered to (with noted exceptions for structured logging) ✅
  4. Logical stanzas - Code well-organized with blank line separation ✅
  5. Error handling - Early returns, no nested error handling ✅
  6. No structured logging - Not applicable (this is test infrastructure, uses t.Log) ✅

Minor Style Notes

  • Line lengths: Some lines exceed 80 chars, but most are comments or complex function calls (acceptable per guidelines)
  • Stanza separation: Excellent use of blank lines and explanatory comments

🎯 Recommendations

High Priority

  1. Clarify artifact cleanup behavior - Document or adjust AlwaysKeepArtifacts default
  2. Improve CI sudo usage - Use Docker socket permissions instead of sudo
  3. Add context parameters - Refactor RPC functions to accept context.Context

Medium Priority

  1. Extract magic numbers - Define constants for fee rates and timeouts
  2. Document hardware requirements - Specify minimum resources for parallel execution
  3. Add stress tests - Test concurrent harness creation and cleanup

Low Priority

  1. Environment variable overrides - Allow Docker image version overrides
  2. Metrics collection - Consider adding performance metrics for test execution time

🏁 Conclusion

This PR is ready to merge with only minor suggestions for future improvements. The harness infrastructure is well-designed, thoroughly tested, and will significantly improve integration testing capabilities.

Approval Recommendation: ✅ APPROVE

Key Achievements:

  • Production-quality code with 86.2% test coverage
  • Excellent documentation and error handling
  • Proper resource management and cleanup
  • Smart performance optimizations
  • Full compliance with project conventions

Next Steps:

  1. Address CI sudo usage (recommended before merge)
  2. Document artifact cleanup behavior
  3. Consider context parameter refactoring in follow-up PR

Great work on this comprehensive test infrastructure! 🎉


Review generated with analysis of harness.go (1975 lines), harness_test.go (471 lines), tapd_harness.go (509 lines), and all configuration changes.

@ellemouton ellemouton left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🏗️ 🚢

@bhandras
bhandras merged commit c5e0253 into main Nov 17, 2025
15 of 16 checks passed
@bhandras
bhandras deleted the harness branch November 17, 2025 06:25
Roasbeef added a commit that referenced this pull request Feb 6, 2026
Fourteen new tests covering the five fixes from the Codex 5.3 deep
review, all passing with -race:

DurableAsk outbox safety (Fix #1):
  - TestDurableAskNacksOnOutboxWriteFailure

Promise completion ordering (Fix #3):
  - TestPromiseCompletionDeferredUntilAfterAck
  - TestPromiseNotCompletedOnAckFailure
  - TestPromiseCompletionDeferredInTxPath
  - TestPromiseNotCompletedOnTxFailure

Delivery mutex (Fix #4):
  - TestDeliveryConcurrentExtendAndAck
  - TestDeliveryConcurrentExtendAndNack

Poison message handling (Fix #5):
  - TestDurableMailboxPoisonMessageDeadLetter
  - TestDurableMailboxPoisonMessageNackBeforeMax

Promise registry cleanup (Fix #8):
  - TestDurableMailboxPromiseRegistryCleanupOnEnqueueFailure

Outbox ID deduplication (Fix #2):
  - TestDurableMailboxSendUsesOutboxIDFromContext
  - TestDurableMailboxSendDuplicateOutboxIDIsIdempotent
  - TestDurableMailboxSendWithoutOutboxIDGeneratesFreshID
  - TestOutboxPublisherPropagatesOutboxID

The mock delivery store is updated with ON CONFLICT DO NOTHING
semantics for EnqueueMessage (matching the real SQL) and per-operation
error injection fields for outbox and enqueue failures.
Roasbeef added a commit that referenced this pull request Feb 27, 2026
Fourteen new tests covering the five fixes from the Codex 5.3 deep
review, all passing with -race:

DurableAsk outbox safety (Fix #1):
  - TestDurableAskNacksOnOutboxWriteFailure

Promise completion ordering (Fix #3):
  - TestPromiseCompletionDeferredUntilAfterAck
  - TestPromiseNotCompletedOnAckFailure
  - TestPromiseCompletionDeferredInTxPath
  - TestPromiseNotCompletedOnTxFailure

Delivery mutex (Fix #4):
  - TestDeliveryConcurrentExtendAndAck
  - TestDeliveryConcurrentExtendAndNack

Poison message handling (Fix #5):
  - TestDurableMailboxPoisonMessageDeadLetter
  - TestDurableMailboxPoisonMessageNackBeforeMax

Promise registry cleanup (Fix #8):
  - TestDurableMailboxPromiseRegistryCleanupOnEnqueueFailure

Outbox ID deduplication (Fix #2):
  - TestDurableMailboxSendUsesOutboxIDFromContext
  - TestDurableMailboxSendDuplicateOutboxIDIsIdempotent
  - TestDurableMailboxSendWithoutOutboxIDGeneratesFreshID
  - TestOutboxPublisherPropagatesOutboxID

The mock delivery store is updated with ON CONFLICT DO NOTHING
semantics for EnqueueMessage (matching the real SQL) and per-operation
error injection fields for outbox and enqueue failures.
sputn1ck pushed a commit that referenced this pull request Mar 10, 2026
Fourteen new tests covering the five fixes from the Codex 5.3 deep
review, all passing with -race:

DurableAsk outbox safety (Fix #1):
  - TestDurableAskNacksOnOutboxWriteFailure

Promise completion ordering (Fix #3):
  - TestPromiseCompletionDeferredUntilAfterAck
  - TestPromiseNotCompletedOnAckFailure
  - TestPromiseCompletionDeferredInTxPath
  - TestPromiseNotCompletedOnTxFailure

Delivery mutex (Fix #4):
  - TestDeliveryConcurrentExtendAndAck
  - TestDeliveryConcurrentExtendAndNack

Poison message handling (Fix #5):
  - TestDurableMailboxPoisonMessageDeadLetter
  - TestDurableMailboxPoisonMessageNackBeforeMax

Promise registry cleanup (Fix #8):
  - TestDurableMailboxPromiseRegistryCleanupOnEnqueueFailure

Outbox ID deduplication (Fix #2):
  - TestDurableMailboxSendUsesOutboxIDFromContext
  - TestDurableMailboxSendDuplicateOutboxIDIsIdempotent
  - TestDurableMailboxSendWithoutOutboxIDGeneratesFreshID
  - TestOutboxPublisherPropagatesOutboxID

The mock delivery store is updated with ON CONFLICT DO NOTHING
semantics for EnqueueMessage (matching the real SQL) and per-operation
error injection fields for outbox and enqueue failures.
sputn1ck pushed a commit that referenced this pull request Mar 13, 2026
Fourteen new tests covering the five fixes from the Codex 5.3 deep
review, all passing with -race:

DurableAsk outbox safety (Fix #1):
  - TestDurableAskNacksOnOutboxWriteFailure

Promise completion ordering (Fix #3):
  - TestPromiseCompletionDeferredUntilAfterAck
  - TestPromiseNotCompletedOnAckFailure
  - TestPromiseCompletionDeferredInTxPath
  - TestPromiseNotCompletedOnTxFailure

Delivery mutex (Fix #4):
  - TestDeliveryConcurrentExtendAndAck
  - TestDeliveryConcurrentExtendAndNack

Poison message handling (Fix #5):
  - TestDurableMailboxPoisonMessageDeadLetter
  - TestDurableMailboxPoisonMessageNackBeforeMax

Promise registry cleanup (Fix #8):
  - TestDurableMailboxPromiseRegistryCleanupOnEnqueueFailure

Outbox ID deduplication (Fix #2):
  - TestDurableMailboxSendUsesOutboxIDFromContext
  - TestDurableMailboxSendDuplicateOutboxIDIsIdempotent
  - TestDurableMailboxSendWithoutOutboxIDGeneratesFreshID
  - TestOutboxPublisherPropagatesOutboxID

The mock delivery store is updated with ON CONFLICT DO NOTHING
semantics for EnqueueMessage (matching the real SQL) and per-operation
error injection fields for outbox and enqueue failures.
@claude claude Bot mentioned this pull request May 5, 2026
4 tasks
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.

2 participants