From 9da34b2f8ab8edb93f7ecf40adcf4973ec91d4ef Mon Sep 17 00:00:00 2001 From: Todd Leonhardt <2812720+tleonhardt@users.noreply.github.com> Date: Sat, 31 Oct 2026 15:47:38 -0400 Subject: [PATCH] Address review feedback on persisted history copy Follow-up to #1796, applying review comments made before it merged. Comment why _persist_history() truncates a copy, so it is not later simplified back into truncating the live history. Trim the new tests. Drop the no-file, empty-history, and write-error cases, which either could not fail or duplicated existing tests, and instead assert unchanged live state in the existing permission-error test with a limit shorter than the history. Drop the reload half of the repeat/append test, which re-tested loading. Simplify the helpers: import lzma directly, move the History import to module level, and stop comparing span and search results that follow from the item identities and session boundary already compared. --- cmd2/cmd2.py | 1 + tests/test_history.py | 102 ++++++++++++------------------------------ 2 files changed, 29 insertions(+), 74 deletions(-) diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py index 4f9003e87..090b50a9c 100644 --- a/cmd2/cmd2.py +++ b/cmd2/cmd2.py @@ -5722,6 +5722,7 @@ def _persist_history(self) -> None: except ModuleNotFoundError: # pragma: no cover import bz2 as compress_lib # type: ignore[no-redef] + # Truncate a copy so live history numbering and the session boundary are unchanged. history = History(self.history) history.truncate(self._persistent_history_length) history_json = history.to_json() diff --git a/tests/test_history.py b/tests/test_history.py index f476739e4..00e758e2f 100644 --- a/tests/test_history.py +++ b/tests/test_history.py @@ -13,7 +13,7 @@ import pytest import cmd2 -from cmd2.history import HistoryFormatError, HistoryIndexError +from cmd2.history import History, HistoryFormatError, HistoryIndexError from cmd2.parsing import Statement from .conftest import ( @@ -1745,26 +1745,6 @@ def test_history_populates_pt(hist_file) -> None: # we assume that the atexit module will call this method # properly # -def test_persist_history_ensure_no_error_if_no_histfile(base_app, capsys) -> None: - # make sure if there is no persistent history file and someone - # calls the private method call that we don't get an error - base_app._persist_history() - out, err = capsys.readouterr() - assert not out - assert not err - - -def test_persist_history_permission_error(hist_file, mocker, capsys) -> None: - app = cmd2.Cmd(persistent_history_file=hist_file) - run_cmd(app, "help") - mock_open = mocker.patch("builtins.open") - mock_open.side_effect = PermissionError - app._persist_history() - out, err = capsys.readouterr() - assert not out - assert "Cannot write persistent history file" in err - - @pytest.fixture def persistence_app(base_app, hist, tmp_path): base_app.history = hist @@ -1774,15 +1754,10 @@ def persistence_app(base_app, hist, tmp_path): def read_persisted_history(app): - try: - import lzma as compress_lib - except ModuleNotFoundError: - import bz2 as compress_lib - - from cmd2.history import History + import lzma with open(app.persistent_history_file, "rb") as stream: - return History.from_json(compress_lib.decompress(stream.read()).decode("utf-8")) + return History.from_json(lzma.decompress(stream.read()).decode("utf-8")) def history_state(app): @@ -1792,11 +1767,33 @@ def history_state(app): tuple(id(item) for item in history), history.to_json(), history.session_start_index, - history.span(":"), - history.str_search("third"), ) +def test_persist_history_ensure_no_error_if_no_histfile(base_app, capsys) -> None: + # make sure if there is no persistent history file and someone + # calls the private method call that we don't get an error + base_app._persist_history() + out, err = capsys.readouterr() + assert not out + assert not err + + +def test_persist_history_permission_error(hist_file, mocker, capsys) -> None: + app = cmd2.Cmd(persistent_history_file=hist_file) + run_cmd(app, "help") + run_cmd(app, "shortcuts") + app._persistent_history_length = 1 + before = history_state(app) + mock_open = mocker.patch("builtins.open") + mock_open.side_effect = PermissionError + app._persist_history() + out, err = capsys.readouterr() + assert not out + assert "Cannot write persistent history file" in err + assert history_state(app) == before + + @pytest.mark.parametrize("limit", [-1, 0, 1, 2, 4, 6]) def test_persist_history_preserves_live_state(persistence_app, limit) -> None: app = persistence_app @@ -1810,17 +1807,7 @@ def test_persist_history_preserves_live_state(persistence_app, limit) -> None: assert history_state(app) == before -def test_persist_history_empty(persistence_app) -> None: - app = persistence_app - app.history.clear() - app._persistent_history_length = 2 - before = history_state(app) - app._persist_history() - assert not read_persisted_history(app) - assert history_state(app) == before - - -def test_persist_history_repeat_append_and_reload(persistence_app, mocker) -> None: +def test_persist_history_repeat_and_append(persistence_app) -> None: app = persistence_app app._persistent_history_length = 2 before = history_state(app) @@ -1833,36 +1820,3 @@ def test_persist_history_repeat_append_and_reload(persistence_app, mocker) -> No app._persist_history() assert [item.raw for item in read_persisted_history(app)] == ["fourth", "fifth café"] assert history_state(app) == before - assert app.history.get(1).raw == "first" - assert list(app.history.span(":")) == [3, 4, 5] - - # The reload is temporary and must not register a later write to a deleted file. - mocker.patch("atexit.register") - reloaded = cmd2.Cmd(persistent_history_file=app.persistent_history_file) - assert [item.raw for item in reloaded.history] == ["fourth", "fifth café"] - assert reloaded.history.session_start_index == 2 - assert reloaded.main_session.history.get_strings() == ["fourth", "fifth café"] - assert history_state(app) == before - - -def test_persist_history_no_file_preserves_live_state(persistence_app, mocker) -> None: - app = persistence_app - app.persistent_history_file = "" - app._persistent_history_length = 1 - before = history_state(app) - open_mock = mocker.patch("builtins.open") - app._persist_history() - open_mock.assert_not_called() - assert history_state(app) == before - - -def test_persist_history_write_error_preserves_live_state(persistence_app, mocker) -> None: - app = persistence_app - app._persistent_history_length = 1 - before = history_state(app) - mocker.patch("builtins.open", side_effect=PermissionError("synthetic write failure")) - error = mocker.patch.object(app, "perror") - app._persist_history() - assert "Cannot write persistent history file" in error.call_args.args[0] - assert "synthetic write failure" in error.call_args.args[0] - assert history_state(app) == before