feat(zfs): enhance command handling with run_zfs and run_zpool functions

tests/test_zfs.py

@@ -2,15 +2,35 @@
 
 import json
 from datetime import UTC, datetime
+from subprocess import CompletedProcess
+from typing import TYPE_CHECKING
 from unittest.mock import call
 
 import pytest
-from pytest_mock import MockerFixture
 
-from python.zfs import Dataset, Snapshot, Zpool, get_datasets
+from python.zfs import (
+    CommandResult,
+    Dataset,
+    Snapshot,
+    Zpool,
+    create_dataset,
+    get_datasets,
+    get_properties,
+    list_dataset_names,
+    run_zfs,
+    run_zpool,
+    set_property,
+)
+
+# Not re-exported from python.zfs on purpose: the wrappers are the default.
+from python.zfs.command import run_command
 from python.zfs.dataset import _zfs_list
 from python.zfs.zpool import _zpool_list
 
+if TYPE_CHECKING:
+    from pytest_mock import MockerFixture
+
+COMMAND = "python.zfs.command"
 DATASET = "python.zfs.dataset"
 ZPOOL = "python.zfs.zpool"
 SAMPLE_SNAPSHOT_DATA = {
@@ -207,12 +227,20 @@ def test_zfs_list_version_check(mocker: MockerFixture) -> None:
 
 def test_get_datasets(mocker: MockerFixture) -> None:
     """Test get_datasets."""
-    mock_bash = mocker.patch(f"{DATASET}.bash_wrapper", return_value=("pool/dataset\npool/other\ninvalid", 0))
+    mock_run = mocker.patch(
+        f"{DATASET}.run_zfs",
+        return_value=CommandResult(
+            args=(),
+            stdout="pool/dataset\npool/other\ninvalid",
+            stderr="",
+            return_code=0,
+        ),
+    )
     mock_dataset = mocker.patch(f"{DATASET}.Dataset")
 
     get_datasets()
 
-    mock_bash.assert_called_once_with("zfs list -Hp -t filesystem -o name")
+    mock_run.assert_called_once_with("list", "-Hp", "-t", "filesystem", "-o", "name")
 
     calls = [call("pool/dataset"), call("pool/other")]
 
@@ -287,11 +315,16 @@ def test_zpool_repr(mocker: MockerFixture) -> None:
 def test_zpool_list(mocker: MockerFixture) -> None:
     """Test version validation in _zpool_list."""
     mocker.patch(
-        f"{ZPOOL}.bash_wrapper",
-        return_value=(json.dumps({"output_version": {"vers_major": 0, "vers_minor": 1, "command": "zpool list"}}), 0),
+        f"{ZPOOL}.run_zpool",
+        return_value=CommandResult(
+            args=(),
+            stdout=json.dumps({"output_version": {"vers_major": 0, "vers_minor": 1, "command": "zpool list"}}),
+            stderr="",
+            return_code=0,
+        ),
     )
 
-    result = _zpool_list("zpool list invalid -pHj -o all")
+    result = _zpool_list("list", "invalid", "-pHj", "-o", "all")
 
     assert result == {"output_version": {"command": "zpool list", "vers_major": 0, "vers_minor": 1}}
 
@@ -299,11 +332,352 @@ def test_zpool_list(mocker: MockerFixture) -> None:
 def test_zpool_list_version_check(mocker: MockerFixture) -> None:
     """Test version validation in _zpool_list."""
     mocker.patch(
-        f"{ZPOOL}.bash_wrapper",
-        return_value=(json.dumps({"output_version": {"vers_major": 1, "vers_minor": 0, "command": "zpool list"}}), 0),
+        f"{ZPOOL}.run_zpool",
+        return_value=CommandResult(
+            args=(),
+            stdout=json.dumps({"output_version": {"vers_major": 1, "vers_minor": 0, "command": "zpool list"}}),
+            stderr="",
+            return_code=0,
+        ),
     )
 
     with pytest.raises(RuntimeError) as excinfo:
-        _zpool_list("zpool list invalid -pHj -o all")
+        _zpool_list("list", "invalid", "-pHj", "-o", "all")
 
     assert "Datasets are not in the correct format" in str(excinfo.value)
+
+
+# -- run_zfs, the subprocess boundary -----------------------------------------
+
+
+def completed(returncode: int = 0, stdout: str = "", stderr: str = "") -> CompletedProcess:
+    """Build a CompletedProcess the way subprocess.run would return one."""
+    return CompletedProcess(args=["zfs"], returncode=returncode, stdout=stdout, stderr=stderr)
+
+
+def test_run_zfs_passes_arguments_as_a_list(mocker: MockerFixture) -> None:
+    """Arguments must never be joined into a string and re-split."""
+    mock_run = mocker.patch(f"{COMMAND}.subprocess.run", return_value=completed(stdout="ok\n"))
+
+    run_zfs("get", "-Hp", "all", "media/temp")
+
+    mock_run.assert_called_once_with(
+        ["zfs", "get", "-Hp", "all", "media/temp"],
+        capture_output=True,
+        encoding="utf-8",
+        check=False,
+    )
+
+
+def test_run_zfs_keeps_values_containing_spaces_intact(mocker: MockerFixture) -> None:
+    """A property value with a space must reach zfs as one argument.
+
+    bash_wrapper split on whitespace, so this silently became several
+    arguments and zfs was handed something it could not parse.
+    """
+    mock_run = mocker.patch(f"{COMMAND}.subprocess.run", return_value=completed())
+
+    run_zfs("set", "mountpoint=/zfs/two words", "media/temp")
+
+    assert mock_run.call_args.args[0] == ["zfs", "set", "mountpoint=/zfs/two words", "media/temp"]
+
+
+def test_run_zfs_reports_success(mocker: MockerFixture) -> None:
+    mocker.patch(f"{COMMAND}.subprocess.run", return_value=completed(stdout="output\n", stderr=""))
+
+    result = run_zfs("list")
+
+    assert result.ok
+    assert result.stdout == "output\n"
+    assert result.return_code == 0
+    assert result.args == ("zfs", "list")
+
+
+def test_run_zfs_returns_failures_rather_than_raising(mocker: MockerFixture) -> None:
+    """Callers decide what a failure means, so run_zfs never raises."""
+    mocker.patch(f"{COMMAND}.subprocess.run", return_value=completed(returncode=1, stderr="no such pool\n"))
+
+    result = run_zfs("list")
+
+    assert not result.ok
+    assert result.stderr == "no such pool\n"
+    assert result.message == "no such pool"
+
+
+def test_run_zfs_keeps_streams_separate(mocker: MockerFixture) -> None:
+    """A warning on stderr must not contaminate stdout.
+
+    bash_wrapper returned stderr in place of stdout whenever stderr was
+    non-empty, even on success, which is how a warning could be parsed as a
+    list of dataset names.
+    """
+    mocker.patch(
+        f"{COMMAND}.subprocess.run",
+        return_value=completed(stdout="pool/one\n", stderr="warning: something\n"),
+    )
+
+    result = run_zfs("list")
+
+    assert result.ok
+    assert result.stdout == "pool/one\n"
+    assert result.stderr == "warning: something\n"
+
+
+def test_command_result_message_falls_back_to_stdout() -> None:
+    """Some zfs errors land on stdout, so the message must not be empty."""
+    result = CommandResult(args=("list",), stdout="  something went wrong \n", stderr="", return_code=1)
+
+    assert result.message == "something went wrong"
+
+
+# -- list_dataset_names -------------------------------------------------------
+
+
+def patch_run(mocker: MockerFixture, **kwargs) -> object:
+    """Patch run_zfs with a single canned result."""
+    return mocker.patch(f"{DATASET}.run_zfs", return_value=CommandResult(args=(), **kwargs))
+
+
+def test_list_dataset_names_builds_the_right_command(mocker: MockerFixture) -> None:
+    mock_run = patch_run(mocker, stdout="pool\n", stderr="", return_code=0)
+
+    list_dataset_names()
+
+    mock_run.assert_called_once_with("list", "-Hp", "-t", "filesystem", "-o", "name")
+
+
+def test_list_dataset_names_includes_pool_roots(mocker: MockerFixture) -> None:
+    """Unlike get_datasets, nothing is filtered out, so existence checks work."""
+    patch_run(mocker, stdout="media\nmedia/temp\nmedia/secure/docker\n", stderr="", return_code=0)
+
+    assert list_dataset_names() == ["media", "media/temp", "media/secure/docker"]
+
+
+def test_list_dataset_names_drops_blank_lines(mocker: MockerFixture) -> None:
+    patch_run(mocker, stdout="media\n\nmedia/temp\n\n", stderr="", return_code=0)
+
+    assert list_dataset_names() == ["media", "media/temp"]
+
+
+def test_list_dataset_names_raises_on_failure(mocker: MockerFixture) -> None:
+    """Never return a partial list: the reconciler would create everything."""
+    patch_run(mocker, stdout="", stderr="cannot open 'media': no such pool\n", return_code=1)
+
+    with pytest.raises(RuntimeError) as excinfo:
+        list_dataset_names()
+
+    assert "Failed to list ZFS datasets" in str(excinfo.value)
+    assert "no such pool" in str(excinfo.value)
+
+
+def test_list_dataset_names_never_parses_stderr_as_names(mocker: MockerFixture) -> None:
+    """The regression this guards: error text read as a dataset list."""
+    patch_run(mocker, stdout="", stderr="permission denied\n", return_code=1)
+
+    with pytest.raises(RuntimeError):
+        list_dataset_names()
+
+
+# -- get_properties -----------------------------------------------------------
+
+
+def test_get_properties_builds_the_right_command(mocker: MockerFixture) -> None:
+    mock_run = patch_run(mocker, stdout="", stderr="", return_code=0)
+
+    get_properties("media/temp")
+
+    mock_run.assert_called_once_with("get", "-Hp", "-o", "property,value,source", "all", "media/temp")
+
+
+def test_get_properties_parses_value_and_source(mocker: MockerFixture) -> None:
+    patch_run(
+        mocker,
+        stdout="compression\tzstd-9\tlocal\natime\toff\tinherited from media\nexec\ton\tdefault\n",
+        stderr="",
+        return_code=0,
+    )
+
+    assert get_properties("media/temp") == {
+        "compression": ("zstd-9", "local"),
+        "atime": ("off", "inherited from media"),
+        "exec": ("on", "default"),
+    }
+
+
+def test_get_properties_skips_blank_lines(mocker: MockerFixture) -> None:
+    """A stray blank line in the middle must not raise on unpacking."""
+    patch_run(mocker, stdout="compression\tzstd\tlocal\n\natime\toff\tlocal\n", stderr="", return_code=0)
+
+    assert get_properties("media/temp") == {
+        "compression": ("zstd", "local"),
+        "atime": ("off", "local"),
+    }
+
+
+def test_get_properties_raises_on_failure(mocker: MockerFixture) -> None:
+    patch_run(mocker, stdout="", stderr="dataset does not exist\n", return_code=1)
+
+    with pytest.raises(RuntimeError) as excinfo:
+        get_properties("media/gone")
+
+    assert "Failed to get properties for media/gone" in str(excinfo.value)
+
+
+# -- create_dataset -----------------------------------------------------------
+
+
+def test_create_dataset_builds_sorted_option_flags(mocker: MockerFixture) -> None:
+    mock_run = patch_run(mocker, stdout="", stderr="", return_code=0)
+
+    assert create_dataset("media/temp", {"sync": "disabled", "compression": "zstd-9"}) is None
+
+    mock_run.assert_called_once_with(
+        "create",
+        "-o",
+        "compression=zstd-9",
+        "-o",
+        "sync=disabled",
+        "media/temp",
+    )
+
+
+def test_create_dataset_with_no_properties(mocker: MockerFixture) -> None:
+    mock_run = patch_run(mocker, stdout="", stderr="", return_code=0)
+
+    create_dataset("media/temp", {})
+
+    mock_run.assert_called_once_with("create", "media/temp")
+
+
+def test_create_dataset_keeps_a_value_with_spaces_together(mocker: MockerFixture) -> None:
+    mock_run = patch_run(mocker, stdout="", stderr="", return_code=0)
+
+    create_dataset("media/temp", {"mountpoint": "/zfs/two words"})
+
+    assert mock_run.call_args.args == ("create", "-o", "mountpoint=/zfs/two words", "media/temp")
+
+
+def test_create_dataset_returns_the_error_on_failure(mocker: MockerFixture) -> None:
+    patch_run(mocker, stdout="", stderr="cannot create 'media/temp': out of space\n", return_code=1)
+
+    error = create_dataset("media/temp", {})
+
+    assert error is not None
+    assert "Failed to create media/temp" in error
+    assert "out of space" in error
+
+
+# -- set_property -------------------------------------------------------------
+
+
+def test_set_property_builds_the_right_command(mocker: MockerFixture) -> None:
+    mock_run = patch_run(mocker, stdout="", stderr="", return_code=0)
+
+    assert set_property("media/temp", "compression", "zstd-9") is None
+
+    mock_run.assert_called_once_with("set", "compression=zstd-9", "media/temp")
+
+
+def test_set_property_keeps_a_value_with_spaces_together(mocker: MockerFixture) -> None:
+    mock_run = patch_run(mocker, stdout="", stderr="", return_code=0)
+
+    set_property("media/temp", "mountpoint", "/zfs/two words")
+
+    assert mock_run.call_args.args == ("set", "mountpoint=/zfs/two words", "media/temp")
+
+
+def test_set_property_returns_the_error_on_failure(mocker: MockerFixture) -> None:
+    patch_run(mocker, stdout="", stderr="permission denied\n", return_code=1)
+
+    error = set_property("media/temp", "compression", "zstd-9")
+
+    assert error is not None
+    assert "Failed to set compression=zstd-9 on media/temp" in error
+    assert "permission denied" in error
+
+
+# -- run_zpool, the same wrapper the zfs side uses ----------------------------
+
+
+def test_run_zpool_prefixes_the_binary(mocker: MockerFixture) -> None:
+    mock_run = mocker.patch(f"{COMMAND}.subprocess.run", return_value=completed(stdout="{}"))
+
+    run_zpool("list", "media", "-pHj", "-o", "all")
+
+    mock_run.assert_called_once_with(
+        ["zpool", "list", "media", "-pHj", "-o", "all"],
+        capture_output=True,
+        encoding="utf-8",
+        check=False,
+    )
+
+
+def test_run_zpool_reports_failures(mocker: MockerFixture) -> None:
+    mocker.patch(f"{COMMAND}.subprocess.run", return_value=completed(returncode=1, stderr="no such pool\n"))
+
+    result = run_zpool("list", "gone")
+
+    assert not result.ok
+    assert result.message == "no such pool"
+    assert result.args == ("zpool", "list", "gone")
+
+
+def test_run_zpool_keeps_streams_separate(mocker: MockerFixture) -> None:
+    """The same contamination bug the zfs side had must not exist here."""
+    mocker.patch(
+        f"{COMMAND}.subprocess.run",
+        return_value=completed(stdout="{}", stderr="warning: pool is degraded\n"),
+    )
+
+    result = run_zpool("list")
+
+    assert result.ok
+    assert result.stdout == "{}"
+
+
+def test_run_zpool_keeps_values_containing_spaces_intact(mocker: MockerFixture) -> None:
+    mock_run = mocker.patch(f"{COMMAND}.subprocess.run", return_value=completed())
+
+    run_zpool("set", "comment=two words", "media")
+
+    assert mock_run.call_args.args[0] == ["zpool", "set", "comment=two words", "media"]
+
+
+def test_run_command_runs_what_it_is_given(mocker: MockerFixture) -> None:
+    """zfs and zpool are the same function with a different first argument."""
+    mock_run = mocker.patch(f"{COMMAND}.subprocess.run", return_value=completed())
+
+    run_command("zpool", "status")
+
+    assert mock_run.call_args.args[0] == ["zpool", "status"]
+
+
+def test_zpool_list_raises_when_the_command_fails(mocker: MockerFixture) -> None:
+    """A failed zpool must never have its error text decoded as json."""
+    mocker.patch(
+        f"{ZPOOL}.run_zpool",
+        return_value=CommandResult(args=(), stdout="", stderr="no such pool: media\n", return_code=1),
+    )
+
+    with pytest.raises(RuntimeError) as excinfo:
+        _zpool_list("list", "media", "-pHj", "-o", "all")
+
+    assert "Failed to run zpool" in str(excinfo.value)
+    assert "no such pool" in str(excinfo.value)
+
+
+def test_zpool_builds_the_right_command(mocker: MockerFixture) -> None:
+    """Zpool passes argv through rather than a formatted string."""
+    mock_run = mocker.patch(
+        f"{ZPOOL}.run_zpool",
+        return_value=CommandResult(
+            args=(),
+            stdout=json.dumps(SAMPLE_ZPOOL_DATA),
+            stderr="",
+            return_code=0,
+        ),
+    )
+
+    Zpool("testpool")
+
+    mock_run.assert_called_once_with("list", "testpool", "-pHj", "-o", "all")