Bryce
791e41cf34
- Added comprehensive test suite for account creation, validation, and grid views - Documented Datomic entity reference handling patterns in test comments - Created 5 test improvement todo documents addressing common test anti-patterns - Todo items cover: removing debug statements, fixing test nesting, strengthening assertions, extracting helpers, and removing doc-only tests
108 lines
3.2 KiB
Markdown
108 lines
3.2 KiB
Markdown
---
|
|
status: complete
|
|
priority: p1
|
|
issue_id: 001
|
|
tags: [test-fix, critical, code-quality]
|
|
---
|
|
|
|
# Problem Statement
|
|
|
|
**Debug statement in sorting test should be removed**
|
|
|
|
A debug `println` statement on line 138 of `test/clj/auto_ap/ssr/admin/accounts_test.clj` is left in the production test code. This will output debug information every time the test runs, which is unnecessary for production testing.
|
|
|
|
**Impact**: Debug output cluttering test runs, potential confusion about test intent, violates clean code principles.
|
|
|
|
## Findings
|
|
|
|
- **Location**: `test/clj/auto_ap/ssr/admin/accounts_test.clj` line 138
|
|
- **Current code**:
|
|
```clojure
|
|
(let [admin-identity (admin-token)
|
|
[accounts matching-count :as z] (sut/fetch-page {:query-params {:page 1 :per-page 10}})] ;; Default sort
|
|
(println "z is" z) ; <-- DEBUG STATEMENT
|
|
;; Test passes if sorting parameter is accepted and function returns successfully
|
|
```
|
|
|
|
- **Issue**: `z` variable is captured but never used (dead code), and the `println` output serves no purpose for test verification
|
|
|
|
- **Evidence**: Code review by kieran-python-reviewer identified this as debug statement that should be removed
|
|
|
|
## Proposed Solutions
|
|
|
|
### Option 1: Remove the println and unused variable (RECOMMENDED)
|
|
**Pros**:
|
|
- Cleanest solution
|
|
- No functionality changes
|
|
- Removes debug clutter
|
|
- Minimal effort
|
|
|
|
**Cons**:
|
|
- None
|
|
|
|
**Effort**: Small
|
|
|
|
**Risk**: None
|
|
|
|
### Option 2: Keep the println but remove unused variable
|
|
**Pros**:
|
|
- Slight performance improvement (no variable allocation)
|
|
|
|
**Cons**:
|
|
- Still leaves debug statement in code
|
|
- Doesn't address the root issue
|
|
|
|
**Effort**: Small
|
|
|
|
**Risk**: None
|
|
|
|
### Option 3: Remove the entire debug variable capture
|
|
**Pros**:
|
|
- Clean variable usage
|
|
|
|
**Cons**:
|
|
- Requires more thought about whether the variable was intentional
|
|
|
|
**Effort**: Small
|
|
|
|
**Risk**: None
|
|
|
|
## Recommended Action
|
|
|
|
**Remove the debug statement and unused variable capture.**
|
|
|
|
The `println "z is" z` is clearly debug code that should not be in production tests. This can be done in one simple edit to line 137-138.
|
|
|
|
## Technical Details
|
|
|
|
**Affected Component**: Test suite, specifically account sorting tests
|
|
|
|
**Affected Files**:
|
|
- `test/clj/auto_ap/ssr/admin/accounts_test.clj` (line 138)
|
|
|
|
**Database Changes**: None
|
|
|
|
**API Changes**: None
|
|
|
|
**Related Code**: None
|
|
|
|
## Acceptance Criteria
|
|
|
|
- [ ] Line 138 `println "z is" z` is removed from test file
|
|
- [ ] The variable capture `:as z` is either removed or assigned to underscore
|
|
- [ ] All tests still pass after removal
|
|
- [ ] No debug output appears when running `lein test auto-ap.ssr.admin.accounts-test`
|
|
|
|
## Work Log
|
|
|
|
- **2026-02-06**: Finding documented through code review agents (kieran-python-reviewer, code-simplicity-reviewer, pattern-recognition-specialist)
|
|
- **2026-02-06**: Fixed by removing line 138 `(println "z is" z)` and removing unused `:as z` variable capture (changed to no variable capture)
|
|
- **2026-02-06**: Verified all tests pass (0 failures, 0 errors)
|
|
|
|
## Resources
|
|
|
|
- **Review Agents**:
|
|
- kieran-python-reviewer (task ses_3c958be13ffehUVu6E1NzOC3Ch)
|
|
- code-simplicity-reviewer (task ses_3c958304dffeVRAAPfMSv583Ik)
|
|
- pattern-recognition-specialist (task ses_3c9578ae3ffejQg4Wl4GT1ZSAk)
|