forked from HomeLab/unraid-mcp
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
This commit is contained in:
@@ -69,9 +69,9 @@ class TestDockerValidation:
|
|||||||
with pytest.raises(ToolError, match="destructive"):
|
with pytest.raises(ToolError, match="destructive"):
|
||||||
await tool_fn(action="remove", container_id="abc123")
|
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()
|
tool_fn = _make_tool()
|
||||||
for action in ("start", "stop", "details", "logs", "pause", "unpause"):
|
|
||||||
with pytest.raises(ToolError, match="container_id"):
|
with pytest.raises(ToolError, match="container_id"):
|
||||||
await tool_fn(action=action)
|
await tool_fn(action=action)
|
||||||
|
|
||||||
@@ -92,18 +92,19 @@ class TestDockerActions:
|
|||||||
|
|
||||||
async def test_start_container(self, _mock_graphql: AsyncMock) -> None:
|
async def test_start_container(self, _mock_graphql: AsyncMock) -> None:
|
||||||
# First call resolves ID, second performs start
|
# First call resolves ID, second performs start
|
||||||
|
cid = "a" * 64 + ":local"
|
||||||
_mock_graphql.side_effect = [
|
_mock_graphql.side_effect = [
|
||||||
{
|
{
|
||||||
"docker": {
|
"docker": {
|
||||||
"containers": [
|
"containers": [
|
||||||
{"id": "abc123def456" * 4 + "abcd1234abcd1234:local", "names": ["plex"]}
|
{"id": cid, "names": ["plex"]}
|
||||||
]
|
]
|
||||||
}
|
}
|
||||||
},
|
},
|
||||||
{
|
{
|
||||||
"docker": {
|
"docker": {
|
||||||
"start": {
|
"start": {
|
||||||
"id": "abc123def456" * 4 + "abcd1234abcd1234:local",
|
"id": cid,
|
||||||
"state": "running",
|
"state": "running",
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -299,7 +300,7 @@ class TestDockerNetworkErrors:
|
|||||||
with pytest.raises(ToolError, match="Connection refused"):
|
with pytest.raises(ToolError, match="Connection refused"):
|
||||||
await tool_fn(action="list")
|
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."""
|
"""HTTP 401 should propagate as ToolError."""
|
||||||
_mock_graphql.side_effect = ToolError("HTTP error 401: Unauthorized")
|
_mock_graphql.side_effect = ToolError("HTTP error 401: Unauthorized")
|
||||||
tool_fn = _make_tool()
|
tool_fn = _make_tool()
|
||||||
|
|||||||
@@ -109,6 +109,45 @@ class TestStorageActions:
|
|||||||
assert result["summary"]["temperature"] == "35\u00b0C"
|
assert result["summary"]["temperature"] == "35\u00b0C"
|
||||||
assert "1.00 GB" in result["summary"]["size_formatted"]
|
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:
|
async def test_disk_details_not_found(self, _mock_graphql: AsyncMock) -> None:
|
||||||
_mock_graphql.return_value = {"disk": None}
|
_mock_graphql.return_value = {"disk": None}
|
||||||
tool_fn = _make_tool()
|
tool_fn = _make_tool()
|
||||||
|
|||||||
@@ -158,10 +158,10 @@ class TestVmMutationFailures:
|
|||||||
assert result["action"] == "start"
|
assert result["action"] == "start"
|
||||||
|
|
||||||
async def test_stop_mutation_returns_null(self, _mock_graphql: AsyncMock) -> None:
|
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}}
|
_mock_graphql.return_value = {"vm": {"stop": None}}
|
||||||
tool_fn = _make_tool()
|
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")
|
result = await tool_fn(action="stop", vm_id="uuid-1")
|
||||||
assert result["success"] is None
|
assert result["success"] is None
|
||||||
assert result["action"] == "stop"
|
assert result["action"] == "stop"
|
||||||
|
|||||||
Reference in New Issue
Block a user