Test/estimate coverage #68

Merged
noe.trevino merged 2 commits from test/estimate-coverage into main 2026-02-13 16:07:58 -06:00
Owner
No description provided.
github-actions[bot] commented 2026-02-13 01:28:34 -06:00 (Migrated from github.com)

PR Summary (auto-generated)

  • Removed unused development files: command_tree_test.go (command structure validation tests), config.example.yaml (example configuration), fix_parallel.py and remove_subtest_parallel.py (test parallelization fix scripts)
  • Added comprehensive architecture documentation (docs/ARCHITECTURE.md, 895 lines) covering system overview, project layout, request lifecycle, all architectural layers, key patterns (IoC, Handler, Adapter, Context Injection), testing strategy, and guide for adding new CLI commands
  • Documented dual-interface architecture (CLI/TUI), database abstraction layer (SQLite/PostgreSQL via SQLC), service layer patterns, and testing infrastructure including coverage strategy (same-package tests for accurate metrics)
  • Cleaned up repository by removing temporary tooling scripts used during parallel test refactoring
  • No functional code changes; purely documentation and cleanup
**PR Summary** _(auto-generated)_ - Removed unused development files: command_tree_test.go (command structure validation tests), config.example.yaml (example configuration), fix_parallel.py and remove_subtest_parallel.py (test parallelization fix scripts) - Added comprehensive architecture documentation (docs/ARCHITECTURE.md, 895 lines) covering system overview, project layout, request lifecycle, all architectural layers, key patterns (IoC, Handler, Adapter, Context Injection), testing strategy, and guide for adding new CLI commands - Documented dual-interface architecture (CLI/TUI), database abstraction layer (SQLite/PostgreSQL via SQLC), service layer patterns, and testing infrastructure including coverage strategy (same-package tests for accurate metrics) - Cleaned up repository by removing temporary tooling scripts used during parallel test refactoring - No functional code changes; purely documentation and cleanup
claude[bot] (Migrated from github.com) reviewed 2026-02-13 01:28:47 -06:00
@ -0,0 +225,4 @@
}
```
Falls back to `fmt.Stringer`, then `%+v` dump.
claude[bot] (Migrated from github.com) commented 2026-02-13 01:28:47 -06:00

Line reference is fragile and will break as the file evolves. Consider removing specific line numbers from documentation, or use relative references like "near the top of the file" or anchor to function names instead.

Line reference is fragile and will break as the file evolves. Consider removing specific line numbers from documentation, or use relative references like "near the top of the file" or anchor to function names instead.
claude[bot] (Migrated from github.com) reviewed 2026-02-13 01:28:48 -06:00
@ -0,0 +332,4 @@
```go
type UpdateTaskRequest struct {
TaskID int
Title *string // nil means don't update
claude[bot] (Migrated from github.com) commented 2026-02-13 01:28:48 -06:00

Another fragile line reference. Same issue throughout the document - line numbers in documentation create maintenance burden and will become incorrect as code changes.

Another fragile line reference. Same issue throughout the document - line numbers in documentation create maintenance burden and will become incorrect as code changes.
claude[bot] (Migrated from github.com) reviewed 2026-02-13 01:28:50 -06:00
@ -0,0 +335,4 @@
Title *string // nil means don't update
Description *string
PriorityID *int
TypeID *int
claude[bot] (Migrated from github.com) commented 2026-02-13 01:28:50 -06:00

The value "138 lines" is metadata that will become stale. Remove file size metrics from documentation.

The value "138 lines" is metadata that will become stale. Remove file size metrics from documentation.
claude[bot] (Migrated from github.com) reviewed 2026-02-13 01:28:53 -06:00
@ -0,0 +369,4 @@
3. **Factory** (`factory.go`, line 53-77):
```go
func NewQuerier(db any, dbType DatabaseType) (Querier, error)
claude[bot] (Migrated from github.com) commented 2026-02-13 01:28:53 -06:00

Same package testing is mentioned here as a coverage optimization, but this is actually a code smell. You're gaming coverage metrics rather than writing proper integration tests. Tests in the same package can access private implementation details, creating brittle tests that break when refactoring internals that should be hidden.

Same package testing is mentioned here as a coverage optimization, but this is actually a code smell. You're gaming coverage metrics rather than writing proper integration tests. Tests in the same package can access private implementation details, creating brittle tests that break when refactoring internals that should be hidden.
claude[bot] (Migrated from github.com) reviewed 2026-02-13 01:28:54 -06:00
@ -0,0 +592,4 @@
type createHandler struct{}
func (h *createHandler) Execute(ctx context.Context, args *Arguments) (any, error) {
cliInstance, _ := cli.GetCLIFromContext(ctx)
claude[bot] (Migrated from github.com) commented 2026-02-13 01:28:54 -06:00

Another fragile line reference (228 lines). This pattern repeats extensively throughout - all these line counts will require constant maintenance.

Another fragile line reference (228 lines). This pattern repeats extensively throughout - all these line counts will require constant maintenance.
claude[bot] (Migrated from github.com) reviewed 2026-02-13 01:28:56 -06:00
@ -0,0 +796,4 @@
1. **Create command file** (`internal/cli/task/my_command.go`):
```go
func MyCommandCmd() *cobra.Command {
claude[bot] (Migrated from github.com) commented 2026-02-13 01:28:56 -06:00

The "33.2% coverage" metric is a point-in-time value that will become incorrect. Don't hardcode metrics in documentation.

The "33.2% coverage" metric is a point-in-time value that will become incorrect. Don't hardcode metrics in documentation.
claude[bot] (Migrated from github.com) reviewed 2026-02-13 01:29:19 -06:00
@ -0,0 +778,4 @@
func TestParseAssignArgs(t *testing.T) { /* ... */ }
```
**Result**: Coverage metrics reflect actual tested code. Went from 0% → 33.2% on CLI task package.
claude[bot] (Migrated from github.com) commented 2026-02-13 01:29:19 -06:00

Same-package testing is being promoted as the solution here, but this is a testing anti-pattern. If ParseAssignArgs needs to be tested, it should be a public function in a service package, not a private utility in a command package. This approach creates fragile tests that couple to implementation details.

Same-package testing is being promoted as the solution here, but this is a testing anti-pattern. If `ParseAssignArgs` needs to be tested, it should be a public function in a service package, not a private utility in a command package. This approach creates fragile tests that couple to implementation details.
claude[bot] (Migrated from github.com) reviewed 2026-02-13 01:29:20 -06:00
@ -0,0 +706,4 @@
type TaskWriter interface {
CreateTask(ctx, req) (*Task, error)
UpdateTask(ctx, req) error
DeleteTask(ctx, taskID) error
claude[bot] (Migrated from github.com) commented 2026-02-13 01:29:20 -06:00

Fragile file count metadata. Don't hardcode "60+ fields" - this will become incorrect.

Fragile file count metadata. Don't hardcode "60+ fields" - this will become incorrect.
claude[bot] (Migrated from github.com) reviewed 2026-02-13 01:29:22 -06:00
@ -0,0 +735,4 @@
2. **Integration tests** (within service/database packages):
- Use in-memory SQLite via `fixtures.SetupTestDB()`
- Test service methods with real database operations
claude[bot] (Migrated from github.com) commented 2026-02-13 01:29:22 -06:00

More fragile metadata - "360 lines", "20+ update/view files", etc. All these counts create maintenance burden.

More fragile metadata - "360 lines", "20+ update/view files", etc. All these counts create maintenance burden.
claude[bot] (Migrated from github.com) reviewed 2026-02-13 01:29:25 -06:00
@ -0,0 +219,4 @@
Errors: `{"success": false, "error": {"code": "...", "message": "..."}}`
3. **Human-readable** (default): Checks if data implements `PrettyPrintable`:
```go
claude[bot] (Migrated from github.com) commented 2026-02-13 01:29:26 -06:00

The document title says "Paso Architecture" but much of the content is prescriptive how-to guides (see sections like "Adding a New CLI Command"). This creates confusion about the document's purpose. Architectural documentation should describe the system as-is, not teach readers how to extend it. Consider splitting this into separate docs: ARCHITECTURE.md for system design and CONTRIBUTING.md or DEVELOPMENT.md for development guides.

The document title says "Paso Architecture" but much of the content is prescriptive how-to guides (see sections like "Adding a New CLI Command"). This creates confusion about the document's purpose. Architectural documentation should describe the system as-is, not teach readers how to extend it. Consider splitting this into separate docs: ARCHITECTURE.md for system design and CONTRIBUTING.md or DEVELOPMENT.md for development guides.
claude[bot] (Migrated from github.com) reviewed 2026-02-13 01:29:34 -06:00
@ -0,0 +353,4 @@
---
### Database Layer (`internal/database/`)
claude[bot] (Migrated from github.com) commented 2026-02-13 01:29:34 -06:00

The handler pattern adds abstraction without clear benefit. Compare:

Handler pattern (current):

type createHandler struct{}
func (h *createHandler) Execute(ctx, args) (any, error)
RunE: handler.Command(&createHandler{}, parseCreateFlags)

Direct RunE:

RunE: func(cmd *cobra.Command, args []string) error { ... }

The handler pattern trades 3 lines and a type definition for what? The Arguments wrapper and auto-formatting could be utility functions. Unless there's a compelling reason (polymorphic command execution, middleware chains), prefer the simpler direct approach.

The handler pattern adds abstraction without clear benefit. Compare: **Handler pattern (current)**: ```go type createHandler struct{} func (h *createHandler) Execute(ctx, args) (any, error) RunE: handler.Command(&createHandler{}, parseCreateFlags) ``` **Direct RunE**: ```go RunE: func(cmd *cobra.Command, args []string) error { ... } ``` The handler pattern trades 3 lines and a type definition for what? The `Arguments` wrapper and auto-formatting could be utility functions. Unless there's a compelling reason (polymorphic command execution, middleware chains), prefer the simpler direct approach.
claude[bot] (Migrated from github.com) reviewed 2026-02-13 01:29:37 -06:00
@ -0,0 +399,4 @@
### Models (`internal/models/`)
**Purpose**: Plain data transfer objects with no database dependencies.
claude[bot] (Migrated from github.com) commented 2026-02-13 01:29:37 -06:00

The Arguments struct with methods like MustGetString, GetString is implementing a string-typed map with getters. This loses compile-time type safety. Why not parse flags directly into a typed struct? The current approach defers validation and makes typos in flag names a runtime error instead of a compile error.

The `Arguments` struct with methods like `MustGetString`, `GetString` is implementing a string-typed map with getters. This loses compile-time type safety. Why not parse flags directly into a typed struct? The current approach defers validation and makes typos in flag names a runtime error instead of a compile error.
claude[bot] (Migrated from github.com) reviewed 2026-02-13 01:29:41 -06:00
@ -0,0 +460,4 @@
Databases []DatabaseConfig `yaml:"databases"`
ActiveDatabase string `yaml:"active_database,omitempty"`
ActiveAssignee string `yaml:"active_assignee,omitempty"`
}
claude[bot] (Migrated from github.com) commented 2026-02-13 01:29:41 -06:00

Exit code 3 for "resource not found" and exit code 4 for "invalid data" are non-standard. Standard practice is:

  • 0: success
  • 1: general errors
  • 2: misuse of shell command (invalid arguments)

Custom exit codes beyond this are rarely useful and create confusion. Users checking $? expect the standard codes. If you need detailed error information, use stderr messages, not exit codes.

Exit code 3 for "resource not found" and exit code 4 for "invalid data" are non-standard. Standard practice is: - 0: success - 1: general errors - 2: misuse of shell command (invalid arguments) Custom exit codes beyond this are rarely useful and create confusion. Users checking `$?` expect the standard codes. If you need detailed error information, use stderr messages, not exit codes.
claude[bot] (Migrated from github.com) reviewed 2026-02-13 01:29:45 -06:00
@ -0,0 +547,4 @@
func FormatAssignJSON(result *AssignResult) map[string]any
```
Command handler becomes thin orchestration:
claude[bot] (Migrated from github.com) commented 2026-02-13 01:29:45 -06:00

Pointer fields for optional values is correct, but the request struct pattern shown here should be questioned. Consider:

type UpdateTaskRequest struct {
    TaskID      int      // Required
    Title       *string  // Optional
    Description *string  // Optional
    PriorityID  *int     // Optional
}

This creates a "bag of fields" that allows nonsensical states. What if all optional fields are nil? That's a no-op update. Better to use the functional options pattern or separate request types for different update operations (UpdateTitle, UpdatePriority, etc.) to make illegal states unrepresentable.

Pointer fields for optional values is correct, but the request struct pattern shown here should be questioned. Consider: ```go type UpdateTaskRequest struct { TaskID int // Required Title *string // Optional Description *string // Optional PriorityID *int // Optional } ``` This creates a "bag of fields" that allows nonsensical states. What if all optional fields are nil? That's a no-op update. Better to use the functional options pattern or separate request types for different update operations (UpdateTitle, UpdatePriority, etc.) to make illegal states unrepresentable.
claude[bot] (Migrated from github.com) reviewed 2026-02-13 01:29:50 -06:00
@ -0,0 +857,4 @@
}
```
4. **Register command** (`internal/cli/task/task.go`):
claude[bot] (Migrated from github.com) commented 2026-02-13 01:29:50 -06:00

The adapter pattern here is over-engineered. You have:

  1. SQLC-generated code per dialect
  2. Adapters that wrap generated code
  3. Converters (convert_to/convert_from)
  4. A factory that dispatches

That's 4 layers of indirection to support two databases. The complexity cost is high. Consider: is dual-database support actually needed? If SQLite is for local and PostgreSQL for remote, could you pick one and simplify? The adapter pattern is appropriate when you have many backends or need to swap at runtime, but for two known databases, this might be YAGNI.

The adapter pattern here is over-engineered. You have: 1. SQLC-generated code per dialect 2. Adapters that wrap generated code 3. Converters (convert_to/convert_from) 4. A factory that dispatches That's 4 layers of indirection to support two databases. The complexity cost is high. Consider: is dual-database support actually needed? If SQLite is for local and PostgreSQL for remote, could you pick one and simplify? The adapter pattern is appropriate when you have many backends or need to swap at runtime, but for two known databases, this might be YAGNI.
claude[bot] (Migrated from github.com) reviewed 2026-02-13 01:30:04 -06:00
@ -0,0 +888,4 @@
- **SQLC docs**: https://docs.sqlc.dev/
- **Cobra docs**: https://cobra.dev/
- **Bubble Tea tutorial**: https://github.com/charmbracelet/bubbletea
claude[bot] (Migrated from github.com) commented 2026-02-13 01:30:04 -06:00

Functional options are being used here, but the example shows only 3-4 options with clear defaults. This pattern shines when you have many options or need to maintain backward compatibility as options grow. For a handful of options, a config struct is simpler:

type AppConfig struct {
    EventPublisher events.EventPublisher
    DatabaseType   database.DatabaseType
    GitDetector    git.Detector
}

func New(db *sql.DB, cfg AppConfig) (*App, error)

Direct and explicit. The functional options pattern is valuable for libraries with evolving APIs, but for internal code, it's often overengineering.

Functional options are being used here, but the example shows only 3-4 options with clear defaults. This pattern shines when you have many options or need to maintain backward compatibility as options grow. For a handful of options, a config struct is simpler: ```go type AppConfig struct { EventPublisher events.EventPublisher DatabaseType database.DatabaseType GitDetector git.Detector } func New(db *sql.DB, cfg AppConfig) (*App, error) ``` Direct and explicit. The functional options pattern is valuable for libraries with evolving APIs, but for internal code, it's often overengineering.
claude[bot] (Migrated from github.com) reviewed 2026-02-13 01:30:09 -06:00
@ -0,0 +746,4 @@
4. **Acceptance tests**:
- `tests/acceptance/cli_validation.sh` - Shell-based CLI validation
### Test Infrastructure
claude[bot] (Migrated from github.com) commented 2026-02-13 01:30:09 -06:00

The "IoC for Testability" pattern shows extracting pure functions into *_logic.go files to get them tested. This is backward. The problem isn't lack of testability - it's that business logic is in the wrong layer.

Instead of:

CLI handler → ParseAssignArgs() → service call → FormatAssignOutput()

It should be:

CLI handler (thin) → service.AssignTask() (thick, well-tested) → format output

The parsing and validation logic should be in the service layer where it can be tested properly through service tests. The CLI layer should be trivially thin. If you're extracting *_logic.go files from CLI commands, that's a symptom that your service layer is too thin.

The "IoC for Testability" pattern shows extracting pure functions into `*_logic.go` files to get them tested. This is backward. The problem isn't lack of testability - it's that business logic is in the wrong layer. Instead of: ``` CLI handler → ParseAssignArgs() → service call → FormatAssignOutput() ``` It should be: ``` CLI handler (thin) → service.AssignTask() (thick, well-tested) → format output ``` The parsing and validation logic should be in the service layer where it can be tested properly through service tests. The CLI layer should be trivially thin. If you're extracting `*_logic.go` files from CLI commands, that's a symptom that your service layer is too thin.
claude[bot] (Migrated from github.com) reviewed 2026-02-13 01:30:28 -06:00
@ -0,0 +627,4 @@
```go
// SQLite: returns sqlite.Adapter wrapping generated_sqlite.Queries
// PostgreSQL: returns postgres.Adapter wrapping generated_postgres.Queries
```
claude[bot] (Migrated from github.com) commented 2026-02-13 01:30:28 -06:00

These magic numbers (1, 2, 3 for relation types; 1, 2 for task types; 1-5 for priorities) should not be constants. Using integers for categories is a premature optimization that makes the database harder to query and understand. SELECT * FROM tasks WHERE type = 'feature' is far more maintainable than WHERE type = 2.

If you need numeric IDs for performance, use a lookup table with foreign keys, not hardcoded constants in Go code.

These magic numbers (1, 2, 3 for relation types; 1, 2 for task types; 1-5 for priorities) should not be constants. Using integers for categories is a premature optimization that makes the database harder to query and understand. `SELECT * FROM tasks WHERE type = 'feature'` is far more maintainable than `WHERE type = 2`. If you need numeric IDs for performance, use a lookup table with foreign keys, not hardcoded constants in Go code.
claude[bot] (Migrated from github.com) reviewed 2026-02-13 01:30:32 -06:00
@ -0,0 +277,4 @@
**`App` struct** (line 19-35):
```go
type App struct {
claude[bot] (Migrated from github.com) commented 2026-02-13 01:30:32 -06:00

The request lifecycle diagram shows the happy path clearly, but architecture documentation should emphasize failure modes:

  • What happens on validation errors at each layer?
  • Where do transactions begin and end?
  • What's the error propagation/wrapping strategy?
  • How are database connection errors handled?

The happy path is easy; the failure paths define the architecture.

The request lifecycle diagram shows the happy path clearly, but architecture documentation should emphasize failure modes: - What happens on validation errors at each layer? - Where do transactions begin and end? - What's the error propagation/wrapping strategy? - How are database connection errors handled? The happy path is easy; the failure paths define the architecture.
claude[bot] commented 2026-02-13 01:30:55 -06:00 (Migrated from github.com)

Review Summary

This PR adds comprehensive architecture documentation (895 lines) while removing obsolete test files and scripts. The documentation has several significant issues:

Documentation Quality Issues

1. Fragile References Throughout

  • Line numbers (e.g., "line 228", "line 335") are hardcoded everywhere and will become incorrect as code changes
  • File size metrics ("138 lines", "360 lines") will become stale
  • Coverage percentages ("33.2%") are point-in-time values that don't belong in documentation

2. Document Purpose Confusion

  • Title says "Architecture" but contains extensive how-to content ("Adding a New CLI Command" is 80+ lines)
  • Mixes architectural principles with step-by-step tutorials
  • Should be split into ARCHITECTURE.md (system design) and CONTRIBUTING.md (development guides)

3. Missing Critical Architecture Details

  • Only documents happy paths, not failure modes
  • No discussion of transaction boundaries
  • No error propagation strategy
  • No explanation of how database connection errors are handled

Architectural Pattern Concerns

1. Testing Anti-Pattern
The document promotes same-package testing (e.g., docs/ARCHITECTURE.md:372, 781, 990) as a solution to get coverage on CLI code. This reveals a deeper problem: business logic is in the wrong layer. If you need to test private functions in command packages, it means the service layer is too thin.

2. Over-Engineering

  • Handler pattern (docs/ARCHITECTURE.md:356) adds abstraction without clear benefit
  • 4-layer database abstraction (docs/ARCHITECTURE.md:860) for just 2 databases may be YAGNI
  • Functional options pattern (docs/ARCHITECTURE.md:891) used when a simple config struct would suffice

3. Type Safety Issues

  • Arguments struct with string keys (docs/ARCHITECTURE.md:402) loses compile-time type safety
  • Should parse flags directly into typed structs

4. Non-Standard Patterns

  • Custom exit codes (docs/ARCHITECTURE.md:463) beyond 0, 1, 2 are non-standard and create confusion
  • Magic number constants (docs/ARCHITECTURE.md:630) instead of strings or lookup tables make DB harder to query

5. Incorrect Interface Segregation
The example at docs/ARCHITECTURE.md:914 isn't true interface segregation - it's just organizing methods. Segregation requires clients to depend only on methods they use, not just splitting interfaces that are still composed back together.

Recommendations

  1. Remove all fragile references: line numbers, file sizes, coverage percentages
  2. Split the document: ARCHITECTURE.md for design, CONTRIBUTING.md for how-tos
  3. Document failure modes: error handling, transactions, edge cases
  4. Reconsider the architecture: Business logic should be in services, not CLI command helpers
  5. Simplify patterns: Only use abstraction patterns when complexity is justified

The deleted files (test files, scripts, example config) are fine to remove.

## Review Summary This PR adds comprehensive architecture documentation (895 lines) while removing obsolete test files and scripts. The documentation has several significant issues: ### Documentation Quality Issues **1. Fragile References Throughout** - Line numbers (e.g., "line 228", "line 335") are hardcoded everywhere and will become incorrect as code changes - File size metrics ("138 lines", "360 lines") will become stale - Coverage percentages ("33.2%") are point-in-time values that don't belong in documentation **2. Document Purpose Confusion** - Title says "Architecture" but contains extensive how-to content ("Adding a New CLI Command" is 80+ lines) - Mixes architectural principles with step-by-step tutorials - Should be split into ARCHITECTURE.md (system design) and CONTRIBUTING.md (development guides) **3. Missing Critical Architecture Details** - Only documents happy paths, not failure modes - No discussion of transaction boundaries - No error propagation strategy - No explanation of how database connection errors are handled ### Architectural Pattern Concerns **1. Testing Anti-Pattern** The document promotes same-package testing (e.g., docs/ARCHITECTURE.md:372, 781, 990) as a solution to get coverage on CLI code. This reveals a deeper problem: business logic is in the wrong layer. If you need to test private functions in command packages, it means the service layer is too thin. **2. Over-Engineering** - Handler pattern (docs/ARCHITECTURE.md:356) adds abstraction without clear benefit - 4-layer database abstraction (docs/ARCHITECTURE.md:860) for just 2 databases may be YAGNI - Functional options pattern (docs/ARCHITECTURE.md:891) used when a simple config struct would suffice **3. Type Safety Issues** - Arguments struct with string keys (docs/ARCHITECTURE.md:402) loses compile-time type safety - Should parse flags directly into typed structs **4. Non-Standard Patterns** - Custom exit codes (docs/ARCHITECTURE.md:463) beyond 0, 1, 2 are non-standard and create confusion - Magic number constants (docs/ARCHITECTURE.md:630) instead of strings or lookup tables make DB harder to query **5. Incorrect Interface Segregation** The example at docs/ARCHITECTURE.md:914 isn't true interface segregation - it's just organizing methods. Segregation requires clients to depend only on methods they use, not just splitting interfaces that are still composed back together. ### Recommendations 1. **Remove all fragile references**: line numbers, file sizes, coverage percentages 2. **Split the document**: ARCHITECTURE.md for design, CONTRIBUTING.md for how-tos 3. **Document failure modes**: error handling, transactions, edge cases 4. **Reconsider the architecture**: Business logic should be in services, not CLI command helpers 5. **Simplify patterns**: Only use abstraction patterns when complexity is justified The deleted files (test files, scripts, example config) are fine to remove.
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
noe.trevino/paso!68
No description provided.