Skip to content

Commit f86f013

Browse files
julienldclaude
andcommitted
refactor(tests): Address Gemini review comments in label operations tests
**Refactoring improvements:** - Extract duplicated `_find_test_entity` methods into pytest fixtures - Simplify result parsing logic: `search_data.get("data", search_data).get("results", [])` - Reduce code duplication across 5 test classes **Bug fixes:** - Fix incorrect operation count (15 → 13) in: - Test docstring - Log messages - Code comments **Impact:** - Removed ~80 lines of duplicated code - Improved maintainability with DRY principle - Fixed confusion from incorrect count in regression test Addresses Gemini Code Assist review feedback: - Comment 1: Refactored helper methods into fixtures - Comment 2: Fixed operation count discrepancy All 12 tests still pass after refactoring. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
1 parent 66187b6 commit f86f013

2 files changed

Lines changed: 58 additions & 120 deletions

File tree

tests/src/e2e/workflows/labels/test_label_operations.py

Lines changed: 56 additions & 118 deletions
Original file line numberDiff line numberDiff line change
@@ -23,32 +23,42 @@
2323
logger = logging.getLogger(__name__)
2424

2525

26+
@pytest.fixture
27+
async def test_entity_id(mcp_client) -> str:
28+
"""Find a single suitable entity for testing."""
29+
search_result = await mcp_client.call_tool(
30+
"ha_search_entities",
31+
{"query": "light", "domain_filter": "light", "limit": 1},
32+
)
33+
search_data = parse_mcp_result(search_result)
34+
results = search_data.get("data", search_data).get("results", [])
35+
if not results:
36+
pytest.skip("No light entities available for testing")
37+
return results[0]["entity_id"]
38+
39+
40+
@pytest.fixture
41+
async def test_entity_ids(mcp_client) -> list[str]:
42+
"""Find multiple suitable entities for testing."""
43+
search_result = await mcp_client.call_tool(
44+
"ha_search_entities",
45+
{"query": "light", "domain_filter": "light", "limit": 20},
46+
)
47+
search_data = parse_mcp_result(search_result)
48+
results = search_data.get("data", search_data).get("results", [])
49+
if len(results) < 3:
50+
pytest.skip("Need at least 3 light entities for bulk testing")
51+
return [r["entity_id"] for r in results[:3]]
52+
53+
2654
@pytest.mark.labels
2755
@pytest.mark.cleanup
2856
class TestLabelAddOperation:
2957
"""Test label 'add' operation (append labels, preserve existing)."""
3058

31-
async def _find_test_entity(self, mcp_client) -> str:
32-
"""Find a suitable entity for testing."""
33-
search_result = await mcp_client.call_tool(
34-
"ha_search_entities",
35-
{"query": "light", "domain_filter": "light", "limit": 10},
36-
)
37-
search_data = parse_mcp_result(search_result)
38-
39-
if "data" in search_data:
40-
results = search_data.get("data", {}).get("results", [])
41-
else:
42-
results = search_data.get("results", [])
43-
44-
if not results:
45-
pytest.skip("No light entities available for testing")
46-
47-
return results[0].get("entity_id", "")
48-
49-
async def test_add_to_empty(self, mcp_client, cleanup_tracker):
59+
async def test_add_to_empty(self, mcp_client, cleanup_tracker, test_entity_id):
5060
"""Test: Add labels to entity with no existing labels."""
51-
entity_id = await self._find_test_entity(mcp_client)
61+
entity_id = test_entity_id
5262

5363
# Create test labels
5464
create_result_1 = await mcp_client.call_tool(
@@ -90,9 +100,9 @@ async def test_add_to_empty(self, mcp_client, cleanup_tracker):
90100
assert label2_id in data.get("labels", [])
91101
logger.info("Added second label, first label preserved ✅")
92102

93-
async def test_add_preserves_existing(self, mcp_client, cleanup_tracker):
103+
async def test_add_preserves_existing(self, mcp_client, cleanup_tracker, test_entity_id):
94104
"""Test: Add operation preserves existing labels."""
95-
entity_id = await self._find_test_entity(mcp_client)
105+
entity_id = test_entity_id
96106

97107
# Create labels
98108
labels = []
@@ -122,9 +132,9 @@ async def test_add_preserves_existing(self, mcp_client, cleanup_tracker):
122132
assert all(lbl in final_labels for lbl in labels)
123133
logger.info("Add operation preserved all existing labels ✅")
124134

125-
async def test_add_duplicate_idempotent(self, mcp_client, cleanup_tracker):
135+
async def test_add_duplicate_idempotent(self, mcp_client, cleanup_tracker, test_entity_id):
126136
"""Test: Adding duplicate label is idempotent (no error, no duplicates)."""
127-
entity_id = await self._find_test_entity(mcp_client)
137+
entity_id = test_entity_id
128138

129139
# Create label
130140
result = await mcp_client.call_tool(
@@ -156,27 +166,9 @@ async def test_add_duplicate_idempotent(self, mcp_client, cleanup_tracker):
156166
class TestLabelRemoveOperation:
157167
"""Test label 'remove' operation (subtract labels, preserve remaining)."""
158168

159-
async def _find_test_entity(self, mcp_client) -> str:
160-
"""Find a suitable entity for testing."""
161-
search_result = await mcp_client.call_tool(
162-
"ha_search_entities",
163-
{"query": "light", "domain_filter": "light", "limit": 10},
164-
)
165-
search_data = parse_mcp_result(search_result)
166-
167-
if "data" in search_data:
168-
results = search_data.get("data", {}).get("results", [])
169-
else:
170-
results = search_data.get("results", [])
171-
172-
if not results:
173-
pytest.skip("No light entities available for testing")
174-
175-
return results[0].get("entity_id", "")
176-
177-
async def test_remove_only_label(self, mcp_client, cleanup_tracker):
169+
async def test_remove_only_label(self, mcp_client, cleanup_tracker, test_entity_id):
178170
"""Test: Remove the only label (clears all)."""
179-
entity_id = await self._find_test_entity(mcp_client)
171+
entity_id = test_entity_id
180172

181173
# Create label
182174
result = await mcp_client.call_tool(
@@ -201,9 +193,9 @@ async def test_remove_only_label(self, mcp_client, cleanup_tracker):
201193
assert len(data.get("labels", [])) == 0
202194
logger.info("Removed only label, entity now has no labels ✅")
203195

204-
async def test_remove_preserves_others(self, mcp_client, cleanup_tracker):
196+
async def test_remove_preserves_others(self, mcp_client, cleanup_tracker, test_entity_id):
205197
"""Test: Remove operation preserves non-specified labels."""
206-
entity_id = await self._find_test_entity(mcp_client)
198+
entity_id = test_entity_id
207199

208200
# Create 3 labels
209201
labels = []
@@ -235,9 +227,9 @@ async def test_remove_preserves_others(self, mcp_client, cleanup_tracker):
235227
assert labels[2] in final_labels
236228
logger.info("Remove operation preserved other labels ✅")
237229

238-
async def test_remove_nonexistent_safe(self, mcp_client, cleanup_tracker):
230+
async def test_remove_nonexistent_safe(self, mcp_client, cleanup_tracker, test_entity_id):
239231
"""Test: Removing non-existent label is safe (no error)."""
240-
entity_id = await self._find_test_entity(mcp_client)
232+
entity_id = test_entity_id
241233

242234
# Create 2 labels
243235
labels = []
@@ -273,27 +265,9 @@ async def test_remove_nonexistent_safe(self, mcp_client, cleanup_tracker):
273265
class TestLabelSetOperation:
274266
"""Test label 'set' operation (replace all labels)."""
275267

276-
async def _find_test_entity(self, mcp_client) -> str:
277-
"""Find a suitable entity for testing."""
278-
search_result = await mcp_client.call_tool(
279-
"ha_search_entities",
280-
{"query": "light", "domain_filter": "light", "limit": 10},
281-
)
282-
search_data = parse_mcp_result(search_result)
283-
284-
if "data" in search_data:
285-
results = search_data.get("data", {}).get("results", [])
286-
else:
287-
results = search_data.get("results", [])
288-
289-
if not results:
290-
pytest.skip("No light entities available for testing")
291-
292-
return results[0].get("entity_id", "")
293-
294-
async def test_set_replaces_all(self, mcp_client, cleanup_tracker):
268+
async def test_set_replaces_all(self, mcp_client, cleanup_tracker, test_entity_id):
295269
"""Test: Set operation replaces all existing labels."""
296-
entity_id = await self._find_test_entity(mcp_client)
270+
entity_id = test_entity_id
297271

298272
# Create labels
299273
labels = []
@@ -326,9 +300,9 @@ async def test_set_replaces_all(self, mcp_client, cleanup_tracker):
326300
assert labels[3] in final_labels
327301
logger.info("Set operation replaced all previous labels ✅")
328302

329-
async def test_set_empty_clears_all(self, mcp_client, cleanup_tracker):
303+
async def test_set_empty_clears_all(self, mcp_client, cleanup_tracker, test_entity_id):
330304
"""Test: Set with empty list clears all labels."""
331-
entity_id = await self._find_test_entity(mcp_client)
305+
entity_id = test_entity_id
332306

333307
# Create and set labels
334308
labels = []
@@ -361,27 +335,9 @@ async def test_set_empty_clears_all(self, mcp_client, cleanup_tracker):
361335
class TestBulkOperations:
362336
"""Test bulk operations (multiple entities)."""
363337

364-
async def _find_test_entities(self, mcp_client, count: int = 3) -> list[str]:
365-
"""Find multiple suitable entities for testing."""
366-
search_result = await mcp_client.call_tool(
367-
"ha_search_entities",
368-
{"query": "light", "domain_filter": "light", "limit": 20},
369-
)
370-
search_data = parse_mcp_result(search_result)
371-
372-
if "data" in search_data:
373-
results = search_data.get("data", {}).get("results", [])
374-
else:
375-
results = search_data.get("results", [])
376-
377-
if len(results) < count:
378-
pytest.skip(f"Need at least {count} light entities for bulk testing")
379-
380-
return [result.get("entity_id", "") for result in results[:count]]
381-
382-
async def test_bulk_parallel_add(self, mcp_client, cleanup_tracker):
338+
async def test_bulk_parallel_add(self, mcp_client, cleanup_tracker, test_entity_ids):
383339
"""Test: Bulk add operation in parallel mode."""
384-
entities = await self._find_test_entities(mcp_client, 3)
340+
entities = test_entity_ids
385341

386342
# Create label
387343
result = await mcp_client.call_tool(
@@ -409,9 +365,9 @@ async def test_bulk_parallel_add(self, mcp_client, cleanup_tracker):
409365
assert data.get("failed") == 0
410366
logger.info(f"Bulk parallel add succeeded for {len(entities)} entities ✅")
411367

412-
async def test_bulk_sequential_add(self, mcp_client, cleanup_tracker):
368+
async def test_bulk_sequential_add(self, mcp_client, cleanup_tracker, test_entity_ids):
413369
"""Test: Bulk add operation in sequential mode."""
414-
entities = await self._find_test_entities(mcp_client, 3)
370+
entities = test_entity_ids
415371

416372
# Create label
417373
result = await mcp_client.call_tool(
@@ -438,9 +394,9 @@ async def test_bulk_sequential_add(self, mcp_client, cleanup_tracker):
438394
assert data.get("successful") == 3
439395
logger.info(f"Bulk sequential add succeeded for {len(entities)} entities ✅")
440396

441-
async def test_bulk_partial_failure(self, mcp_client, cleanup_tracker):
397+
async def test_bulk_partial_failure(self, mcp_client, cleanup_tracker, test_entity_ids):
442398
"""Test: Bulk operation with some invalid entities (error isolation)."""
443-
entities = await self._find_test_entities(mcp_client, 2)
399+
entities = test_entity_ids[:2].copy()
444400

445401
# Add invalid entity ID
446402
entities.append("light.nonexistent_entity_12345")
@@ -477,35 +433,17 @@ async def test_bulk_partial_failure(self, mcp_client, cleanup_tracker):
477433
class TestRegressionIssue396:
478434
"""Regression test for Issue #396: Entity registry corruption from rapid operations."""
479435

480-
async def _find_test_entity(self, mcp_client) -> str:
481-
"""Find a suitable entity for testing."""
482-
search_result = await mcp_client.call_tool(
483-
"ha_search_entities",
484-
{"query": "light", "domain_filter": "light", "limit": 10},
485-
)
486-
search_data = parse_mcp_result(search_result)
487-
488-
if "data" in search_data:
489-
results = search_data.get("data", {}).get("results", [])
490-
else:
491-
results = search_data.get("results", [])
492-
493-
if not results:
494-
pytest.skip("No light entities available for testing")
495-
496-
return results[0].get("entity_id", "")
497-
498-
async def test_rapid_operations_no_corruption(self, mcp_client, cleanup_tracker):
436+
async def test_rapid_operations_no_corruption(self, mcp_client, cleanup_tracker, test_entity_id):
499437
"""
500-
Test: 15+ rapid label operations don't corrupt entity registry.
438+
Test: 13+ rapid label operations don't corrupt entity registry.
501439
502440
Issue #396 reported that 5+ rapid label operations would corrupt
503441
the entity registry, making the label UI inaccessible.
504442
505443
This test validates that the new implementation handles rapid
506444
operations correctly without corruption.
507445
"""
508-
entity_id = await self._find_test_entity(mcp_client)
446+
entity_id = test_entity_id
509447

510448
# Create test labels
511449
labels = []
@@ -517,9 +455,9 @@ async def test_rapid_operations_no_corruption(self, mcp_client, cleanup_tracker)
517455
cleanup_tracker.track("label", label_id)
518456
labels.append(label_id)
519457

520-
logger.info("Starting rapid operation test (15 operations)...")
458+
logger.info("Starting rapid operation test (13 operations)...")
521459

522-
# Perform 15 rapid operations (add/remove/set cycle)
460+
# Perform 13 rapid operations (add/remove/set cycle)
523461
operations = []
524462

525463
# Cycle 1: Add all labels one by one
@@ -575,5 +513,5 @@ async def test_rapid_operations_no_corruption(self, mcp_client, cleanup_tracker)
575513
)
576514
assert_mcp_success(final_result, "final label modification")
577515

578-
logger.info("✅ Entity registry not corrupted after 15+ rapid operations")
516+
logger.info("✅ Entity registry not corrupted after 13+ rapid operations")
579517
logger.info("✅ Issue #396 regression test PASSED")

uv.lock

Lines changed: 2 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)