test: strengthen weak assertions and add presentation layer coverage#586
test: strengthen weak assertions and add presentation layer coverage#586carlos-alm merged 3 commits intomainfrom
Conversation
Strengthen 8 tests that used only toBeDefined() as their sole assertion — add type checks, property assertions, and value validations so regressions are caught. Add 56 new tests covering the previously untested presentation layer: - table.ts: formatTable alignment, padding, truncation, truncEnd edge cases - colors.ts: node/role/community color constant validation - result-formatter.js: JSON, NDJSON, CSV, table output dispatch + CSV escaping - queries-cli: inspect, path, exports, overview modules via mocked domain data
|
Claude finished @carlos-alm's task —— View job Code Review Complete ✅Reviewing PR #586: test improvements and presentation layer coverage. Root Cause AnalysisProblem Being Solved: This PR addresses test quality issues — 8 tests that used only Why This Approach: The approach is sound — strengthening weak assertions and adding missing test coverage is exactly what should be done. The "false positive cleanup" note about temp dir tests indicates good analysis. Risk Assessment: Very low risk. Pure test additions with no behavior changes to production code. Backlog Compliance
Review Summary✅ Strengths
✅ Technical Quality
🔍 Minor Observations
Final Recommendation
This is exactly the kind of foundational quality work that should be approved quickly and serve as a template for future test improvements. |
Greptile SummaryThis PR strengthens 8 previously weak Changes by area:
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant T as Test (queries-cli.test.js)
participant V as Vitest (vi.mock)
participant CLI as inspect.js / path.js / exports.js / overview.js
participant DM as domain/queries.js (mocked)
participant RF as result-formatter.js (mocked)
Note over T,V: vi.mock hoisted before imports
V->>DM: register mock (mocks object)
V->>RF: register mock (outputResult → false)
T->>CLI: await import(inspect.js)
CLI->>DM: (resolved via mock)
CLI->>RF: (resolved via mock)
T->>T: beforeEach — spy console.log, capture lines[]
T->>DM: mocks.whereData.mockReturnValue({...})
T->>CLI: where('foo', '/db')
CLI->>DM: whereData('foo', '/db', {})
DM-->>CLI: { mode: 'symbol', results: [...] }
CLI->>RF: outputResult(data, 'results', {}) → false
CLI->>CLI: console.log(formatted output)
CLI-->>T: lines[] captured via spy
T->>T: expect(output()).toContain(...)
T->>T: afterEach — vi.restoreAllMocks()
Reviews (2): Last reviewed commit: "fix: address review feedback — negative ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| file: 'lib.js', | ||
| totalExported: 3, | ||
| totalInternal: 0, | ||
| totalUnused: 1, | ||
| results: [ |
There was a problem hiding this comment.
symbolPath error test doesn't assert result returns after printing
The printNotFound/error path in symbolPath uses console.log(data.error) and then an early return, but the test only checks that the error string was printed — it does not assert that subsequent rendering code (path steps, etc.) was not called. This leaves the door open for a future regression where the function continues past the error branch.
Consider adding a negative assertion to pin this:
| file: 'lib.js', | |
| totalExported: 3, | |
| totalInternal: 0, | |
| totalUnused: 1, | |
| results: [ | |
| it('prints error when present', () => { | |
| mocks.pathData.mockReturnValue({ error: 'Symbol not found' }); | |
| symbolPath('x', 'y', '/db'); | |
| expect(output()).toContain('Symbol not found'); | |
| // ensure we stopped after the error — no path-rendering output | |
| expect(output()).not.toContain('Path from'); | |
| }); |
There was a problem hiding this comment.
Added the negative assertion \ to ensure the error branch exits before any path-rendering output. Committed in dd29a60.
| callees: [{ kind: 'function', name: 'ast', file: 'a.js', line: 1, summary: 'parse AST' }], | ||
| callers: [{ kind: 'function', name: 'main', file: 'm.js', line: 10 }], | ||
| implementors: [], |
There was a problem hiding this comment.
vi.mock('../../src/infrastructure/result-formatter.js', ...) works today because inspect.js, path.js, exports.js, and overview.js all import outputResult from ../../infrastructure/result-formatter.js — which is a thin backward-compat re-export of src/presentation/result-formatter.js.
If a future refactor moves those four files to import directly from the presentation path (the canonical location), this mock will silently stop working and the tests will start hitting real I/O without any indication. Mocking the canonical path is safer:
| callees: [{ kind: 'function', name: 'ast', file: 'a.js', line: 1, summary: 'parse AST' }], | |
| callers: [{ kind: 'function', name: 'main', file: 'm.js', line: 10 }], | |
| implementors: [], | |
| vi.mock('../../src/presentation/result-formatter.js', () => ({ | |
| outputResult: () => false, // never short-circuit — always render CLI output | |
| })); |
There was a problem hiding this comment.
Added a second vi.mock targeting the canonical presentation/result-formatter.js path alongside the existing compat re-export mock. This way, if the source files are ever refactored to import from the canonical path directly, the tests will still intercept correctly. Committed in dd29a60.
…exible palette count (#586)
|
Addressed Greptile's feedback in dd29a60:
All tests pass locally. |
1 similar comment
1 participant
Summary
toBeDefined()now also validate types, properties, and valuestests/presentation/covering 4 new test files:table.test.js—formatTablealignment, padding, truncation;truncEndedge casescolors.test.js— node/role/community color constant completeness and hex format validationresult-formatter.test.js— JSON, NDJSON, CSV, table output dispatch; CSV escaping; object flatteningqueries-cli.test.js—inspect.js(where, queryName, context, children, explain, implementations, interfaces),path.js,exports.js,overview.js(moduleMap, roles) via mocked domain dataincr-edge-gap,incremental-edge-parity,resolution/parity) actually use try/finally cleanup — no changes neededTest plan
npx vitest run tests/presentation/)