path: root/docs/sessions/refactor.org

#+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.*