diff options
Diffstat (limited to 'docs/sessions/refactor.org')
| -rw-r--r-- | docs/sessions/refactor.org | 593 |
1 files changed, 0 insertions, 593 deletions
diff --git a/docs/sessions/refactor.org b/docs/sessions/refactor.org deleted file mode 100644 index 11ff0a91..00000000 --- a/docs/sessions/refactor.org +++ /dev/null @@ -1,593 +0,0 @@ -#+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.* |