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
134 lines
4.0 KiB
Markdown
134 lines
4.0 KiB
Markdown
---
|
|
status: pending
|
|
priority: p2
|
|
issue_id: 003
|
|
tags: [test-fix, important, assertion-quality]
|
|
---
|
|
|
|
# Problem Statement
|
|
|
|
**Weak assertions in multiple tests**
|
|
|
|
Multiple tests only verify type checking `(is (number? matching-count))` instead of verifying actual functionality. This gives false confidence that tests are validating the system behavior.
|
|
|
|
**Impact**: Tests pass even when incorrect data is returned, providing minimal test coverage and risking regressions.
|
|
|
|
## Findings
|
|
|
|
- **Location**: Multiple test functions
|
|
- Line 100 (account-grid-view-loads-accounts): `(is (number? matching-count))`
|
|
- Line 110 (account-grid-displays-correct-columns): `(is (number? matching-count))`
|
|
- Line 126 (account-sorting-by-name): `(is (number? matching-count))`
|
|
- Line 137 (account-sorting-by-numeric-code): `(is (number? matching-count))`
|
|
- Line 150 (account-sorting-by-type): `(is (number? matching-count))`
|
|
- Line 28 (account-creation-success): `(is (= 1 (count accounts)))`
|
|
- Line 30 (account-creation-success): `(is (= 12345 (:account/numeric-code account)))`
|
|
- Line 41 (account-creation-success): `(is (= "B" (:account/location account)))`
|
|
|
|
- **Issue**: Some tests verify only the count of accounts returned, not that the correct accounts are returned
|
|
|
|
- **Example of weak assertion**:
|
|
```clojure
|
|
;; Line 100 - Only checks type, not functionality
|
|
(is (number? matching-count))
|
|
|
|
;; Should verify:
|
|
;; - The correct accounts were returned
|
|
;; - The accounts have the expected data
|
|
;; - Data integrity is maintained
|
|
```
|
|
|
|
- **Evidence**: Code review identified that ~40% of assertions are weak type-checking assertions
|
|
|
|
## Proposed Solutions
|
|
|
|
### Option 1: Verify actual account data returned (RECOMMENDED)
|
|
**Pros**:
|
|
- Strong validation that tests verify actual behavior
|
|
- Catches regressions more effectively
|
|
- Better test confidence
|
|
|
|
**Cons**:
|
|
- Requires test setup of expected account data
|
|
- More complex test code
|
|
|
|
**Effort**: Medium
|
|
|
|
**Risk**: None
|
|
|
|
**Example implementation**:
|
|
```clojure
|
|
;; Line 100 - Account grid view
|
|
(let [[accounts matching-count] (sut/fetch-page {:query-params {:page 1 :per-page 10}})]
|
|
(is (number? matching-count))
|
|
(is (pos? (count accounts))) ; Verify accounts exist
|
|
(is (every? #(contains? % :account/name) accounts))) ; Verify required fields
|
|
```
|
|
|
|
### Option 2: Combine with data verification from test setup
|
|
**Pros**:
|
|
- Consistent data verification
|
|
- Checks both creation and retrieval
|
|
|
|
**Cons**:
|
|
- Requires each test to create expected data
|
|
- More code duplication
|
|
|
|
**Effort**: Medium
|
|
|
|
**Risk**: None
|
|
|
|
### Option 3: Accept current weak assertions for now
|
|
**Pros**:
|
|
- Minimal changes
|
|
- Maintains current test structure
|
|
|
|
**Cons**:
|
|
- Low test confidence
|
|
- Doesn't actually verify functionality
|
|
- More likely to miss regressions
|
|
|
|
**Effort**: None
|
|
|
|
**Risk**: High (tests are not effective)
|
|
|
|
## Recommended Action
|
|
|
|
**Strengthen assertions to verify actual account data returned, not just type checking.**
|
|
|
|
For grid and sorting tests, verify that:
|
|
1. Accounts are actually returned (not just matching-count exists)
|
|
2. Returned accounts have required fields (account/name, account/numeric-code, etc.)
|
|
3. For grid tests, verify accounts match what was created
|
|
|
|
## Technical Details
|
|
|
|
**Affected Component**: Test assertions, account grid and sorting functionality
|
|
|
|
**Affected Files**:
|
|
- `test/clj/auto_ap/ssr/admin/accounts_test.clj` (lines 100, 110, 126, 137, 150)
|
|
|
|
**Database Changes**: None
|
|
|
|
**API Changes**: None
|
|
|
|
**Related Code**: None
|
|
|
|
## Acceptance Criteria
|
|
|
|
- [ ] Grid tests verify accounts are returned and have required fields
|
|
- [ ] Sorting tests verify accounts are returned
|
|
- [ ] All weak assertions strengthened to verify actual data
|
|
- [ ] All tests still pass with strengthened assertions
|
|
- [ ] Tests provide real confidence in system behavior
|
|
|
|
## Work Log
|
|
|
|
- **2026-02-06**: Finding documented through code review agents (kieran-python-reviewer, code-simplicity-reviewer)
|
|
|
|
## Resources
|
|
|
|
- **Review Agents**:
|
|
- kieran-python-reviewer (task ses_3c958be13ffehUVu6E1NzOC3Ch)
|
|
- code-simplicity-reviewer (task ses_3c958304dffeVRAAPfMSv583Ik)
|