test: add coverage for internal/analytics + binder-ambiguity fix (#8) - #16
Conversation
First tests for the analytics package, covering: - Exporter.Export — parquet round-trip produces a non-empty file - SessionsParquetHasSource — missing file / fresh file / synthetic legacy file (schema-compat detection added in PR #6) - GetSummary / GetTokensByDay / GetTopProjects / GetTokensByModel — full query coverage over a multi-source fixture; aggregations verified against known totals - GetTokensBySource / GetSessionsBySource — multi-source aggregation - Missing-cache error paths for all query methods Includes a synthetic legacyParquetRecord (pre-#6 schema without the source column) written directly via parquet-go to exercise the schema-compat detection and the legacy-parquet source-query error path. ## Incidental fix (queries.go) The COALESCE(source, 'claude-code') fallback in GetTokensBySource and GetSessionsBySource was aspirational: on a legacy parquet without a source column, the previous SQL SELECT COALESCE(source, ...) as source FROM read_parquet(...) GROUP BY COALESCE(source, ...) triggered a confusing DuckDB binder error about aliases being illegal in GROUP BY expressions, because DuckDB shadowed the missing column with the SELECT alias named `source`. Renamed the alias to `source_out` and switched to `GROUP BY 1` so the error surface is now honest: "Referenced column source not found in FROM clause" — actionable for anyone bypassing the SessionsParquetHasSource + rebuild guard in the TUI. rows.Scan is positional so the Go-side struct field mapping is unchanged. Fixes #8
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAnalytics tests now use SQLite, Parquet, and DuckDB fixtures. They verify exports, schema detection, aggregations, legacy-cache behavior, and missing-cache errors. Source queries now use explicit aliases and positional grouping for legacy Parquet compatibility. ChangesAnalytics coverage and compatibility
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Summary
Closes #8.
First tests for the `internal/analytics` package. Also carries a small incidental fix to `queries.go` that the tests surfaced.
What's tested
Real SQLite fixture → real parquet export → real DuckDB queries. No mocks.
The legacy-parquet tests use a synthetic `legacySessionRecord` written directly via `parquet-go` to simulate a pre-multi-source cache.
Incidental fix
The `COALESCE(source, 'claude-code') as source ... GROUP BY COALESCE(source, ...)` fallback in `GetTokensBySource`/`GetSessionsBySource` was aspirational: on a legacy parquet without a source column, DuckDB's binder shadowed the missing column with the SELECT alias named `source` and errored inside the COALESCE with a confusing message.
Renamed the alias to `source_out` and switched to `GROUP BY 1` so the error is now the honest "Referenced column source not found in FROM clause." `rows.Scan` is positional, so Go-side struct mapping is unchanged.
The COALESCE fallback still can't rescue legacy parquets (DuckDB errors at bind-time regardless), but callers hit the `SessionsParquetHasSource + rebuild` guard in the TUI before running these queries. The test suite documents this.
Test plan
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by CodeRabbit
Bug Fixes
Tests