[test, ignore] harness: basic infrastructure test environment

[bhandras]

No description provided.

[claude]

Pull Request Review: Test Harness Infrastructure

This PR introduces comprehensive integration test infrastructure. Overall, this is high-quality work with excellent documentation, thoughtful design, and good test coverage.

✅ Strengths

Code Quality

• Excellent documentation with thorough package and function comments
• Well-structured code with clear separation of concerns
• Robust error handling with descriptive messages
• Resource cleanup using sync.Once for idempotent cleanup
• Parallel test support with semaphores and dynamic ports

Test Coverage

• Comprehensive test suite covering startup, mining, faucet, mempool, esplora, multi-node, postgres
• Good test organization with ParallelN helper
• Real-world scenarios including multi-node channel setup

🔍 Issues Found

1. Critical: Tab Spacing

Location: All files - Run make fmt before committing per CLAUDE.md

2. Style: Function Comments

Location: harness.go:304, 323 - Comments must start with function name

3. Bug: Signal Handler Race

Location: harness.go:502-516 - Check channel close to avoid panic

4. Style: Line Length

Location: lines 280, 433, 818 - Exceed 80 chars

5. Bug: randSuffix Error Handling

Location: harness.go:1866 - Should panic on crypto RNG failure

6. Missing: Structured Logging

Location: Throughout - CLAUDE.md requires slog (or document exemption)

7. Code Organization

Location: harness.go:1830 - Group helper functions

8. Test Coverage

Location: harness_test.go - Add negative test cases

9. Security Comment Needed

Location: harness.go:62 - Document why hard-coded creds are OK

10. Error Messages

Check consistency of lowercase formatting

📊 Summary

• Critical: 1 (tab spacing)
• Bugs: 2 (signal race, RNG)
• Style: 3 (comments, length, logging)
• Suggestions: 4

✅ Must Fix Before Merge

1. Run make fmt
2. Fix signal handler race
3. Update function comments
4. Fix randSuffix error handling

🎯 Assessment

Recommendation: Approve with changes

Excellent infrastructure code. Issues are mostly style-related. Core logic is sound.

Estimated effort: 1-2 hours

Great work! 🚀

[claude]

Pull Request Review: Test Harness Infrastructure

Overview

This PR adds a comprehensive integration test harness for darepo-client that manages Docker containers for bitcoind, LND, tapd, electrs, and PostgreSQL. The infrastructure provides ~2,900 lines of well-structured test framework code.

Code Quality Assessment

✅ Strengths

1. Excellent Documentation: Every function has comprehensive comments explaining the 'how' and 'why', following the literate programming style from CLAUDE.md
2. Well-Organized Structure: Code is logically separated into stanzas with clear explanatory comments
3. Proper Resource Management: Signal handlers, cleanup, and Docker resource lifecycle management are thorough
4. Parallel Test Support: Smart use of semaphores to limit concurrent test execution
5. Comprehensive Test Coverage: 10 tests covering mining, faucet, mempool, Esplora, multi-node setups, and PostgreSQL

🔍 Style Compliance Issues

Critical: Tab Width

The codebase appears to use 8-space tabs per CLAUDE.md. Please verify your editor is configured correctly with tab = 8 spaces.

Logging Style Violations

Found several instances of non-structured logging that violate CLAUDE.md guidelines:

harness.go:

• Lines 345-356: Using fmt.Sprintf in log messages instead of structured logging
• Should use log.InfoS(ctx, "message", slog.String("key", value)) pattern
• Multiple instances of h.Logf throughout that should be converted to structured logging with btclog

Examples to fix:

// WRONG (line 417):
h.Logf("Starting harness, artifacts dir: %v", h.artifactsDir)

// RIGHT:
log.InfoS(ctx, "Starting harness",
    slog.String("artifacts_dir", h.artifactsDir))

Error Log Level Issues

Line 575-578, 606-607: Using error-level logging for container kill failures. Per CLAUDE.md:

Only use error level for internal errors never expected during normal operation

Container kill failures during cleanup are not internal errors—they're expected external events. These should be warn or info level.

🐛 Potential Bugs

1. harness.go:1870-1877: Fallback random suffix uses time-based generation when RNG fails, but the algorithm is weak (sequential indices). Consider using time.Now().UnixNano() with proper modulo.

2. harness.go:714-730: waitContainerRunning uses pool.Retry without explicit timeout. While dockertest has internal timeouts, this could hang if misconfigured.

3. harness_test.go:29: Global testParallelismSem channel is never closed. While not a memory leak (process exits), consider closing in TestMain cleanup for completeness.

4. tapd_harness.go:184: os.Getuid() may not work correctly on Windows. Consider adding build tags or runtime checks if cross-platform support is needed.

⚠️ Potential Issues

1. Race Condition: harness.go:505-515: Signal handler goroutine accesses h.sigCh without synchronization. While Stop() uses sync.Once, the channel close in disableSignalHandlers could race with the signal receive.

2. Resource Leak: harness.go:346-349: h.harnessLogFile.WriteString errors are checked but the file stays open. If writes fail repeatedly, this could accumulate buffered data.

3. Context Usage: Multiple functions create context.Background() instead of using test context. Go 1.21+ tests provide t.Context() which is cancelled when the test completes—use that instead.

4. Hard-coded Timeouts: defaultTimeout = 30 * time.Second may be too short for CI environments with limited resources. Consider making this configurable via environment variable.

🔒 Security Concerns

1. Credentials in Code: bitcoindRPCUser="admin1", bitcoindRPCPass="123" are hard-coded. While this is regtest-only, consider using generated random credentials per test run to establish better security practices.

2. File Permissions: Several os.MkdirAll calls use 0o755. For directories containing private keys/macaroons, consider 0o700 (harness.go:444-446).

3. No TLS Verification: harness.go:1182: DisableTLS: true for Bitcoin RPC. Expected for regtest, but add a comment explaining why.

🎯 Performance Considerations

1. Sequential Startup: harness.go:452-481: Only postgres and bitcoind start concurrently. Consider parallelizing LND and tapd startup where possible.

2. Polling Intervals: pollInterval = 200ms is reasonable, but some operations (like container startup) could use adaptive backoff.

3. HTTP Client Reuse: Good practice using a shared harnessHTTPClient (harness.go:88-95).

📝 Test Coverage Concerns

1. Missing Error Cases: Tests primarily cover happy paths. Consider adding tests for:

   • Container startup failures
   • Network isolation verification
   • Cleanup under error conditions
   • Port collision handling

2. No Negative Tests: All tests expect success. Add tests that verify proper error handling when bitcoind/LND fail.

🔧 Suggestions for Improvement

1. Add Context Throughout: Pass context.Context to all major functions for better cancellation support:

func (h *Harness) Start(ctx context.Context) { ... }

2. Extract Magic Numbers: Lines like 106 (numInitialBlocks), 500_000 (channel capacity) should be constants with explanatory comments.

3. Improve Error Messages: Many errors just wrap without adding context. Example:

// CURRENT
return nil, fmt.Errorf("failed to create network: %w", err)

// BETTER  
return nil, fmt.Errorf("failed to create network %s: %w", name, err)

4. Add Metrics/Instrumentation: Consider logging timing information for slow operations (container startup, block generation) to help diagnose CI issues.

5. Separate Concerns: harness.go is 1,920 lines. Consider splitting into:

   • harness_docker.go - Docker/container management
   • harness_bitcoin.go - Bitcoin-specific operations
   • harness_logging.go - Logging infrastructure

Testing Before Merge

Please run:

# Verify code compiles and formats correctly
make build
make fmt
make lint

# Run unit tests
make unit pkg=harness timeout=5m

# Run tests with debug logging to verify structured logging
make unit-debug log="stdlog trace" pkg=harness timeout=10s

Check logs carefully to ensure:

• No [ERR] lines appear unless testing error paths
• Structured logging format is correct
• No log spam

Dependencies

The PR adds significant dependencies (109 additions to go.mod). Most look appropriate for LND/tapd integration, but verify:

• All transitive dependencies are expected
• No known vulnerabilities (go list -m -json all | nancy sleuth or similar)
• License compatibility

Verdict

⚠️ CONDITIONAL APPROVAL - Changes Required

The infrastructure is solid and well-designed, but must address:

1. ✅ Convert all logging to structured logging (required by CLAUDE.md)
2. ✅ Fix error log levels (external failures should not be error level)
3. ✅ Verify 8-space tab configuration
4. ⚠️ Add context cancellation support
5. ⚠️ Fix signal handler race condition

The code demonstrates excellent organization and testing practices. After addressing the structured logging violations and error log levels (both explicit requirements in CLAUDE.md), this will be ready to merge.

Recommendations for Follow-up PRs

1. Add integration tests that verify error handling
2. Extract large functions and split harness.go
3. Add performance benchmarks for test startup time
4. Document expected resource usage (memory, disk, CPU)

Great work on the comprehensive test infrastructure! 🚀