feat(ssr): add test suite for admin account management and document test improvements
- 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
This commit is contained in:
127
todos/005-pending-p2-remove-doc-only-test.md
Normal file
127
todos/005-pending-p2-remove-doc-only-test.md
Normal file
@@ -0,0 +1,127 @@
|
||||
---
|
||||
status: pending
|
||||
priority: p2
|
||||
issue_id: 005
|
||||
tags: [test-fix, important, code-quality]
|
||||
---
|
||||
|
||||
# Problem Statement
|
||||
|
||||
**Documentation-only test without actual assertions should be removed**
|
||||
|
||||
Lines 60-73 contain a test that is primarily documentation rather than a functional test. It explains that duplicate account name detection is not currently implemented, but doesn't actually test any functionality.
|
||||
|
||||
**Impact**: Test provides no value, adds to test maintenance burden, violates test purpose (testing should verify behavior, not document non-existent functionality).
|
||||
|
||||
## Findings
|
||||
|
||||
- **Location**: `test/clj/auto_ap/ssr/admin/accounts_test.clj` lines 60-73
|
||||
- **Current code**:
|
||||
```clojure
|
||||
(deftest account-creation-duplicate-name-detection
|
||||
(testing "Duplicate account name detection is not currently implemented"
|
||||
;; Current implementation only checks for duplicate numeric codes
|
||||
;; Duplicate name validation could be added following the same pattern as duplicate code
|
||||
;; For now, this test documents that duplicate names are allowed
|
||||
(with-redefs [auto-ap.solr/impl (auto-ap.solr/->InMemSolrClient (atom {}))]
|
||||
;; Create first account with name "Unique Account"
|
||||
(sut/account-save {:form-params {:account/numeric-code 12348 :account/name "Unique Account" :account/type :account-type/asset :account/location "A"} :request-method :post :identity (admin-token)})
|
||||
;; Create second account with same name
|
||||
(sut/account-save {:form-params {:account/numeric-code 12349 :account/name "Unique Account" :account/type :account-type/asset :account/location "B"} :request-method :post :identity (admin-token)})
|
||||
;; Verify both accounts exist (they should, as name uniqueness is not enforced)
|
||||
(let [db (dc/db conn)
|
||||
accounts (dc/q '[:find ?e :where [?e :account/name "Unique Account"]] db)]
|
||||
(is (= 2 (count accounts)))))))
|
||||
```
|
||||
|
||||
- **Issue**: This test documents behavior that is intentionally not implemented. It:
|
||||
1. Creates two accounts with the same name
|
||||
2. Verifies both exist (they should)
|
||||
3. Does NOT test that this is actually the desired behavior
|
||||
4. Is a documentation test, not a functional test
|
||||
|
||||
- **Problems**:
|
||||
- If future implementation adds duplicate name detection, this test would fail
|
||||
- It tests a design decision, not system behavior
|
||||
- Test name suggests it's testing functionality ("detection") but actually just documents it doesn't exist
|
||||
- Adds to maintenance burden for documentation that could be in a design doc
|
||||
|
||||
- **Evidence**: kieran-python-reviewer and code-simplicity-reviewer both identified this as redundant documentation test
|
||||
|
||||
## Proposed Solutions
|
||||
|
||||
### Option 1: Remove the test entirely (RECOMMENDED)
|
||||
**Pros**:
|
||||
- Eliminates redundant code
|
||||
- Removes confusion about what's actually tested
|
||||
- Documentation belongs in design docs, not tests
|
||||
- Reduces test suite size
|
||||
|
||||
**Cons**:
|
||||
- None
|
||||
|
||||
**Effort**: Small
|
||||
|
||||
**Risk**: None
|
||||
|
||||
### Option 2: Rename to clearly indicate it's documentation
|
||||
**Pros**:
|
||||
- Keeps the intent of the test
|
||||
- Makes it clear what it does
|
||||
|
||||
**Cons**:
|
||||
- Test is still not functional
|
||||
- Still adds to test maintenance
|
||||
- Doesn't solve the fundamental issue
|
||||
|
||||
**Effort**: Small
|
||||
|
||||
**Risk**: None
|
||||
|
||||
### Option 3: Convert to proper test if implementation changes
|
||||
**Pros**:
|
||||
- Can be re-added when feature is implemented
|
||||
|
||||
**Cons**:
|
||||
- Requires tracking
|
||||
- Test name needs to change
|
||||
|
||||
**Effort**: None (for now)
|
||||
|
||||
**Risk**: None
|
||||
|
||||
## Recommended Action
|
||||
|
||||
**Remove the test entirely.**
|
||||
|
||||
This is a documentation test that serves no functional purpose. If duplicate name detection is ever implemented in the future, it should have a proper test. For now, documentation of this design decision belongs in a design document or README, not in the test suite.
|
||||
|
||||
## Technical Details
|
||||
|
||||
**Affected Component**: Test suite organization and test purpose
|
||||
|
||||
**Affected Files**:
|
||||
- `test/clj/auto_ap/ssr/admin/accounts_test.clj` (lines 60-73)
|
||||
|
||||
**Database Changes**: None
|
||||
|
||||
**API Changes**: None
|
||||
|
||||
**Related Code**: Account creation functionality
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] Lines 60-73 test function is removed
|
||||
- [ ] No references to this test remain in test suite
|
||||
- [ ] All remaining tests still pass
|
||||
- [ ] Documentation of this behavior moved to appropriate documentation file
|
||||
|
||||
## 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)
|
||||
Reference in New Issue
Block a user