1 files changed, 593 insertions, 0 deletions
diff --git a/docs/sessions/refactor.org b/docs/sessions/refactor.org
new file mode 100644
index 00000000..11ff0a91
--- /dev/null
+++ b/docs/sessions/refactor.org
@@ -0,0 +1,593 @@
+#+TITLE: Test-Driven Quality Engineering Session: music-config.el
+#+AUTHOR: Craig Jennings & Claude
+#+DATE: 2025-11-01
+
+* Overview
+
+This document describes a comprehensive test-driven quality engineering session for =music-config.el=, an EMMS music player configuration module. The session demonstrates systematic testing practices, refactoring for testability, bug discovery through tests, and decision-making processes when tests fail.
+
+* Session Goals
+
+1. Add comprehensive unit test coverage for testable functions in =music-config.el=
+2. Discover and fix bugs through systematic testing
+3. Follow quality engineering principles from =ai-prompts/quality-engineer.org=
+4. Demonstrate refactoring patterns for testability
+5. Document the decision-making process for test vs production code issues
+
+* Phase 1: Feature Addition with Testability in Mind
+
+** The Feature Request
+
+Add functionality to append a track from the EMMS playlist to an existing M3U file by pressing ~A~ on the track.
+
+Requirements:
+- Show completing-read with available M3U playlists
+- Allow cancellation (C-g and explicit "(Cancel)" option)
+- Append track's absolute path to selected M3U
+- Provide clear success/failure feedback
+
+** Refactoring for Testability
+
+Following the "Interactive vs Non-Interactive Function Pattern" from =quality-engineer.org=:
+
+*Problem:* Directly implementing as an interactive function would require:
+- Mocking =completing-read=
+- Mocking =emms-playlist-track-at=
+- Testing Emacs UI functionality, not our business logic
+
+*Solution:* Split into two functions:
+
+1. *Helper Function* (=cj/music--append-track-to-m3u-file=):
+   - Pure, deterministic
+   - Takes explicit parameters: =(track-path m3u-file)=
+   - No user interaction
+   - Returns =t= on success, signals errors naturally
+   - 100% testable with ERT, no mocking needed
+
+2. *Interactive Wrapper* (=cj/music-append-track-to-playlist=):
+   - Thin layer handling only user interaction
+   - Gets track at point
+   - Shows completing-read
+   - Catches errors and displays messages
+   - Delegates all business logic to helper
+   - No tests needed (just testing Emacs)
+
+** Benefits of This Pattern
+
+From =quality-engineer.org=:
+#+begin_quote
+When writing functions that combine business logic with user interaction:
+- Split into internal implementation and interactive wrapper
+- Internal function (prefix with =--=): Pure logic, takes all parameters explicitly
+- Dramatically simpler testing (no interactive mocking)
+- Code reusable programmatically without prompts
+- Clear separation of concerns (logic vs UI)
+#+end_quote
+
+This pattern enabled:
+- Zero mocking in tests
+- Fast, deterministic tests
+- Easy reasoning about correctness
+- Reusable helper function
+
+* Phase 2: Writing the First Test
+
+** Test File: =test-music-config--append-track-to-m3u-file.el=
+
+Following the naming convention from =quality-engineer.org=:
+- Pattern: =test-<module>-<function>.el=
+- One test file per function for easy discovery when tests fail
+- User sees failure → immediately knows which file to open
+
+** Test Organization
+
+Following the three-category structure:
+
+*** Normal Cases (4 tests)
+- Append to empty file
+- Append to file with trailing newline
+- Append to file without trailing newline (adds leading newline)
+- Multiple appends (allows duplicates)
+
+*** Boundary Cases (4 tests)
+- Very long paths (~500 chars)
+- Unicode characters (中文, emoji)
+- Spaces and special characters
+- M3U with comments/metadata
+
+*** Error Cases (3 tests)
+- Nonexistent file
+- Read-only file
+- Directory instead of file
+
+** Writing Tests with Zero Mocking
+
+Key principle: "Don't mock what you're testing" (from =quality-engineer.org=)
+
+Example test:
+#+begin_src elisp
+(ert-deftest test-music-config--append-track-to-m3u-file-normal-empty-file-appends-track ()
+  "Append to brand new empty M3U file."
+  (test-music-config--append-track-to-m3u-file-setup)
+  (unwind-protect
+      (let* ((m3u-file (cj/create-temp-test-file "test-playlist-"))
+             (track-path "/home/user/music/artist/song.mp3"))
+        (cj/music--append-track-to-m3u-file track-path m3u-file)
+        (with-temp-buffer
+          (insert-file-contents m3u-file)
+          (should (string= (buffer-string) (concat track-path "\n")))))
+    (test-music-config--append-track-to-m3u-file-teardown)))
+#+end_src
+
+Notice:
+- No mocks
+- Real file I/O using =testutil-general.el= helpers
+- Tests actual function behavior
+- Clean setup/teardown
+
+** Result
+
+All 11 tests passed on first run. The pure, deterministic helper function worked correctly.
+
+* Phase 3: Systematic Test Coverage Analysis
+
+** Identifying Testable Functions
+
+Reviewed all functions in =music-config.el= and categorized:
+
+*** Easy to Test (Pure/Deterministic)
+- =cj/music--valid-file-p= - Extension validation
+- =cj/music--valid-directory-p= - Directory validation
+- =cj/music--safe-filename= - String sanitization
+- =cj/music--m3u-file-tracks= - M3U file parsing
+- =cj/music--get-m3u-basenames= - Basename extraction
+
+*** Medium Complexity (Need File I/O)
+- =cj/music--collect-entries-recursive= - Recursive directory traversal
+- =cj/music--get-m3u-files= - File discovery
+- =cj/music--completion-table= - Completion table generation
+
+*** Hard to Test (EMMS Buffer Dependencies)
+- =cj/music--ensure-playlist-buffer= - EMMS buffer creation
+- =cj/music--playlist-tracks= - EMMS buffer reading
+- =cj/music--playlist-modified-p= - EMMS buffer state
+- =cj/music--assert-valid-playlist-file= - Buffer-local state
+
+*Decision:* Test easy and medium complexity functions. Skip EMMS-dependent functions (would require extensive mocking/setup, diminishing returns).
+
+** File Organization Principle
+
+From =quality-engineer.org=:
+#+begin_quote
+*Unit Tests*: One file per method
+- Naming: =test-<filename>-<methodname>.el=
+- Example: =test-org-gcal--safe-substring.el=
+#+end_quote
+
+*Rationale:* When a test fails in CI:
+1. Developer sees: =test-music-config--get-m3u-files-normal-multiple-files-returns-list FAILED=
+2. Immediately knows: Look for =test-music-config--get-m3u-files.el=
+3. Opens file and fixes issue - *fast cognitive path*
+
+If combined files:
+1. Test fails: =test-music-config--get-m3u-files-normal-multiple-files-returns-list FAILED=
+2. Which file? =test-music-config--m3u-helpers.el=? =test-music-config--combined.el=?
+3. Developer wastes time searching - *slower, frustrating*
+
+*The 1:1 mapping is a usability feature for developers under pressure.*
+
+* Phase 4: Testing Function by Function
+
+** Function 1: =cj/music--valid-file-p=
+
+*** Test Categories
+
+*Normal Cases:*
+- Valid extensions (mp3, flac, etc.)
+- Case-insensitive matching (MP3, Mp3)
+
+*Boundary Cases:*
+- Dots in path (only last extension matters)
+- Multiple extensions (uses rightmost)
+- No extension
+- Empty string
+
+*Error Cases:*
+- Nil input
+- Non-music extensions
+
+*** First Run: 14/15 Passed, 1 FAILED
+
+*Failure:*
+#+begin_src
+test-music-config--valid-file-p-error-nil-input-returns-nil
+Expected: Returns nil gracefully
+Actual: (wrong-type-argument stringp nil) - CRASHED
+#+end_src
+
+*** Bug Analysis: Test or Production Code?
+
+*Process:*
+1. Read the test expectation: "nil input returns nil gracefully"
+2. Read the production code:
+   #+begin_src elisp
+   (defun cj/music--valid-file-p (file)
+     (when-let ((ext (file-name-extension file)))  ; ← Crashes here
+       (member (downcase ext) cj/music-file-extensions)))
+   #+end_src
+3. Identify issue: =file-name-extension= expects string, crashes on nil
+4. Consider context: This is defensive validation code, called in various contexts
+
+*Decision: Fix production code*
+
+*Rationale:*
+- Function should be defensive (validation code)
+- Returning nil for invalid input is more robust than crashing
+- Common pattern in Emacs Lisp validation functions
+
+*Fix:*
+#+begin_src elisp
+(defun cj/music--valid-file-p (file)
+  (when (and file (stringp file))  ; ← Guard added
+    (when-let ((ext (file-name-extension file)))
+      (member (downcase ext) cj/music-file-extensions))))
+#+end_src
+
+Result: All 15 tests passed.
+
+** Function 2: =cj/music--valid-directory-p=
+
+*** First Run: 11/13 Passed, 2 FAILED
+
+*Failures:*
+1. Nil input crashed (same pattern as =valid-file-p=)
+2. Empty string returned non-nil (treated as current directory)
+
+*Fix:*
+#+begin_src elisp
+(defun cj/music--valid-directory-p (dir)
+  (when (and dir (stringp dir) (not (string-empty-p dir)))  ; ← Guards added
+    (and (file-directory-p dir)
+         (not (string-prefix-p "." (file-name-nondirectory
+                                    (directory-file-name dir)))))))
+#+end_src
+
+Result: All 13 tests passed.
+
+** Function 3: =cj/music--safe-filename=
+
+*** First Run: 12/13 Passed, 1 FAILED
+
+*Failure:*
+#+begin_src
+test-music-config--safe-filename-boundary-special-chars-replaced
+Expected: "playlist__________" (10 underscores)
+Actual: "playlist_________" (9 underscores)
+#+end_src
+
+*** Bug Analysis: Test or Production Code?
+
+*Process:*
+1. Count special chars in input: =@#$%^&*()= = 9 characters
+2. Test expected 10, but input only has 9
+3. Production code is correct
+
+*Decision: Fix test code*
+
+*The bug was in the test expectation, not the implementation.*
+
+Result: All 13 tests passed.
+
+** Function 4: =cj/music--m3u-file-tracks= (M3U Parser)
+
+This is where we found a **significant bug** through testing!
+
+*** Test Categories
+
+*Normal Cases:*
+- Absolute paths
+- Relative paths (expanded to M3U directory)
+- HTTP/HTTPS/MMS URLs preserved
+- Mixed paths and URLs
+
+*Boundary Cases:*
+- Empty lines ignored (important for playlist robustness!)
+- Whitespace-only lines ignored
+- Comments ignored (#EXTM3U, #EXTINF)
+- Leading/trailing whitespace trimmed
+- Order preserved
+
+*Error Cases:*
+- Nonexistent file
+- Nil input
+
+*** First Run: 11/15 Passed, 4 FAILED
+
+All 4 failures related to URL handling:
+
+*Failure Pattern:*
+#+begin_src
+Expected: "http://example.com/stream.mp3"
+Actual: "/home/cjennings/.temp-emacs-tests/http:/example.com/stream.mp3"
+#+end_src
+
+HTTP/HTTPS/MMS URLs were being treated as relative paths and mangled!
+
+*** Root Cause Analysis
+
+*Production code (line 110):*
+#+begin_src elisp
+(string-match-p "\`\(https?\|mms\)://" line)
+#+end_src
+
+*Problem:* Regex escaping is wrong!
+
+In the string literal ="\`"=:
+- The backslash-backtick becomes a *literal backtick character*
+- Not the regex anchor =\`= (start of string)
+
+The regex never matched, so URLs were treated as relative paths.
+
+*Correct version:*
+#+begin_src elisp
+(string-match-p "\\`\\(https?\\|mms\\)://" line)
+#+end_src
+
+Double backslashes for string literal escaping → results in regex =\`\(https?\|mms\)://=
+
+*** Impact Assessment
+
+*This is a significant bug:*
+- Radio stations (HTTP streams) would be broken
+- Any M3U with URLs would fail
+- Data corruption: URLs transformed into nonsensical file paths
+- Function worked for local files, so bug went unnoticed
+- Users would see mysterious errors when loading playlists with streams
+
+*Tests caught a real production bug that could have caused user data corruption!*
+
+Result: All 15 tests passed after fix.
+
+* Phase 5: Continuing Through the Test Suite
+
+** Functions Tested Successfully
+
+5. =cj/music--get-m3u-files= - 7 tests
+   - Learned: Directory listing order is filesystem-dependent
+   - Solution: Sort results before comparing in tests
+
+6. =cj/music--get-m3u-basenames= - 6 tests
+   - Kept as separate file (not combined with get-m3u-files)
+   - Reason: Usability when tests fail
+
+7. =cj/music--collect-entries-recursive= - 12 tests
+   - Medium complexity: Required creating test directory trees
+   - Used =testutil-general.el= helpers for setup/teardown
+   - All tests passed first time (well-factored function)
+
+8. =cj/music--completion-table= - 12 tests
+   - Tested higher-order function (returns lambda)
+   - Initially misunderstood completion protocol behavior
+   - Fixed test expectations to match actual Emacs behavior
+
+* Key Principles Applied
+
+** 1. Refactor for Testability BEFORE Writing Tests
+
+The Interactive vs Non-Interactive pattern from =quality-engineer.org= made testing trivial:
+- No mocking required
+- Fast, deterministic tests
+- Clear separation of concerns
+
+** 2. Systematic Test Organization
+
+Every test file followed the same structure:
+- Normal Cases
+- Boundary Cases
+- Error Cases
+
+This makes it easy to:
+- Identify coverage gaps
+- Add new tests
+- Understand what's being tested
+
+** 3. Test Naming Convention
+
+Pattern: =test-<module>-<function>-<category>-<scenario>-<expected-result>=
+
+Examples:
+- =test-music-config--valid-file-p-normal-mp3-extension-returns-true=
+- =test-music-config--m3u-file-tracks-boundary-empty-lines-ignored=
+- =test-music-config--safe-filename-error-nil-input-signals-error=
+
+Benefits:
+- Self-documenting
+- Easy to understand what failed
+- Searchable/grepable
+- Clear category organization
+
+** 4. Zero Mocking for Pure Functions
+
+From =quality-engineer.org=:
+#+begin_quote
+DON'T MOCK WHAT YOU'RE TESTING
+- Only mock external side-effects and dependencies, not the domain logic itself
+- If mocking removes the actual work the function performs, you're testing the mock
+- Use real data structures that the function is designed to operate on
+- Rule of thumb: If the function body could be =(error "not implemented")= and tests still pass, you've over-mocked
+#+end_quote
+
+Our tests used:
+- Real file I/O
+- Real strings
+- Real data structures
+- Actual function behavior
+
+Result: Tests caught real bugs, not mock configuration issues.
+
+** 5. Test vs Production Code Bug Decision Framework
+
+When a test fails, ask:
+
+1. *What is the test expecting?*
+   - Read the test name and assertions
+   - Understand the intended behavior
+
+2. *What is the production code doing?*
+   - Read the implementation
+   - Trace through the logic
+
+3. *Which is correct?*
+   - Is the test expectation reasonable?
+   - Is the production behavior defensive/robust?
+   - What is the usage context?
+
+4. *Consider the impact:*
+   - Defensive code: Fix production to handle edge cases
+   - Wrong expectation: Fix test
+   - Unclear spec: Ask user for clarification
+
+Examples from our session:
+- *Nil input crashes* → Fix production (defensive coding)
+- *Empty string treated as valid* → Fix production (defensive coding)
+- *Wrong count in test* → Fix test (test bug)
+- *Regex escaping wrong* → Fix production (real bug!)
+
+** 6. Fast Feedback Loop
+
+Pattern: "Write tests, run them all, report errors, and see where we are!"
+
+This became a mantra during the session:
+1. Write comprehensive tests for one function
+2. Run immediately
+3. Analyze failures
+4. Fix bugs (test or production)
+5. Verify all tests pass
+6. Move to next function
+
+Benefits:
+- Caught bugs immediately
+- Small iteration cycles
+- Clear progress
+- High confidence in changes
+
+* Final Results
+
+** Test Coverage
+
+*9 functions tested, 103 tests total:*
+1. =cj/music--append-track-to-m3u-file= - 11 tests
+2. =cj/music--valid-file-p= - 15 tests
+3. =cj/music--valid-directory-p= - 13 tests
+4. =cj/music--safe-filename= - 13 tests
+5. =cj/music--m3u-file-tracks= - 15 tests
+6. =cj/music--get-m3u-files= - 7 tests
+7. =cj/music--get-m3u-basenames= - 6 tests
+8. =cj/music--collect-entries-recursive= - 12 tests
+9. =cj/music--completion-table= - 12 tests
+
+** Bugs Discovered and Fixed
+
+1. *=cj/music--valid-file-p=*
+   - Issue: Crashed on nil input
+   - Fix: Added nil/string guard
+   - Impact: Prevents crashes in validation code
+
+2. *=cj/music--valid-directory-p=*
+   - Issue: Crashed on nil, treated empty string as valid
+   - Fix: Added guards for nil and empty string
+   - Impact: More robust directory validation
+
+3. *=cj/music--m3u-file-tracks=* ⚠️ *SIGNIFICANT BUG*
+   - Issue: URL regex escaping wrong - HTTP/HTTPS/MMS URLs mangled as relative paths
+   - Fix: Corrected regex escaping: ="\`"= → ="\\`"=
+   - Impact: Radio stations and streaming URLs now work correctly
+   - *This bug would have corrupted user data and broken streaming playlists*
+
+** Code Quality Improvements
+
+- All testable helper functions now have comprehensive test coverage
+- More defensive error handling (nil guards)
+- Clear separation of concerns (pure helpers vs interactive wrappers)
+- Systematic boundary condition testing
+- Unicode and special character handling verified
+
+* Lessons Learned
+
+** 1. Tests as Bug Discovery Tools
+
+Tests aren't just for preventing regressions - they actively *discover existing bugs*:
+- The URL regex bug existed in production
+- Nil handling bugs would have manifested in edge cases
+- Tests made these issues visible immediately
+
+** 2. Refactoring Enables Testing
+
+The decision to split functions into pure helpers + interactive wrappers:
+- Made testing dramatically simpler
+- Enabled 100+ tests with zero mocking
+- Improved code reusability
+- Clarified function responsibilities
+
+** 3. Systematic Process Matters
+
+Following the same pattern for each function:
+- Reduced cognitive load
+- Made it easy to maintain consistency
+- Enabled quick iteration
+- Built confidence in coverage
+
+** 4. File Organization Aids Debugging
+
+One test file per function:
+- Fast discovery when tests fail
+- Clear ownership
+- Easy to maintain
+- Follows user's mental model
+
+** 5. Test Quality Equals Production Quality
+
+Our tests:
+- Used real I/O (not mocks)
+- Tested actual behavior
+- Covered edge cases systematically
+- Found real bugs
+
+This is only possible with well-factored, testable code.
+
+* Applying These Principles
+
+When adding tests to other modules:
+
+1. *Identify testable functions* - Look for pure helpers, file I/O, string manipulation
+2. *Refactor if needed* - Split interactive functions into pure helpers
+3. *Write systematically* - Normal, Boundary, Error categories
+4. *Run frequently* - Fast feedback loop
+5. *Analyze failures carefully* - Test bug vs production bug
+6. *Fix immediately* - Don't accumulate technical debt
+7. *Maintain organization* - One file per function, clear naming
+
+* Reference
+
+See =ai-prompts/quality-engineer.org= for comprehensive quality engineering guidelines, including:
+- Test organization and structure
+- Test naming conventions
+- Mocking and stubbing best practices
+- Interactive vs non-interactive function patterns
+- Integration testing guidelines
+- Test maintenance strategies
+
+Note: =quality-engineer.org= evolves as we learn more quality best practices. This document captures principles applied during this specific session.
+
+* Conclusion
+
+This session demonstrated how systematic testing combined with refactoring for testability can:
+- Discover real bugs before they reach users
+- Improve code quality and robustness
+- Build confidence in changes
+- Create maintainable test suites
+- Follow industry best practices
+
+The 103 tests and 3 bug fixes represent a significant quality improvement to =music-config.el=. The URL regex bug alone justified the entire testing effort - that bug could have caused user data corruption and broken a major feature (streaming radio).
+
+*Testing is not just about preventing future bugs - it's about finding bugs that already exist.*