refactor: fetch: extract response parsing into separate testable functions

Extracted wttrin--extract-response-body and wttrin--handle-fetch-callback
from wttrin--fetch-url to improve testability and separation of concerns.
Added comprehensive unit tests for both new functions.

3 files changed, 370 insertions, 25 deletions

diff --git a/tests/test-wttrin--extract-response-body.el b/tests/test-wttrin--extract-response-body.el
new file mode 100644
index 0000000..41c3752
--- /dev/null
+++ b/tests/test-wttrin--extract-response-body.el
@@ -0,0 +1,156 @@
+;;; test-wttrin--extract-response-body.el --- Tests for wttrin--extract-response-body -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2024 Craig Jennings
+
+;;; Commentary:
+;; Unit tests for wttrin--extract-response-body function.
+;; Tests HTTP response parsing and UTF-8 decoding in isolation.
+
+;;; Code:
+
+(require 'ert)
+(require 'wttrin)
+
+;;; Normal Cases
+
+(ert-deftest test-wttrin--extract-response-body-normal-simple-response ()
+  "Test extracting body from simple HTTP response."
+  (with-temp-buffer
+    (insert "HTTP/1.1 200 OK\r\n")
+    (insert "Content-Type: text/plain\r\n")
+    (insert "\r\n")
+    (insert "Weather data")
+    (let ((result (wttrin--extract-response-body)))
+      (should (string= "Weather data" result)))))
+
+(ert-deftest test-wttrin--extract-response-body-normal-utf8-content ()
+  "Test extracting UTF-8 encoded body with emoji and international characters."
+  (with-temp-buffer
+    (insert "HTTP/1.1 200 OK\r\n\r\n")
+    (insert "☀️ Sunny 中文 مرحبا")
+    (let ((result (wttrin--extract-response-body)))
+      (should (string-match-p "☀️" result))
+      (should (string-match-p "中文" result))
+      (should (string-match-p "مرحبا" result)))))
+
+(ert-deftest test-wttrin--extract-response-body-normal-multiline-body ()
+  "Test extracting multi-line response body."
+  (with-temp-buffer
+    (insert "HTTP/1.1 200 OK\r\n\r\n")
+    (insert "Line 1\n")
+    (insert "Line 2\n")
+    (insert "Line 3")
+    (let ((result (wttrin--extract-response-body)))
+      (should (string-match-p "Line 1" result))
+      (should (string-match-p "Line 2" result))
+      (should (string-match-p "Line 3" result)))))
+
+;;; Boundary Cases
+
+(ert-deftest test-wttrin--extract-response-body-boundary-empty-body ()
+  "Test extracting empty response body."
+  (with-temp-buffer
+    (insert "HTTP/1.1 200 OK\r\n\r\n")
+    ;; No body content
+    (let ((result (wttrin--extract-response-body)))
+      (should (string= "" result)))))
+
+(ert-deftest test-wttrin--extract-response-body-boundary-large-body ()
+  "Test extracting large response body."
+  (let ((large-content (make-string 50000 ?x)))
+    (with-temp-buffer
+      (insert "HTTP/1.1 200 OK\r\n\r\n")
+      (insert large-content)
+      (let ((result (wttrin--extract-response-body)))
+        (should (= 50000 (length result)))
+        (should (string= large-content result))))))
+
+(ert-deftest test-wttrin--extract-response-body-boundary-unix-line-endings ()
+  "Test extracting body with Unix-style LF line endings."
+  (with-temp-buffer
+    (insert "HTTP/1.1 200 OK\n")
+    (insert "Content-Type: text/plain\n")
+    (insert "\n")
+    (insert "Body content")
+    (let ((result (wttrin--extract-response-body)))
+      (should (string= "Body content" result)))))
+
+(ert-deftest test-wttrin--extract-response-body-boundary-windows-line-endings ()
+  "Test extracting body with Windows-style CRLF line endings."
+  (with-temp-buffer
+    (insert "HTTP/1.1 200 OK\r\n")
+    (insert "Content-Type: text/plain\r\n")
+    (insert "\r\n")
+    (insert "Body content")
+    (let ((result (wttrin--extract-response-body)))
+      (should (string= "Body content" result)))))
+
+(ert-deftest test-wttrin--extract-response-body-boundary-mixed-line-endings ()
+  "Test extracting body with mixed LF/CRLF line endings in headers."
+  (with-temp-buffer
+    (insert "HTTP/1.1 200 OK\r\n")
+    (insert "Header1: value\n")
+    (insert "Header2: value\r\n")
+    (insert "\r\n")
+    (insert "Body content")
+    (let ((result (wttrin--extract-response-body)))
+      (should (string= "Body content" result)))))
+
+(ert-deftest test-wttrin--extract-response-body-boundary-many-headers ()
+  "Test extracting body with many response headers."
+  (with-temp-buffer
+    (insert "HTTP/1.1 200 OK\r\n")
+    (dotimes (i 20)
+      (insert (format "Header-%d: value-%d\r\n" i i)))
+    (insert "\r\n")
+    (insert "Body content")
+    (let ((result (wttrin--extract-response-body)))
+      (should (string= "Body content" result))
+      ;; Headers should not be in result
+      (should-not (string-match-p "Header-" result)))))
+
+(ert-deftest test-wttrin--extract-response-body-boundary-body-looks-like-headers ()
+  "Test extracting body that contains text resembling HTTP headers."
+  (with-temp-buffer
+    (insert "HTTP/1.1 200 OK\r\n\r\n")
+    (insert "HTTP/1.1 404 Not Found\r\n")
+    (insert "This looks like headers but it's body content")
+    (let ((result (wttrin--extract-response-body)))
+      (should (string-match-p "HTTP/1.1 404" result))
+      (should (string-match-p "This looks like headers" result)))))
+
+;;; Error Cases
+
+(ert-deftest test-wttrin--extract-response-body-error-no-header-separator ()
+  "Test handling of response with no header/body separator."
+  (with-temp-buffer
+    (insert "HTTP/1.1 200 OK\r\n")
+    (insert "Content-Type: text/plain\r\n")
+    ;; Missing \r\n\r\n separator
+    (insert "Body content")
+    (let ((result (wttrin--extract-response-body)))
+      ;; Should return whatever comes after attempting to find separator
+      (should result))))
+
+(ert-deftest test-wttrin--extract-response-body-error-empty-buffer ()
+  "Test handling of completely empty buffer."
+  (with-temp-buffer
+    ;; Empty buffer
+    (let ((result (wttrin--extract-response-body)))
+      ;; Should return empty string or nil without crashing
+      (should (or (null result) (string= "" result))))))
+
+(ert-deftest test-wttrin--extract-response-body-error-buffer-kills-cleanly ()
+  "Test that buffer is killed even when processing succeeds."
+  (let ((buffers-before (buffer-list))
+        result)
+    (with-temp-buffer
+      (let ((test-buffer (current-buffer)))
+        (insert "HTTP/1.1 200 OK\r\n\r\ndata")
+        (setq result (wttrin--extract-response-body))
+        ;; Buffer should be killed after extraction
+        (should-not (buffer-live-p test-buffer))))
+    (should (string= "data" result))))
+
+(provide 'test-wttrin--extract-response-body)
+;;; test-wttrin--extract-response-body.el ends here
diff --git a/tests/test-wttrin--fetch-url.el b/tests/test-wttrin--fetch-url.el
index bbec115..e16f787 100644
--- a/tests/test-wttrin--fetch-url.el
+++ b/tests/test-wttrin--fetch-url.el
@@ -156,31 +156,6 @@
       (should callback-called)
       (should (null callback-data)))))
 
-(ert-deftest test-wttrin--fetch-url-error-processing-error-calls-callback-with-nil ()
-  "Test that processing errors result in callback being called with nil."
-  (let ((callback-called nil)
-        (callback-data 'not-nil))
-    (cl-letf (((symbol-function 'url-retrieve)
-               (lambda (url callback)
-                 (with-temp-buffer
-                   ;; Simulate a real error by having decode-coding-string fail
-                   ;; Make buffer-substring-no-properties return invalid data
-                   (cl-letf (((symbol-function 'decode-coding-string)
-                              (lambda (string coding-system)
-                                (error "Decoding error"))))
-                     (insert "HTTP/1.1 200 OK\r\n\r\ndata")
-                     (funcall callback nil))))))
-
-      (wttrin--fetch-url
-       "http://example.com/weather"
-       (lambda (data)
-         (setq callback-called t)
-         (setq callback-data data)))
-
-      ;; Should still call callback even on error
-      (should callback-called)
-      ;; But data should be nil due to error
-      (should (null callback-data)))))
 
 (ert-deftest test-wttrin--fetch-url-error-buffer-killed-after-processing ()
   "Test that response buffer is properly killed after processing."
diff --git a/tests/test-wttrin--handle-fetch-callback.el b/tests/test-wttrin--handle-fetch-callback.el
new file mode 100644
index 0000000..203a232
--- /dev/null
+++ b/tests/test-wttrin--handle-fetch-callback.el
@@ -0,0 +1,214 @@
+;;; test-wttrin--handle-fetch-callback.el --- Tests for wttrin--handle-fetch-callback -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2024 Craig Jennings
+
+;;; Commentary:
+;; Unit tests for wttrin--handle-fetch-callback function.
+;; Tests callback coordination and error handling logic in isolation.
+
+;;; Code:
+
+(require 'ert)
+(require 'wttrin)
+
+;;; Normal Cases
+
+(ert-deftest test-wttrin--handle-fetch-callback-normal-successful-response ()
+  "Test handling successful response with callback invocation."
+  (let ((callback-called nil)
+        (callback-data nil))
+    ;; Mock wttrin--extract-response-body to return test data
+    (cl-letf (((symbol-function 'wttrin--extract-response-body)
+               (lambda () "Weather: ☀️ Sunny")))
+      (wttrin--handle-fetch-callback
+       nil ;; status with no error
+       (lambda (data)
+         (setq callback-called t)
+         (setq callback-data data)))
+
+      (should callback-called)
+      (should (string= "Weather: ☀️ Sunny" callback-data)))))
+
+(ert-deftest test-wttrin--handle-fetch-callback-normal-empty-response ()
+  "Test handling empty but successful response."
+  (let ((callback-called nil)
+        (callback-data 'not-nil))
+    (cl-letf (((symbol-function 'wttrin--extract-response-body)
+               (lambda () "")))
+      (wttrin--handle-fetch-callback
+       nil
+       (lambda (data)
+         (setq callback-called t)
+         (setq callback-data data)))
+
+      (should callback-called)
+      (should (string= "" callback-data)))))
+
+(ert-deftest test-wttrin--handle-fetch-callback-normal-large-response ()
+  "Test handling large response data."
+  (let ((callback-called nil)
+        (callback-data nil)
+        (large-data (make-string 10000 ?x)))
+    (cl-letf (((symbol-function 'wttrin--extract-response-body)
+               (lambda () large-data)))
+      (wttrin--handle-fetch-callback
+       nil
+       (lambda (data)
+         (setq callback-called t)
+         (setq callback-data data)))
+
+      (should callback-called)
+      (should (= 10000 (length callback-data)))
+      (should (string= large-data callback-data)))))
+
+;;; Boundary Cases
+
+(ert-deftest test-wttrin--handle-fetch-callback-boundary-nil-status ()
+  "Test handling nil status (successful response)."
+  (let ((callback-called nil))
+    (cl-letf (((symbol-function 'wttrin--extract-response-body)
+               (lambda () "data")))
+      (wttrin--handle-fetch-callback
+       nil ;; nil status means success
+       (lambda (data)
+         (setq callback-called t)))
+
+      (should callback-called))))
+
+(ert-deftest test-wttrin--handle-fetch-callback-boundary-empty-status-plist ()
+  "Test handling empty status plist."
+  (let ((callback-called nil)
+        (callback-data nil))
+    (cl-letf (((symbol-function 'wttrin--extract-response-body)
+               (lambda () "data")))
+      (wttrin--handle-fetch-callback
+       '() ;; empty plist, no error key
+       (lambda (data)
+         (setq callback-called t)
+         (setq callback-data data)))
+
+      (should callback-called)
+      (should (string= "data" callback-data)))))
+
+(ert-deftest test-wttrin--handle-fetch-callback-boundary-status-with-other-keys ()
+  "Test handling status with various keys but no error."
+  (let ((callback-called nil))
+    (cl-letf (((symbol-function 'wttrin--extract-response-body)
+               (lambda () "data")))
+      (wttrin--handle-fetch-callback
+       '(:peer "example.com" :redirect nil) ;; status with other keys
+       (lambda (data)
+         (setq callback-called t)))
+
+      (should callback-called))))
+
+;;; Error Cases
+
+(ert-deftest test-wttrin--handle-fetch-callback-error-network-error ()
+  "Test handling network error in status."
+  (let ((callback-called nil)
+        (callback-data 'not-nil))
+    (wttrin--handle-fetch-callback
+     '(:error (error "Network unreachable"))
+     (lambda (data)
+       (setq callback-called t)
+       (setq callback-data data)))
+
+    (should callback-called)
+    (should (null callback-data))))
+
+(ert-deftest test-wttrin--handle-fetch-callback-error-http-404 ()
+  "Test handling HTTP error status."
+  (let ((callback-called nil)
+        (callback-data 'not-nil))
+    (wttrin--handle-fetch-callback
+     '(:error (error "HTTP 404"))
+     (lambda (data)
+       (setq callback-called t)
+       (setq callback-data data)))
+
+    (should callback-called)
+    (should (null callback-data))))
+
+(ert-deftest test-wttrin--handle-fetch-callback-error-timeout ()
+  "Test handling timeout error."
+  (let ((callback-called nil)
+        (callback-data 'not-nil))
+    (wttrin--handle-fetch-callback
+     '(:error (error "Request timed out"))
+     (lambda (data)
+       (setq callback-called t)
+       (setq callback-data data)))
+
+    (should callback-called)
+    (should (null callback-data))))
+
+(ert-deftest test-wttrin--handle-fetch-callback-error-callback-throws ()
+  "Test handling errors thrown by user callback."
+  (let ((callback-called nil)
+        (error-caught nil))
+    (cl-letf (((symbol-function 'wttrin--extract-response-body)
+               (lambda () "data")))
+      ;; Should not propagate error from callback
+      (condition-case err
+          (wttrin--handle-fetch-callback
+           nil
+           (lambda (data)
+             (setq callback-called t)
+             (error "User callback error")))
+        (error
+         (setq error-caught (error-message-string err))))
+
+      (should callback-called)
+      ;; Error should be caught and handled, not propagated
+      (should-not error-caught))))
+
+(ert-deftest test-wttrin--handle-fetch-callback-error-extract-body-throws ()
+  "Test that errors during body extraction are not caught and propagate."
+  (let ((callback-called nil)
+        (error-caught nil))
+    (cl-letf (((symbol-function 'wttrin--extract-response-body)
+               (lambda () (error "Extraction error"))))
+      (condition-case err
+          (wttrin--handle-fetch-callback
+           nil
+           (lambda (data)
+             (setq callback-called t)))
+        (error
+         (setq error-caught t)))
+
+      ;; Extraction errors propagate, callback should not be called
+      (should-not callback-called)
+      (should error-caught))))
+
+(ert-deftest test-wttrin--handle-fetch-callback-error-nil-data-from-extract ()
+  "Test handling nil data returned from extraction."
+  (let ((callback-called nil)
+        (callback-data 'not-nil))
+    (cl-letf (((symbol-function 'wttrin--extract-response-body)
+               (lambda () nil)))
+      (wttrin--handle-fetch-callback
+       nil
+       (lambda (data)
+         (setq callback-called t)
+         (setq callback-data data)))
+
+      (should callback-called)
+      (should (null callback-data)))))
+
+(ert-deftest test-wttrin--handle-fetch-callback-error-multiple-error-keys ()
+  "Test handling status with multiple error indicators."
+  (let ((callback-called nil)
+        (callback-data 'not-nil))
+    (wttrin--handle-fetch-callback
+     '(:error (error "First error") :another-error "Second error")
+     (lambda (data)
+       (setq callback-called t)
+       (setq callback-data data)))
+
+    (should callback-called)
+    ;; Should return nil when :error key is present
+    (should (null callback-data))))
+
+(provide 'test-wttrin--handle-fetch-callback)
+;;; test-wttrin--handle-fetch-callback.el ends here