From a0721e38dd5c72b4056b142d9090e10aa2220465 Mon Sep 17 00:00:00 2001 From: Jacob Magar Date: Sun, 15 Feb 2026 23:02:32 -0500 Subject: [PATCH] fix: address PR review comments on test suite - Rename test_start_http_401_unauthorized to test_list_http_401_unauthorized to match the actual action="list" being tested (threads #19, #23) - Use consistent PrefixedID format ("a"*64+":local") in test_start_container instead of "abc123def456"*4 concatenation (thread #37) - Refactor container_actions_require_id to use @pytest.mark.parametrize so each action runs independently (thread #18) - Fix docstring claiming ToolError for test that asserts success in test_stop_mutation_returns_null (thread #26) - Fix inaccurate comment about `in` operator checking truthiness; it checks key existence (thread #25) - Add edge case tests for temperature=0, temperature=null, and logFile=null in test_storage.py (thread #31) Resolves review threads: PRRT_kwDOO6Hdxs5uvO2-, PRRT_kwDOO6Hdxs5uvOcf, PRRT_kwDOO6Hdxs5uu7zx, PRRT_kwDOO6Hdxs5uvO28, PRRT_kwDOO6Hdxs5uvOcp, PRRT_kwDOO6Hdxs5uvOcn, PRRT_kwDOO6Hdxs5uvKr3 --- tests/test_docker.py | 15 ++++++++------- tests/test_storage.py | 39 +++++++++++++++++++++++++++++++++++++++ tests/test_vm.py | 4 ++-- 3 files changed, 49 insertions(+), 9 deletions(-) diff --git a/tests/test_docker.py b/tests/test_docker.py index 35c4244..c725979 100644 --- a/tests/test_docker.py +++ b/tests/test_docker.py @@ -69,11 +69,11 @@ class TestDockerValidation: with pytest.raises(ToolError, match="destructive"): await tool_fn(action="remove", container_id="abc123") - async def test_container_actions_require_id(self, _mock_graphql: AsyncMock) -> None: + @pytest.mark.parametrize("action", ["start", "stop", "details", "logs", "pause", "unpause"]) + async def test_container_actions_require_id(self, _mock_graphql: AsyncMock, action: str) -> None: tool_fn = _make_tool() - for action in ("start", "stop", "details", "logs", "pause", "unpause"): - with pytest.raises(ToolError, match="container_id"): - await tool_fn(action=action) + with pytest.raises(ToolError, match="container_id"): + await tool_fn(action=action) async def test_network_details_requires_id(self, _mock_graphql: AsyncMock) -> None: tool_fn = _make_tool() @@ -92,18 +92,19 @@ class TestDockerActions: async def test_start_container(self, _mock_graphql: AsyncMock) -> None: # First call resolves ID, second performs start + cid = "a" * 64 + ":local" _mock_graphql.side_effect = [ { "docker": { "containers": [ - {"id": "abc123def456" * 4 + "abcd1234abcd1234:local", "names": ["plex"]} + {"id": cid, "names": ["plex"]} ] } }, { "docker": { "start": { - "id": "abc123def456" * 4 + "abcd1234abcd1234:local", + "id": cid, "state": "running", } } @@ -299,7 +300,7 @@ class TestDockerNetworkErrors: with pytest.raises(ToolError, match="Connection refused"): await tool_fn(action="list") - async def test_start_http_401_unauthorized(self, _mock_graphql: AsyncMock) -> None: + async def test_list_http_401_unauthorized(self, _mock_graphql: AsyncMock) -> None: """HTTP 401 should propagate as ToolError.""" _mock_graphql.side_effect = ToolError("HTTP error 401: Unauthorized") tool_fn = _make_tool() diff --git a/tests/test_storage.py b/tests/test_storage.py index 6c9812f..9cd7867 100644 --- a/tests/test_storage.py +++ b/tests/test_storage.py @@ -109,6 +109,45 @@ class TestStorageActions: assert result["summary"]["temperature"] == "35\u00b0C" assert "1.00 GB" in result["summary"]["size_formatted"] + async def test_disk_details_temperature_zero(self, _mock_graphql: AsyncMock) -> None: + """Temperature of 0 should display as '0\u00b0C', not 'N/A'.""" + _mock_graphql.return_value = { + "disk": { + "id": "d:1", + "device": "sda", + "name": "WD", + "serialNum": "SN1", + "size": 1073741824, + "temperature": 0, + } + } + tool_fn = _make_tool() + result = await tool_fn(action="disk_details", disk_id="d:1") + assert result["summary"]["temperature"] == "0\u00b0C" + + async def test_disk_details_temperature_null(self, _mock_graphql: AsyncMock) -> None: + """Null temperature should display as 'N/A'.""" + _mock_graphql.return_value = { + "disk": { + "id": "d:1", + "device": "sda", + "name": "WD", + "serialNum": "SN1", + "size": 1073741824, + "temperature": None, + } + } + tool_fn = _make_tool() + result = await tool_fn(action="disk_details", disk_id="d:1") + assert result["summary"]["temperature"] == "N/A" + + async def test_logs_null_log_file(self, _mock_graphql: AsyncMock) -> None: + """logFile being null should return an empty dict.""" + _mock_graphql.return_value = {"logFile": None} + tool_fn = _make_tool() + result = await tool_fn(action="logs", log_path="/var/log/syslog") + assert result == {} + async def test_disk_details_not_found(self, _mock_graphql: AsyncMock) -> None: _mock_graphql.return_value = {"disk": None} tool_fn = _make_tool() diff --git a/tests/test_vm.py b/tests/test_vm.py index a18b2f3..7680a5d 100644 --- a/tests/test_vm.py +++ b/tests/test_vm.py @@ -158,10 +158,10 @@ class TestVmMutationFailures: assert result["action"] == "start" async def test_stop_mutation_returns_null(self, _mock_graphql: AsyncMock) -> None: - """VM stop returning None in the field should raise ToolError (field not 'in' data).""" + """VM stop returning None in the field should succeed (key exists, value is None).""" _mock_graphql.return_value = {"vm": {"stop": None}} tool_fn = _make_tool() - # The check is `field in data["vm"]` — None is truthy for `in`, so it succeeds + # The check is `field in data["vm"]` — `in` checks key existence, not truthiness result = await tool_fn(action="stop", vm_id="uuid-1") assert result["success"] is None assert result["action"] == "stop"