Subject: [PATCH] Add some more Eshell history tests

From: Morgan Smith <[email-masked]>
Date: Mon, 17 Mar 2025 15:52:15 -0400

Tags: patch

This patch just adds some tests so I don't think that should be too
controversial

I have included some comments in the patch that I would like to talk
about though.

In the test `em-hist-test/eshell-history-file-name/nil' we don't save
any history to a file because `eshell-history-file-name' is nil and the
environment variable HISTFILE is unset.  I would like to see some user
visible warning here.  In theory this could be intended behavior to
prevent any history from being saved to disk but I think the more likely
case is the user accidentally misconfigured it.

I added a comment to the existing test
`em-hist-test/write-history/overwrite-multiple-shells' as I don't like
the behavior it demonstrates.  I was experiencing the occasional history
loss for years before I finally decided to investigate and now I know to
set `eshell-history-append' to true.

While writing this email I discovered Bug#66700 which seemed to come to
the conclusion that we should set `eshell-history-append' to true in the
next version of Emacs.  However, this would effectively make
`eshell-hist-ignoredups' useless.  I would be interested in a solution
where we check if the histfile has been updated to make sure we
overwrite without history loss.

>From 3036053cc1ce861cc926efa6fe1ca544a0eb6621 Mon Sep 17 00:00:00 2001
From: Morgan Smith <[email-masked]>
Date: Sun, 16 Mar 2025 15:06:44 -0400
Subject: [PATCH] Add some more Eshell history tests

* test/lisp/eshell/em-hist-tests.el
(em-hist-test/eshell-history-file-name/nil)
(em-hist-test/eshell-history-file-name/env)
(em-hist-test/eshell-history-size/nil)
(em-hist-test/eshell-history-size): New tests.
(em-hist-test/write-history/overwrite-multiple-shells): Add comment.
---
 test/lisp/eshell/em-hist-tests.el | 64 +++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/test/lisp/eshell/em-hist-tests.el b/test/lisp/eshell/em-hist-tests.el
index 6288ae071dc..71c08c99e02 100644
--- a/test/lisp/eshell/em-hist-tests.el
+++ b/test/lisp/eshell/em-hist-tests.el
@@ -51,6 +51,65 @@ em-hist-test/check-history-file
 
 ;;; Tests:
 
+(ert-deftest em-hist-test/eshell-history-file-name/nil ()
+  "Test setting 'history-file-name' and environment variable 'HISTFILE' to nil."
+  ;; It might be nice to warn the user in this case!  That would have
+  ;; saved me time debugging my init file.
+  (with-temp-eshell
+    (eshell-insert-command "echo hi")
+    (eshell-insert-command "echo bye")
+    (eshell-save-some-history) ; ensure no error
+    (should (equal (nreverse (ring-elements eshell-history-ring))
+                   '("echo hi" "echo bye")))))
+
+(ert-deftest em-hist-test/eshell-history-file-name/env ()
+  "Test setting history file from the environment variable 'HISTFILE'."
+  (ert-with-temp-file histfile
+    ;; A hack to set 'HISTFILE' as the test infrastructure clears it
+    (let ((buffer-list-update-hook
+           (list (lambda ()
+                   (setq process-environment
+                         (cons (concat "HISTFILE=" histfile)
+                               process-environment))))))
+      (with-temp-eshell
+        (em-hist-test/check-history-file histfile nil)
+        (eshell-insert-command "echo hi")
+        (eshell-save-some-history)
+        (em-hist-test/check-history-file histfile '("echo hi"))
+        (eshell-insert-command "echo bye")
+        (eshell-save-some-history)
+        (em-hist-test/check-history-file histfile '("echo hi" "echo bye"))))))
+
+(ert-deftest em-hist-test/eshell-history-size/nil ()
+  "Test setting `eshell-history-size' to nil.
+We should fallback to the default value."
+  (let ((process-environment (cons "HISTSIZE" process-environment))
+        (eshell-history-size nil))
+    (with-temp-eshell
+      (should
+       (equal eshell-history-size
+              (eval (car (get 'eshell-history-size 'standard-value)) t))))))
+
+(ert-deftest em-hist-test/eshell-history-size ()
+  "Test setting `eshell-history-size'."
+  (ert-with-temp-file histfile
+    (let ((process-environment (cons "HISTSIZE" process-environment))
+          (eshell-history-size 1))
+      (with-temp-eshell
+        (em-hist-test/check-history-file histfile nil)
+        (eshell-insert-command "echo hi")
+        (eshell-write-history histfile 'append)
+        (em-hist-test/check-history-file histfile '("echo hi"))
+        (eshell-insert-command "echo bye")
+        (eshell-write-history histfile 'append)
+        (em-hist-test/check-history-file histfile
+                                         '("echo hi" "echo bye")
+                                         '("echo bye"))
+        (eshell-insert-command "echo lost history")
+        (eshell-write-history histfile)
+        (em-hist-test/check-history-file histfile
+                                         '("echo lost history"))))))
+
 (ert-deftest em-hist-test/write-history/append ()
   "Test appending new history to history file."
   (ert-with-temp-file histfile
@@ -103,6 +162,11 @@ em-hist-test/write-history/overwrite
 
 (ert-deftest em-hist-test/write-history/overwrite-multiple-shells ()
   "Test overwriting history file from multiple Eshells."
+  ;; Is this really the behavior we want?  This is how eshell comes out
+  ;; of the box with zero configuration.  Users using multiple eshells
+  ;; are going to be missing history in a way that is going to be hard
+  ;; for them to track down as it depends on when shells are opened and
+  ;; closed relative to each other.
   (ert-with-temp-file histfile
     (with-temp-eshell
      (with-temp-eshell
-- 
2.48.1