From 6cf0aac96ae0abb455ee8525e0ede9a63f5974f3 Mon Sep 17 00:00:00 2001
From: Craig Jennings <c@cjennings.net>
Date: Tue, 4 Nov 2025 07:56:31 -0600
Subject: Add comprehensive ERT test suite and fix critical bugs
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Session 1: Testing infrastructure and initial test coverage

Bug fixes in wttrin.el:
- Fix wttrin-additional-url-params to handle nil unit system
- Remove incorrect callback parameter to url-retrieve-synchronously
- Add nil buffer check for network failures
- Strip HTTP headers before decoding response
- Kill buffer after fetch to prevent memory leaks
- Fix double concatenation of URL params in cache function
- Add proper URL encoding via new wttrin--build-url function

Refactoring:
- Extract wttrin--build-url as pure, testable function
- Separate URL building logic from network I/O

Test infrastructure (33 tests, 100% passing):
- tests/testutil-wttrin.el: Shared test utilities
- tests/test-wttrin-additional-url-params.el: 7 tests
- tests/test-wttrin--make-cache-key.el: 9 tests
- tests/test-wttrin--build-url.el: 10 tests
- tests/test-wttrin--cleanup-cache-if-needed.el: 7 tests

Documentation:
- docs/testing-plan.org: Comprehensive testing roadmap
- docs/bugs.org: Bug analysis from code review
- docs/NOTES.org: Session tracking and guidelines
- docs/session-1-summary.org: Detailed session summary

Next session: Cache workflow tests, parsing logic extraction, integration tests

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
---
 tests/test-wttrin--build-url.el               |  91 +++++++++++++++++++
 tests/test-wttrin--cleanup-cache-if-needed.el | 124 ++++++++++++++++++++++++++
 tests/test-wttrin--make-cache-key.el          |  69 ++++++++++++++
 tests/test-wttrin-additional-url-params.el    |  60 +++++++++++++
 tests/testutil-wttrin.el                      |  83 +++++++++++++++++
 5 files changed, 427 insertions(+)
 create mode 100644 tests/test-wttrin--build-url.el
 create mode 100644 tests/test-wttrin--cleanup-cache-if-needed.el
 create mode 100644 tests/test-wttrin--make-cache-key.el
 create mode 100644 tests/test-wttrin-additional-url-params.el
 create mode 100644 tests/testutil-wttrin.el

(limited to 'tests')

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
-- 
cgit v1.2.3