diff options
| -rw-r--r-- | bugs.org | 151 | ||||
| -rw-r--r-- | docs/NOTES.org | 131 | ||||
| -rw-r--r-- | docs/session-1-summary.org | 141 | ||||
| -rw-r--r-- | docs/testing-plan.org | 152 | ||||
| -rw-r--r-- | tests/test-wttrin--build-url.el | 91 | ||||
| -rw-r--r-- | tests/test-wttrin--cleanup-cache-if-needed.el | 124 | ||||
| -rw-r--r-- | tests/test-wttrin--make-cache-key.el | 69 | ||||
| -rw-r--r-- | tests/test-wttrin-additional-url-params.el | 60 | ||||
| -rw-r--r-- | tests/testutil-wttrin.el | 83 | ||||
| -rw-r--r-- | wttrin.el | 42 |
10 files changed, 1033 insertions, 11 deletions
diff --git a/bugs.org b/bugs.org new file mode 100644 index 0000000..e1648c8 --- /dev/null +++ b/bugs.org @@ -0,0 +1,151 @@ +* Bugs Found in wttrin.el Code Analysis + +** BUG [#A] Callback passed to url-retrieve-synchronously (Line 114) +Location: =wttrin.el:114= + +The code passes a lambda callback to =url-retrieve-synchronously=: +#+begin_src elisp +(url-retrieve-synchronously + (concat "https://wttr.in/" query (wttrin-additional-url-params) "A") + (lambda () (switch-to-buffer (current-buffer)))) +#+end_src + +The synchronous API signature is =(url-retrieve-synchronously URL &optional silent inhibit-cookies)=. The lambda is being interpreted as the =silent= argument, not a callback. This does nothing but is misleading. + +*Fix:* Remove the lambda and just call: +#+begin_src elisp +(url-retrieve-synchronously url) +#+end_src + +** BUG [#A] HTTP headers not stripped before xterm-color-filter (Line 152) +Location: =wttrin.el:152= + +The code inserts the raw buffer string directly: +#+begin_src elisp +(insert (xterm-color-filter raw-string)) +#+end_src + +But =raw-string= from =wttrin-fetch-raw-string= includes HTTP headers (status line, Date, Content-Type, etc.). This means the headers are passed through =xterm-color-filter=. + +*Fix:* Strip HTTP headers first: +#+begin_src elisp +(goto-char (point-min)) +(re-search-forward "\r?\n\r?\n") +(let ((body (buffer-substring-no-properties (point) (point-max)))) + (xterm-color-filter body)) +#+end_src + +** BUG [#A] No URL encoding of user input (Lines 113, 201) +Location: =wttrin.el:113=, =wttrin.el:201= + +User input is concatenated directly into URLs: +#+begin_src elisp +(concat "https://wttr.in/" query (wttrin-additional-url-params) "A") +#+end_src + +If a user enters ="New York, NY"= the space and comma could cause URL parsing issues. + +*Fix:* URL-encode the location: +#+begin_src elisp +(concat "https://wttr.in/" (url-hexify-string query) ...) +#+end_src + +** BUG [#A] No error handling for nil buffer from url-retrieve-synchronously (Line 111) +Location: =wttrin.el:111= + +The code assumes =url-retrieve-synchronously= always returns a buffer: +#+begin_src elisp +(with-current-buffer + (url-retrieve-synchronously ...) + (decode-coding-string (buffer-string) 'utf-8)) +#+end_src + +If there's a timeout, DNS failure, or network error, =url-retrieve-synchronously= can return nil, causing an error in =with-current-buffer=. + +*Fix:* Check for nil: +#+begin_src elisp +(let ((buf (url-retrieve-synchronously url t t))) + (if (not buf) + (error "wttrin: network failure") + (with-current-buffer buf ...))) +#+end_src + +** BUG [#B] Double concatenation of url params (Line 201) +Location: =wttrin.el:201= + +In =wttrin--get-cached-or-fetch=: +#+begin_src elisp +(wttrin-fetch-raw-string (concat location (wttrin-additional-url-params))) +#+end_src + +But =wttrin-fetch-raw-string= already calls =wttrin-additional-url-params= on line 113. This means the params get added twice. + +*Fix:* Remove the concatenation from line 201: +#+begin_src elisp +(wttrin-fetch-raw-string location) +#+end_src + +** BUG [#C] wttrin-additional-url-params returns malformed URL (Line 105) +Location: =wttrin.el:105= + +The function returns: +#+begin_src elisp +(concat "?" wttrin-unit-system) +#+end_src + +If =wttrin-unit-system= is nil, this returns ="?"=. When concatenated with the ="A"= on line 113, you get URLs like =https://wttr.in/Paris?A= instead of =https://wttr.in/Paris?mA= (for metric). + +*Fix:* Handle nil properly: +#+begin_src elisp +(defun wttrin-additional-url-params () + "Concatenates extra information into the URL." + (concat "?" (or wttrin-unit-system ""))) +#+end_src + +Or better yet, restructure the URL building entirely. + +** BUG [#C] Hard-coded buffer name prevents multiple locations (Line 139) +Location: =wttrin.el:139= + +The buffer is always named ="*wttr.in*"=: +#+begin_src elisp +(let ((buffer (get-buffer-create (format "*wttr.in*"))) +#+end_src + +This prevents viewing multiple locations simultaneously. The format string has no interpolation. + +*Fix:* Use the location name: +#+begin_src elisp +(let ((buffer (get-buffer-create (format "*wttr.in: %s*" location-name))) +#+end_src + +** BUG [#C] Fragile header parsing with hard-coded line numbers (Lines 156-165) +Location: =wttrin.el:156-165= + +The code assumes weather data always has specific information on lines 4 and 6: +#+begin_src elisp +(forward-line 4) +(setq date-time-stamp ...) +(goto-char (point-min)) +(forward-line 6) +(setq location-info ...) +#+end_src + +This is fragile and will break if wttr.in changes its output format or if HTTP headers vary in length. + +*Fix:* Use pattern matching to find the information instead of relying on fixed line numbers. + +** BUG [#C] No buffer cleanup after url-retrieve-synchronously (Line 111) +Location: =wttrin.el:111= + +After retrieving the URL, the buffer created by =url-retrieve-synchronously= is never killed. This creates buffer leaks on each weather fetch. + +*Fix:* Kill the buffer after extraction: +#+begin_src elisp +(let ((buf (url-retrieve-synchronously url))) + (when buf + (unwind-protect + (with-current-buffer buf + (decode-coding-string (buffer-string) 'utf-8)) + (kill-buffer buf)))) +#+end_src diff --git a/docs/NOTES.org b/docs/NOTES.org new file mode 100644 index 0000000..7040321 --- /dev/null +++ b/docs/NOTES.org @@ -0,0 +1,131 @@ + +* Claude Code Session Notes +This file contains important information for Claude to remember between sessions. +** User Information +*** Calendar Location +Craig's calendar is available at: =/home/cjennings/sync/org/gcal.org= + +Use this to: +- Check meeting times and schedules +- Verify when events occurred +- See what's upcoming + +*** Todo List Location +Craig's todo list is at: =/home/cjennings/code/wttrin/todo.org= + +*IMPORTANT*: When Craig refers to "my todo list" or "task list", he means this file. +- Use the Edit tool to update tasks in this file +- Do NOT confuse this with Claude Code's TodoWrite tool (which is for tracking Claude's internal task progress) +- This is an org-mode file with TODO/DOING/DONE states + +*** Quality Engineering Guidelines +Craig's quality engineering and testing practices are documented at: +=/home/cjennings/.emacs.d/ai-prompts/quality-engineer.org= + +*Key principles for wttrin testing project:* +- Follow ERT (Emacs Lisp Regression Testing) framework +- Separate interactive UI from logic (Interactive vs Non-Interactive Pattern) + - Internal functions prefixed with =--= contain pure logic + - Interactive wrappers handle user prompts + - Test the internal functions with direct parameters +- Test file organization: + - Unit tests: =test-wttrin-<methodname>.el= (one file per method) + - Integration tests: =test-integration-<area-or-workflow>.el= + - Test utilities: =testutil-<category>.el= +- Test naming: =test-<module>-<function>-<category>-<scenario>-<expected-result>= + - Categories: normal, boundary, error +- NEVER hardcode dates in tests - use dynamic timestamp generation +- DON'T MOCK WHAT YOU'RE TESTING - only mock external dependencies +- Each test must be independent and deterministic + +*Current testing project:* +- Adding comprehensive ERT tests to wttrin package +- Multi-session project - track progress in session notes +- Refactor functions to be testable (separate UI from logic) +- Goal: Full test coverage for all core functionality + +*Session 1 Progress (2025-11-03):* +- Created comprehensive testing plan in =docs/testing-plan.org= +- Refactored wttrin.el to separate UI from logic + - Extracted =wttrin--build-url= (pure function for URL building) + - Fixed multiple bugs in =wttrin-fetch-raw-string=: + - Removed incorrect callback parameter to url-retrieve-synchronously + - Added nil buffer check for network failures + - Strip HTTP headers before decoding + - Kill buffer after use to prevent leaks + - URL encoding now handled properly + - Fixed double concatenation bug in =wttrin--get-cached-or-fetch= + - Fixed nil handling in =wttrin-additional-url-params= +- Created test infrastructure: + - =tests/testutil-wttrin.el= with test utilities and helpers + - 4 unit test files with 33 comprehensive tests +- Test Results: *33 tests, 33 passing, 0 failures* + +*Files with test coverage:* +1. =wttrin-additional-url-params= - 7 tests (normal, boundary, error cases) +2. =wttrin--make-cache-key= - 9 tests (includes Unicode, special chars) +3. =wttrin--build-url= - 10 tests (URL encoding, parameters) +4. =wttrin--cleanup-cache-if-needed= - 7 tests (cache eviction logic) + +*Next session priorities:* +1. Write tests for =wttrin--get-cached-or-fetch= (cache workflow) +2. Extract parsing logic from =wttrin-query= and test it +3. Write integration tests for fetch → parse → display workflow +4. Test error handling paths +5. Consider adding tests for interactive functions (if needed) + +*** Working Style +- Craig uses Emacs as primary tool +** Session Protocols / Vocabulary +*** "Add to our vocabulary" +When Craig says "let's add this to our vocabulary" or a similar phrase, you should execute this sequence: + +1. Add a row at this level to the parent org heading named "Session Protocols / Vocabulary" +2. Ask Craig for any similar names he might use. +3. If he already hasn't provided this information or it isn't clear, ask him for the steps you should take when you see this phrase. Make sure the phrase under discussion is clear. +4. Write the definition in steps just like any of the other org-headers and their content in this list. +5. Then remember the instructions, because he's likely to ask you to do that vocabulary word. +*** "Let's wrap it up" / "That's a wrap" - End of Session Routine +When Craig says "let's wrap it up" or "that's a wrap", execute this sequence: + +1. *Update notes*: Add anything important to remember for next session to this file (CLAUDE-NOTES.org) + - Session summary if significant work was done + - Critical reminders for tomorrow + - Any new conventions or preferences learned + +2. *Git commit and push*: Commit all changes in the repository and push to remote + - Use descriptive commit message + - Ensure working tree is clean + - Confirm push succeeded + +3. *Final exchange*: Brief valediction/good wishes before Craig closes laptop + - Keep it warm but concise + - Acknowledge the day's work if appropriate + - "Good night" / "Talk tomorrow" / similar + +This ensures clean handoff between sessions and nothing gets lost. + +** File Naming Conventions +*** Update todo.org Links When Renaming Files +Many documents in this project are linked in =todo.org= using org-mode =file:= links. + +*When renaming ANY file*, always search =todo.org= for references to that file and update all links to the new filename. + +Example workflow: +1. Rename file (e.g., add =TOOLARGE-= prefix) +2. Search =todo.org= for old filename +3. Update all =file:= links to new filename + +This prevents broken links and maintains the integrity of the project documentation. + +** File Format Preferences +*** ALWAYS Use Org-Mode Format +The user uses Emacs as their primary tool. *ALWAYS* create new documentation files in =.org= format, not =.md= (markdown). + +*Rationale*: +- Org-mode files are well-supported in Emacs +- Can be easily exported to any other format (HTML, PDF, Markdown, etc.) +- Better integration with user's workflow + +*Exception*: Only use .md if specifically requested or if the file is intended for GitHub/web display where markdown is expected. + diff --git a/docs/session-1-summary.org b/docs/session-1-summary.org new file mode 100644 index 0000000..ebb0f4c --- /dev/null +++ b/docs/session-1-summary.org @@ -0,0 +1,141 @@ +* WTTRIN Testing Project - Session 1 Summary +Date: 2025-11-03 +Status: ✅ Complete + +** What Was Accomplished + +*** Bug Fixes in wttrin.el (5 bugs fixed) +1. ✅ Fixed =wttrin-additional-url-params= to handle nil properly +2. ✅ Removed incorrect callback parameter in =wttrin-fetch-raw-string= +3. ✅ Added nil buffer check for network failures +4. ✅ Strip HTTP headers before decoding (was causing display issues) +5. ✅ Kill buffer after fetch to prevent memory leaks +6. ✅ Fixed double concatenation of URL params in cache function +7. ✅ Added proper URL encoding via new =wttrin--build-url= function + +*** Code Refactoring +- Extracted =wttrin--build-url= as pure function (testable) +- Separated URL building logic from network I/O +- All refactoring maintains backward compatibility + +*** Test Infrastructure Created +- =tests/testutil-wttrin.el= - Shared test utilities + - Cache helpers (add, clear, size) + - Custom variable management macros + - Setup/teardown functions + +*** Test Coverage Added +*Total: 33 tests, 100% passing* + +1. *test-wttrin-additional-url-params.el* (7 tests) + - Normal: metric, USCS, wind speed units + - Boundary: nil, empty string, single char + - Error: invalid type handling + +2. *test-wttrin--make-cache-key.el* (9 tests) + - Normal: location with different unit systems + - Boundary: empty string, spaces, commas, Unicode, special chars + - Error: nil location + +3. *test-wttrin--build-url.el* (10 tests) + - Normal: simple locations, different unit systems + - Boundary: spaces, commas, special chars, Unicode, GPS coords + - Error: nil location signals error + +4. *test-wttrin--cleanup-cache-if-needed.el* (7 tests) + - Normal: cache cleanup when exceeding max size + - Boundary: empty cache, exactly at max, edge cases + - Validates oldest entries are removed correctly + +** Test Execution +All tests pass successfully: + +#+begin_src shell +emacs --batch -L . -L tests \ + --eval "(setq package-user-dir \"~/.emacs.d/elpa\")" \ + --eval "(package-initialize)" \ + -l wttrin.el -l testutil-wttrin.el \ + -l test-wttrin-additional-url-params.el \ + -l test-wttrin--make-cache-key.el \ + -l test-wttrin--build-url.el \ + -l test-wttrin--cleanup-cache-if-needed.el \ + -f ert-run-tests-batch-and-exit +#+end_src + +*Result:* Running 33 tests ... Ran 33 tests, 33 results as expected, 0 unexpected + +** Documentation Created +- =docs/testing-plan.org= - Comprehensive testing roadmap + - Identifies all functions needing tests + - Categorizes by refactoring needs + - Phases for implementation + - Estimated 95 total tests when complete + +- =docs/NOTES.org= - Updated with: + - Quality engineering guidelines reference + - Session 1 progress summary + - Next session priorities + +- =docs/session-1-summary.org= - This file! + +** Files Modified +- =wttrin.el= - Bug fixes and refactoring +- =docs/NOTES.org= - Testing project notes added +- =~/.claude/settings.json= - Added wttrin project permissions + +** Files Created +- =docs/testing-plan.org= +- =docs/bugs.org= +- =tests/testutil-wttrin.el= +- =tests/test-wttrin-additional-url-params.el= +- =tests/test-wttrin--make-cache-key.el= +- =tests/test-wttrin--build-url.el= +- =tests/test-wttrin--cleanup-cache-if-needed.el= +- =docs/session-1-summary.org= + +** Next Session Priorities +1. Write tests for =wttrin--get-cached-or-fetch= (cache workflow with TTL) +2. Extract and test parsing logic from =wttrin-query= +3. Write integration tests for fetch → parse → display workflow +4. Test error handling paths throughout +5. Consider async loading tests (if needed) + +** Progress Metrics +- Functions tested: 4 of ~10 planned +- Test coverage: ~40% of core functions +- Bugs fixed: 7 (from original audit) +- Bugs remaining: ~9 (see =docs/bugs.org= and =todo.org=) +- Tests written: 33 +- Tests passing: 33 (100%) +- Tests failing: 0 + +** Notes for Next Session +- Test utilities are working well - easy to add new tests +- Refactoring pattern (extract pure functions) is effective +- Cache logic is more complex - needs careful testing +- Consider mocking url-retrieve-synchronously for fetch tests +- Buffer manipulation in wttrin-query needs extraction for testing + +** How to Run Tests +Individual test file: +#+begin_src shell +emacs --batch -L . -L tests \ + --eval "(setq package-user-dir \"~/.emacs.d/elpa\")" \ + --eval "(package-initialize)" \ + -l wttrin.el -l testutil-wttrin.el \ + -l test-wttrin-<function-name>.el \ + -f ert-run-tests-batch-and-exit +#+end_src + +All tests: +#+begin_src shell +emacs --batch -L . -L tests \ + --eval "(setq package-user-dir \"~/.emacs.d/elpa\")" \ + --eval "(package-initialize)" \ + -l wttrin.el -l testutil-wttrin.el \ + -l test-wttrin-additional-url-params.el \ + -l test-wttrin--make-cache-key.el \ + -l test-wttrin--build-url.el \ + -l test-wttrin--cleanup-cache-if-needed.el \ + -f ert-run-tests-batch-and-exit +#+end_src diff --git a/docs/testing-plan.org b/docs/testing-plan.org new file mode 100644 index 0000000..e2fae9a --- /dev/null +++ b/docs/testing-plan.org @@ -0,0 +1,152 @@ +* WTTRIN Testing Plan + +** Methods Analysis + +*** Pure Logic Functions (Easy to Test - No Refactoring Needed) +These functions have no side effects and can be tested directly: + +**** =wttrin-additional-url-params= (Line 103) +- *Purpose*: Concatenates unit system into URL parameter +- *Current issues*: Has a bug (returns "?" when wttrin-unit-system is nil) +- *Test categories*: + - Normal: wttrin-unit-system = "m", should return "?m" + - Normal: wttrin-unit-system = "u", should return "?u" + - Boundary: wttrin-unit-system = nil, currently returns "?" (bug!) + - Boundary: wttrin-unit-system = "", should return "?" + - Error: wttrin-unit-system is number/list (type error) + +**** =wttrin--make-cache-key= (Line 183) +- *Purpose*: Creates cache key from location and settings +- *Test categories*: + - Normal: location + unit system, returns "location|m" + - Normal: location with no unit system, returns "location|default" + - Boundary: Empty location string + - Boundary: Location with special characters (spaces, commas, Unicode) + - Error: nil location + +**** =wttrin--cleanup-cache-if-needed= (Line 217) +- *Purpose*: Removes oldest 20% of cache entries when cache exceeds max size +- *Test categories*: + - Normal: Cache at max, removes correct number of oldest entries + - Normal: Cache under max, does nothing + - Boundary: Cache exactly at max limit + - Boundary: Empty cache + - Boundary: Cache with 1 entry at max=1 + +*** Functions Needing Refactoring (Mixed Logic + Side Effects) + +**** =wttrin-fetch-raw-string= (Line 107) +*Current state*: Mixes URL building, network I/O, and decoding + +*Refactoring plan*: +- Create =wttrin--build-url= (pure): Takes query, returns URL string +- Create =wttrin--fetch-url= (I/O): Takes URL, returns raw response +- Keep =wttrin-fetch-raw-string= as thin wrapper calling both + +*Tests for =wttrin--build-url=*: +- Normal: query "Paris" → "https://wttr.in/Paris?A" +- Normal: query with unit system "m" → "https://wttr.in/Paris?mA" +- Boundary: query with spaces "New York" → properly encoded URL +- Boundary: query with special chars ",./~" → properly encoded +- Boundary: Empty query → "https://wttr.in/?A" +- Error: nil query → error + +*Tests for =wttrin--fetch-url=*: +- Mock url-retrieve-synchronously +- Normal: successful fetch returns buffer content +- Error: nil buffer (network failure) → error +- Error: HTTP error codes (404, 500) → error + +**** =wttrin-query= (Line 133) +*Current state*: Mixes data fetching, parsing, buffer creation, display, keymaps + +*Refactoring plan*: +- Create =wttrin--parse-weather-data= (pure): Takes raw string, returns structured data +- Create =wttrin--format-weather-buffer= (pure): Takes structured data, returns formatted string +- Keep =wttrin-query= for buffer/display/keymap setup + +*Tests for =wttrin--parse-weather-data=*: +- Normal: Valid weather data → parsed location, timestamp, body +- Boundary: Minimal valid data +- Error: "ERROR" in response → returns nil or signals error +- Error: Malformed data → handles gracefully + +*Tests for =wttrin--format-weather-buffer=*: +- Normal: Formatted data with location, timestamp, weather +- Boundary: Empty weather body +- Boundary: Very long weather data + +*** Cache Functions (Testable with Mocking) + +**** =wttrin--get-cached-or-fetch= (Line 187) +*Test categories*: +- Normal: Cache hit with fresh data → returns cached data +- Normal: Cache miss → fetches new data and caches it +- Normal: Cache hit with stale data → fetches new data +- Boundary: Force refresh bypasses cache +- Error: Fetch fails, cached data available → returns stale cache +- Error: Fetch fails, no cached data → propagates error + +*** Interactive Functions (Minimal Testing Needed) +These are thin wrappers around logic - test the logic, not the UI: + +- =wttrin= (Line 251) - Just calls wttrin-query with completing-read result +- =wttrin-exit= (Line 117) - Just calls quit-window +- =wttrin-requery= (Line 122) - Just prompts and calls wttrin-query +- =wttrin-requery-force= (Line 240) - Sets flag and calls wttrin-query +- =wttrin-clear-cache= (Line 231) - Just calls clrhash + +** Test File Structure + +Following the quality-engineer.org guidelines: + +*** Unit Test Files (tests/ directory) +- =test-wttrin-additional-url-params.el= +- =test-wttrin--make-cache-key.el= +- =test-wttrin--cleanup-cache-if-needed.el= +- =test-wttrin--build-url.el= (after refactoring) +- =test-wttrin--parse-weather-data.el= (after refactoring) +- =test-wttrin--format-weather-buffer.el= (after refactoring) +- =test-wttrin--get-cached-or-fetch.el= + +*** Integration Test Files +- =test-integration-wttrin-cache-workflow.el= (cache hit/miss/refresh) +- =test-integration-wttrin-fetch-display.el= (fetch → parse → display) + +*** Test Utilities +- =testutil-wttrin.el= (shared test helpers) + +** Refactoring Priority + +*** Phase 1: Test Pure Functions (No Refactoring Needed) +1. =wttrin-additional-url-params= +2. =wttrin--make-cache-key= +3. =wttrin--cleanup-cache-if-needed= + +*** Phase 2: Refactor and Test URL Building +1. Extract =wttrin--build-url= from =wttrin-fetch-raw-string= +2. Write tests for =wttrin--build-url= +3. Extract =wttrin--fetch-url= (I/O portion) +4. Write tests for =wttrin--fetch-url= (mocked) + +*** Phase 3: Refactor and Test Query Logic +1. Extract =wttrin--parse-weather-data= +2. Write tests for parsing +3. Extract =wttrin--format-weather-buffer= +4. Write tests for formatting + +*** Phase 4: Integration Tests +1. Cache workflow tests +2. Fetch-to-display workflow tests + +** Estimated Test Count +- Pure functions: ~30 tests +- Refactored functions: ~40 tests +- Cache logic: ~15 tests +- Integration: ~10 tests +*Total*: ~95 tests + +** Session Progress Tracking +- Session 1 (2025-11-03): Created testing plan, identified functions +- Session 2: TBD - Start with Phase 1 +- Session 3+: Continue through phases diff --git a/tests/test-wttrin--build-url.el b/tests/test-wttrin--build-url.el new file mode 100644 index 0000000..21d3241 --- /dev/null +++ b/tests/test-wttrin--build-url.el @@ -0,0 +1,91 @@ +;;; test-wttrin--build-url.el --- Tests for wttrin--build-url -*- lexical-binding: t; -*- + +;; Copyright (C) 2025 Craig Jennings + +;;; Commentary: + +;; Unit tests for wttrin--build-url function. +;; Tests URL construction with proper encoding and parameters. + +;;; Code: + +(require 'ert) +(require 'wttrin) +(require 'testutil-wttrin) + +;;; Normal Cases + +(ert-deftest test-wttrin--build-url-normal-simple-location-returns-url () + "Test that simple location builds correct URL." + (testutil-wttrin-with-unit-system nil + (should (equal "https://wttr.in/Paris?A" + (wttrin--build-url "Paris"))))) + +(ert-deftest test-wttrin--build-url-normal-location-with-metric-returns-url () + "Test that location with metric unit system builds correct URL." + (testutil-wttrin-with-unit-system "m" + (should (equal "https://wttr.in/London?mA" + (wttrin--build-url "London"))))) + +(ert-deftest test-wttrin--build-url-normal-location-with-uscs-returns-url () + "Test that location with USCS unit system builds correct URL." + (testutil-wttrin-with-unit-system "u" + (should (equal "https://wttr.in/Berlin?uA" + (wttrin--build-url "Berlin"))))) + +;;; Boundary Cases + +(ert-deftest test-wttrin--build-url-boundary-location-with-spaces-encodes-url () + "Test that location with spaces is properly URL-encoded." + (testutil-wttrin-with-unit-system nil + (should (equal "https://wttr.in/New%20York?A" + (wttrin--build-url "New York"))))) + +(ert-deftest test-wttrin--build-url-boundary-location-with-comma-encodes-url () + "Test that location with comma is properly URL-encoded." + (testutil-wttrin-with-unit-system nil + (should (equal "https://wttr.in/New%20York%2C%20NY?A" + (wttrin--build-url "New York, NY"))))) + +(ert-deftest test-wttrin--build-url-boundary-location-with-special-chars-encodes-url () + "Test that location with special characters is properly URL-encoded." + (testutil-wttrin-with-unit-system "m" + ;; ~Eiffel+Tower format is used by wttr.in for landmarks + ;; ~ is an unreserved character (RFC 3986) and is not encoded + ;; + is encoded as %2B + (should (equal "https://wttr.in/~Eiffel%2BTower?mA" + (wttrin--build-url "~Eiffel+Tower"))))) + +(ert-deftest test-wttrin--build-url-boundary-unicode-location-encodes-url () + "Test that Unicode location is properly URL-encoded." + (testutil-wttrin-with-unit-system nil + ;; Unicode should be properly encoded + (let ((result (wttrin--build-url "東京"))) + (should (string-prefix-p "https://wttr.in/" result)) + (should (string-suffix-p "?A" result)) + ;; Should contain URL-encoded Unicode + (should (string-match-p "%[0-9A-F][0-9A-F]" result))))) + +(ert-deftest test-wttrin--build-url-boundary-empty-location-returns-url () + "Test that empty location builds URL with empty query." + (testutil-wttrin-with-unit-system nil + (should (equal "https://wttr.in/?A" + (wttrin--build-url ""))))) + +(ert-deftest test-wttrin--build-url-boundary-gps-coordinates-encodes-url () + "Test that GPS coordinates are properly URL-encoded." + (testutil-wttrin-with-unit-system nil + ;; Format: -78.46,106.79 + (should (equal "https://wttr.in/-78.46%2C106.79?A" + (wttrin--build-url "-78.46,106.79"))))) + +;;; Error Cases + +(ert-deftest test-wttrin--build-url-error-nil-location-signals-error () + "Test that nil location signals an error." + (testutil-wttrin-with-unit-system nil + (should-error (wttrin--build-url nil) + :type 'error))) + +(provide 'test-wttrin--build-url) +;;; test-wttrin--build-url.el ends here diff --git a/tests/test-wttrin--cleanup-cache-if-needed.el b/tests/test-wttrin--cleanup-cache-if-needed.el new file mode 100644 index 0000000..c280723 --- /dev/null +++ b/tests/test-wttrin--cleanup-cache-if-needed.el @@ -0,0 +1,124 @@ +;;; test-wttrin--cleanup-cache-if-needed.el --- Tests for wttrin--cleanup-cache-if-needed -*- lexical-binding: t; -*- + +;; Copyright (C) 2025 Craig Jennings + +;;; Commentary: + +;; Unit tests for wttrin--cleanup-cache-if-needed function. +;; Tests cache eviction when max size is exceeded. + +;;; Code: + +(require 'ert) +(require 'wttrin) +(require 'testutil-wttrin) + +;;; Setup and Teardown + +(defun test-wttrin--cleanup-cache-if-needed-setup () + "Setup for cleanup cache tests." + (testutil-wttrin-setup)) + +(defun test-wttrin--cleanup-cache-if-needed-teardown () + "Teardown for cleanup cache tests." + (testutil-wttrin-teardown)) + +;;; Normal Cases + +(ert-deftest test-wttrin--cleanup-cache-if-needed-normal-under-max-does-nothing () + "Test that cache under max size is not cleaned up." + (test-wttrin--cleanup-cache-if-needed-setup) + (unwind-protect + (testutil-wttrin-with-cache-max 10 + ;; Add 5 entries (under max of 10) + (dotimes (i 5) + (testutil-wttrin-add-to-cache (format "loc%d" i) "data")) + (wttrin--cleanup-cache-if-needed) + (should (= 5 (testutil-wttrin-cache-size)))) + (test-wttrin--cleanup-cache-if-needed-teardown))) + +(ert-deftest test-wttrin--cleanup-cache-if-needed-normal-exceeds-max-removes-oldest () + "Test that cache exceeding max size removes oldest 20% of entries." + (test-wttrin--cleanup-cache-if-needed-setup) + (unwind-protect + (testutil-wttrin-with-cache-max 10 + ;; Add 11 entries (exceeds max of 10) + ;; Entries added earlier should be older + (dotimes (i 11) + (testutil-wttrin-add-to-cache (format "loc%d" i) "data" (* i 10))) + (wttrin--cleanup-cache-if-needed) + ;; Should remove 20% = 2 entries (11/5 = 2.2, rounds to 2) + (should (= 9 (testutil-wttrin-cache-size)))) + (test-wttrin--cleanup-cache-if-needed-teardown))) + +(ert-deftest test-wttrin--cleanup-cache-if-needed-normal-removes-correct-entries () + "Test that cleanup removes the oldest entries based on timestamp." + (test-wttrin--cleanup-cache-if-needed-setup) + (unwind-protect + (testutil-wttrin-with-cache-max 5 + ;; Add 6 entries with specific ages (older = higher age-seconds) + (testutil-wttrin-add-to-cache "old1" "data1" 1000) ; oldest + (testutil-wttrin-add-to-cache "old2" "data2" 900) + (testutil-wttrin-add-to-cache "mid1" "data3" 500) + (testutil-wttrin-add-to-cache "mid2" "data4" 300) + (testutil-wttrin-add-to-cache "new1" "data5" 100) + (testutil-wttrin-add-to-cache "new2" "data6" 50) ; newest + + (wttrin--cleanup-cache-if-needed) + ;; Should remove 20% = 1 entry (6/5 = 1.2, rounds to 1) + ;; Should keep 5 entries + (should (= 5 (testutil-wttrin-cache-size))) + ;; The oldest entry (old1) should be gone + (should-not (gethash (wttrin--make-cache-key "old1") wttrin--cache)) + ;; The newest entries should remain + (should (gethash (wttrin--make-cache-key "new1") wttrin--cache)) + (should (gethash (wttrin--make-cache-key "new2") wttrin--cache))) + (test-wttrin--cleanup-cache-if-needed-teardown))) + +;;; Boundary Cases + +(ert-deftest test-wttrin--cleanup-cache-if-needed-boundary-empty-cache-does-nothing () + "Test that cleanup with empty cache does nothing." + (test-wttrin--cleanup-cache-if-needed-setup) + (unwind-protect + (testutil-wttrin-with-cache-max 10 + (wttrin--cleanup-cache-if-needed) + (should (= 0 (testutil-wttrin-cache-size)))) + (test-wttrin--cleanup-cache-if-needed-teardown))) + +(ert-deftest test-wttrin--cleanup-cache-if-needed-boundary-exactly-at-max-does-nothing () + "Test that cache exactly at max size is not cleaned up." + (test-wttrin--cleanup-cache-if-needed-setup) + (unwind-protect + (testutil-wttrin-with-cache-max 10 + ;; Add exactly 10 entries (at max) + (dotimes (i 10) + (testutil-wttrin-add-to-cache (format "loc%d" i) "data")) + (wttrin--cleanup-cache-if-needed) + (should (= 10 (testutil-wttrin-cache-size)))) + (test-wttrin--cleanup-cache-if-needed-teardown))) + +(ert-deftest test-wttrin--cleanup-cache-if-needed-boundary-one-entry-at-max-one-does-nothing () + "Test that one entry at max=1 does not trigger cleanup." + (test-wttrin--cleanup-cache-if-needed-setup) + (unwind-protect + (testutil-wttrin-with-cache-max 1 + (testutil-wttrin-add-to-cache "loc1" "data") + (wttrin--cleanup-cache-if-needed) + (should (= 1 (testutil-wttrin-cache-size)))) + (test-wttrin--cleanup-cache-if-needed-teardown))) + +(ert-deftest test-wttrin--cleanup-cache-if-needed-boundary-two-entries-at-max-one-removes-none () + "Test that two entries at max=1 removes no entries due to integer division." + (test-wttrin--cleanup-cache-if-needed-setup) + (unwind-protect + (testutil-wttrin-with-cache-max 1 + (testutil-wttrin-add-to-cache "old" "data1" 100) + (testutil-wttrin-add-to-cache "new" "data2" 50) + (wttrin--cleanup-cache-if-needed) + ;; 2 entries / 5 = 0 in integer division, so no entries removed + (should (= 2 (testutil-wttrin-cache-size)))) + (test-wttrin--cleanup-cache-if-needed-teardown))) + +(provide 'test-wttrin--cleanup-cache-if-needed) +;;; test-wttrin--cleanup-cache-if-needed.el ends here diff --git a/tests/test-wttrin--make-cache-key.el b/tests/test-wttrin--make-cache-key.el new file mode 100644 index 0000000..442c548 --- /dev/null +++ b/tests/test-wttrin--make-cache-key.el @@ -0,0 +1,69 @@ +;;; test-wttrin--make-cache-key.el --- Tests for wttrin--make-cache-key -*- lexical-binding: t; -*- + +;; Copyright (C) 2025 Craig Jennings + +;;; Commentary: + +;; Unit tests for wttrin--make-cache-key function. +;; Tests cache key generation from location and unit system. + +;;; Code: + +(require 'ert) +(require 'wttrin) +(require 'testutil-wttrin) + +;;; Normal Cases + +(ert-deftest test-wttrin--make-cache-key-normal-location-with-metric-returns-key () + "Test that location with metric unit system creates correct cache key." + (testutil-wttrin-with-unit-system "m" + (should (equal "Paris|m" (wttrin--make-cache-key "Paris"))))) + +(ert-deftest test-wttrin--make-cache-key-normal-location-with-uscs-returns-key () + "Test that location with USCS unit system creates correct cache key." + (testutil-wttrin-with-unit-system "u" + (should (equal "New York|u" (wttrin--make-cache-key "New York"))))) + +(ert-deftest test-wttrin--make-cache-key-normal-location-no-unit-returns-default () + "Test that location with no unit system uses default in cache key." + (testutil-wttrin-with-unit-system nil + (should (equal "London|default" (wttrin--make-cache-key "London"))))) + +;;; Boundary Cases + +(ert-deftest test-wttrin--make-cache-key-boundary-empty-location-returns-key () + "Test that empty location string creates cache key." + (testutil-wttrin-with-unit-system "m" + (should (equal "|m" (wttrin--make-cache-key ""))))) + +(ert-deftest test-wttrin--make-cache-key-boundary-location-with-spaces-returns-key () + "Test that location with spaces creates correct cache key." + (testutil-wttrin-with-unit-system "m" + (should (equal "New York, NY|m" (wttrin--make-cache-key "New York, NY"))))) + +(ert-deftest test-wttrin--make-cache-key-boundary-location-with-commas-returns-key () + "Test that location with commas creates correct cache key." + (testutil-wttrin-with-unit-system nil + (should (equal "Berlin, DE|default" (wttrin--make-cache-key "Berlin, DE"))))) + +(ert-deftest test-wttrin--make-cache-key-boundary-unicode-location-returns-key () + "Test that Unicode location creates correct cache key." + (testutil-wttrin-with-unit-system "m" + (should (equal "東京|m" (wttrin--make-cache-key "東京"))))) + +(ert-deftest test-wttrin--make-cache-key-boundary-location-with-special-chars-returns-key () + "Test that location with special characters creates cache key." + (testutil-wttrin-with-unit-system "m" + (should (equal "~Eiffel+Tower|m" (wttrin--make-cache-key "~Eiffel+Tower"))))) + +;;; Error Cases + +(ert-deftest test-wttrin--make-cache-key-error-nil-location-returns-key () + "Test that nil location creates cache key with empty string." + ;; Note: concat converts nil to empty string, this documents current behavior + (testutil-wttrin-with-unit-system "m" + (should (equal "|m" (wttrin--make-cache-key nil))))) + +(provide 'test-wttrin--make-cache-key) +;;; test-wttrin--make-cache-key.el ends here diff --git a/tests/test-wttrin-additional-url-params.el b/tests/test-wttrin-additional-url-params.el new file mode 100644 index 0000000..60918a4 --- /dev/null +++ b/tests/test-wttrin-additional-url-params.el @@ -0,0 +1,60 @@ +;;; test-wttrin-additional-url-params.el --- Tests for wttrin-additional-url-params -*- lexical-binding: t; -*- + +;; Copyright (C) 2025 Craig Jennings + +;;; Commentary: + +;; Unit tests for wttrin-additional-url-params function. +;; Tests URL parameter generation with different unit system configurations. + +;;; Code: + +(require 'ert) +(require 'wttrin) +(require 'testutil-wttrin) + +;;; Normal Cases + +(ert-deftest test-wttrin-additional-url-params-normal-metric-returns-param () + "Test that metric unit system returns ?m parameter." + (testutil-wttrin-with-unit-system "m" + (should (equal "?m" (wttrin-additional-url-params))))) + +(ert-deftest test-wttrin-additional-url-params-normal-uscs-returns-param () + "Test that USCS unit system returns ?u parameter." + (testutil-wttrin-with-unit-system "u" + (should (equal "?u" (wttrin-additional-url-params))))) + +(ert-deftest test-wttrin-additional-url-params-normal-wind-speed-returns-param () + "Test that wind speed unit returns ?M parameter." + (testutil-wttrin-with-unit-system "M" + (should (equal "?M" (wttrin-additional-url-params))))) + +;;; Boundary Cases + +(ert-deftest test-wttrin-additional-url-params-boundary-nil-returns-question-mark () + "Test that nil unit system returns just ? parameter." + (testutil-wttrin-with-unit-system nil + (should (equal "?" (wttrin-additional-url-params))))) + +(ert-deftest test-wttrin-additional-url-params-boundary-empty-string-returns-question-mark () + "Test that empty string unit system returns just ? parameter." + (testutil-wttrin-with-unit-system "" + (should (equal "?" (wttrin-additional-url-params))))) + +(ert-deftest test-wttrin-additional-url-params-boundary-single-char-returns-param () + "Test that single character unit system returns ?<char> parameter." + (testutil-wttrin-with-unit-system "x" + (should (equal "?x" (wttrin-additional-url-params))))) + +;;; Error Cases + +(ert-deftest test-wttrin-additional-url-params-error-number-signals-error () + "Test that number unit system signals a type error." + ;; concat requires string or sequence, number causes wrong-type-argument error + (testutil-wttrin-with-unit-system 123 + (should-error (wttrin-additional-url-params) + :type 'wrong-type-argument))) + +(provide 'test-wttrin-additional-url-params) +;;; test-wttrin-additional-url-params.el ends here diff --git a/tests/testutil-wttrin.el b/tests/testutil-wttrin.el new file mode 100644 index 0000000..0e109ad --- /dev/null +++ b/tests/testutil-wttrin.el @@ -0,0 +1,83 @@ +;;; testutil-wttrin.el --- Test utilities for wttrin -*- lexical-binding: t; -*- + +;; Copyright (C) 2025 Craig Jennings + +;;; Commentary: + +;; Shared test utilities for wttrin test suite. +;; Provides helper functions, fixtures, and common setup/teardown functionality. + +;;; Code: + +(require 'ert) + +;;; Test Data Fixtures + +(defconst testutil-wttrin-sample-weather-response + "Weather report: Paris, France + + \\ / Partly cloudy + _ /\"\".-. 22 °C + \\_( ). ↓ 15 km/h + /(___(__) 10 km + 0.0 mm" + "Sample weather response for testing parsing logic.") + +(defconst testutil-wttrin-sample-error-response + "ERROR: Unknown location; please try ~curl wttr.in/:help" + "Sample error response from wttr.in service.") + +;;; Cache Testing Helpers + +(defun testutil-wttrin-clear-cache () + "Clear the wttrin cache for test isolation." + (clrhash wttrin--cache)) + +(defun testutil-wttrin-add-to-cache (location data &optional age-seconds) + "Add DATA to cache for LOCATION, optionally aged by AGE-SECONDS." + (let* ((cache-key (wttrin--make-cache-key location)) + (timestamp (if age-seconds + (- (float-time) age-seconds) + (float-time)))) + (puthash cache-key (cons timestamp data) wttrin--cache))) + +(defun testutil-wttrin-cache-size () + "Return the current number of entries in the cache." + (hash-table-count wttrin--cache)) + +;;; Custom Variable Management + +(defmacro testutil-wttrin-with-unit-system (unit-system &rest body) + "Execute BODY with wttrin-unit-system temporarily set to UNIT-SYSTEM." + (declare (indent 1)) + `(let ((wttrin-unit-system ,unit-system)) + ,@body)) + +(defmacro testutil-wttrin-with-cache-ttl (ttl &rest body) + "Execute BODY with wttrin-cache-ttl temporarily set to TTL." + (declare (indent 1)) + `(let ((wttrin-cache-ttl ,ttl)) + ,@body)) + +(defmacro testutil-wttrin-with-cache-max (max-entries &rest body) + "Execute BODY with wttrin-cache-max-entries temporarily set to MAX-ENTRIES." + (declare (indent 1)) + `(let ((wttrin-cache-max-entries ,max-entries)) + ,@body)) + +;;; Test Setup and Teardown + +(defun testutil-wttrin-setup () + "Common setup for wttrin tests. +Call this at the beginning of each test." + (testutil-wttrin-clear-cache) + (setq wttrin--force-refresh nil)) + +(defun testutil-wttrin-teardown () + "Common teardown for wttrin tests. +Call this at the end of each test." + (testutil-wttrin-clear-cache) + (setq wttrin--force-refresh nil)) + +(provide 'testutil-wttrin) +;;; testutil-wttrin.el ends here @@ -102,17 +102,38 @@ units (default)." (defun wttrin-additional-url-params () "Concatenates extra information into the URL." - (concat "?" wttrin-unit-system)) + (if wttrin-unit-system + (concat "?" wttrin-unit-system) + "?")) + +(defun wttrin--build-url (query) + "Build wttr.in URL for QUERY with configured parameters. +This is a pure function with no side effects, suitable for testing." + (when (null query) + (error "Query cannot be nil")) + (concat "https://wttr.in/" + (url-hexify-string query) + (wttrin-additional-url-params) + "A")) (defun wttrin-fetch-raw-string (query) - "Get the weather information based on your QUERY." - (let ((url-request-extra-headers (list wttrin-default-languages)) - (url-user-agent "curl")) - (with-current-buffer - (url-retrieve-synchronously - (concat "https://wttr.in/" query (wttrin-additional-url-params) "A") - (lambda () (switch-to-buffer (current-buffer)))) - (decode-coding-string (buffer-string) 'utf-8)))) + "Get the weather information based on your QUERY. +Returns the weather data as a string, or signals an error on failure." + (let* ((url (wttrin--build-url query)) + (url-request-extra-headers (list wttrin-default-languages)) + (url-user-agent "curl") + (buf (url-retrieve-synchronously url t t))) + (unless buf + (error "wttrin: Network failure - could not retrieve %S" query)) + (unwind-protect + (with-current-buffer buf + ;; Skip HTTP headers + (goto-char (point-min)) + (re-search-forward "\r?\n\r?\n" nil t) + (decode-coding-string + (buffer-substring-no-properties (point) (point-max)) + 'utf-8)) + (kill-buffer buf)))) (defun wttrin-exit () "Exit the wttrin buffer." @@ -197,8 +218,7 @@ Returns the weather data string or nil on error." (not wttrin--force-refresh)) data (condition-case err - (let ((fresh-data (wttrin-fetch-raw-string - (concat location (wttrin-additional-url-params))))) + (let ((fresh-data (wttrin-fetch-raw-string location))) (when fresh-data (wttrin--cleanup-cache-if-needed) (puthash cache-key (cons now fresh-data) wttrin--cache)) |