test: add unit tests for all code-review bug fixes
19 tests covering: - I2/I3: STRINGS dict entries (System tab label, status_loading_items) - I1/C-1/S-2: nav_gen guard logic in _finalize_list_loading (matching gen applies, stale gen discards, None bypasses guard, old zero default now correctly treated as stale) - C1: url=None initialization order in _fetch_tree_children_bg - S2: dead SITE branch absent from _append_list_items - S-1: is_breadcrumb parameter removed from _navigate_to_item_data Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Martin Tranberg
225
tests/test_review_fixes.py
Normal file
225
tests/test_review_fixes.py
Normal file
@@ -0,0 +1,225 @@
|
||||
"""
|
||||
Unit tests for the code-review bug fixes applied to sharepoint_browser.py.
|
||||
|
||||
Covers:
|
||||
C1 - url=None prevents UnboundLocalError in _fetch_tree_children_bg
|
||||
I1 - nav_gen guard in _finalize_list_loading discards stale results
|
||||
I2 - System tab label derivation from STRINGS key
|
||||
I3 - status_loading_items translation key present and formattable
|
||||
C-1 - Refresh calls (nav_gen=None default) always apply their results
|
||||
S-2 - nav_gen=None sentinel is safer than 0
|
||||
S-1 - is_breadcrumb parameter removed from _navigate_to_item_data
|
||||
S2 - Dead SITE branch removed from _append_list_items
|
||||
|
||||
All tests run without a live display; wx is imported but no widgets are
|
||||
instantiated.
|
||||
"""
|
||||
|
||||
import sys
|
||||
import os
|
||||
import inspect
|
||||
import unittest
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
|
||||
import sharepoint_browser as sb
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# I2 + I3: STRINGS dictionary
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
class TestStrings(unittest.TestCase):
|
||||
|
||||
def test_system_tab_label_da(self):
|
||||
"""I2: Danish settings_logging_group yields 'System' after split/strip."""
|
||||
group = sb.STRINGS["da"]["settings_logging_group"]
|
||||
self.assertEqual(group.split("/")[0].strip(), "System")
|
||||
|
||||
def test_system_tab_label_en(self):
|
||||
"""I2: English settings_logging_group yields 'System' after split/strip."""
|
||||
group = sb.STRINGS["en"]["settings_logging_group"]
|
||||
self.assertEqual(group.split("/")[0].strip(), "System")
|
||||
|
||||
def test_status_loading_items_present_da(self):
|
||||
"""I3: status_loading_items key exists in Danish STRINGS."""
|
||||
self.assertIn("status_loading_items", sb.STRINGS["da"])
|
||||
|
||||
def test_status_loading_items_present_en(self):
|
||||
"""I3: status_loading_items key exists in English STRINGS."""
|
||||
self.assertIn("status_loading_items", sb.STRINGS["en"])
|
||||
|
||||
def test_status_loading_items_format_da(self):
|
||||
"""I3: Danish template formats with named {count} argument."""
|
||||
result = sb.STRINGS["da"]["status_loading_items"].format(count=42)
|
||||
self.assertIn("42", result)
|
||||
|
||||
def test_status_loading_items_format_en(self):
|
||||
"""I3: English template formats with named {count} argument."""
|
||||
result = sb.STRINGS["en"]["status_loading_items"].format(count=99)
|
||||
self.assertIn("99", result)
|
||||
|
||||
def test_status_loading_items_da_uses_count_kwarg(self):
|
||||
"""I3: Danish template uses {count} placeholder (not positional)."""
|
||||
template = sb.STRINGS["da"]["status_loading_items"]
|
||||
self.assertIn("{count}", template)
|
||||
|
||||
def test_status_loading_items_en_uses_count_kwarg(self):
|
||||
"""I3: English template uses {count} placeholder (not positional)."""
|
||||
template = sb.STRINGS["en"]["status_loading_items"]
|
||||
self.assertIn("{count}", template)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# I1 + C-1 + S-2: nav_gen guard in _finalize_list_loading
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
class TestNavGenGuard(unittest.TestCase):
|
||||
"""
|
||||
_finalize_list_loading(self, items_data, nav_gen=None)
|
||||
|
||||
Guard logic:
|
||||
nav_gen is None → always apply (refresh / unconstrained calls)
|
||||
nav_gen == self._nav_gen → apply (matches current navigation)
|
||||
nav_gen != self._nav_gen → discard (stale; user navigated away)
|
||||
"""
|
||||
|
||||
def _make_app(self, current_gen: int):
|
||||
"""Minimal mock that satisfies _finalize_list_loading's needs."""
|
||||
app = MagicMock()
|
||||
app._nav_gen = current_gen
|
||||
# MagicMock is truthy by default so `if not self` passes
|
||||
return app
|
||||
|
||||
# --- nav_gen=None cases (C-1 / S-2) ---
|
||||
|
||||
def test_none_gen_applies_when_nav_gen_is_1(self):
|
||||
"""C-1/S-2: nav_gen=None applies results regardless of _nav_gen."""
|
||||
app = self._make_app(1)
|
||||
items = [{"name": "a"}]
|
||||
sb.SharePointApp._finalize_list_loading(app, items, nav_gen=None)
|
||||
self.assertEqual(app.current_items, items)
|
||||
app.apply_sorting.assert_called_once()
|
||||
|
||||
def test_none_gen_applies_when_nav_gen_is_high(self):
|
||||
"""C-1: nav_gen=None still applies when _nav_gen is large."""
|
||||
app = self._make_app(99)
|
||||
items = [{"name": "b"}]
|
||||
sb.SharePointApp._finalize_list_loading(app, items, nav_gen=None)
|
||||
self.assertEqual(app.current_items, items)
|
||||
|
||||
def test_default_gen_arg_applies(self):
|
||||
"""C-1: Omitting nav_gen entirely (default=None) always applies."""
|
||||
app = self._make_app(5)
|
||||
items = [{"name": "refresh"}]
|
||||
sb.SharePointApp._finalize_list_loading(app, items) # no nav_gen kwarg
|
||||
self.assertEqual(app.current_items, items)
|
||||
app.apply_sorting.assert_called_once()
|
||||
|
||||
# --- matching gen (I1, happy path) ---
|
||||
|
||||
def test_matching_gen_applies(self):
|
||||
"""I1: Results applied when nav_gen matches _nav_gen."""
|
||||
app = self._make_app(3)
|
||||
items = [{"name": "c"}]
|
||||
sb.SharePointApp._finalize_list_loading(app, items, nav_gen=3)
|
||||
self.assertEqual(app.current_items, items)
|
||||
app.apply_sorting.assert_called_once()
|
||||
|
||||
# --- stale gen cases (I1) ---
|
||||
|
||||
def test_stale_gen_discards_results(self):
|
||||
"""I1: Results discarded when nav_gen < _nav_gen (user navigated away)."""
|
||||
app = self._make_app(5)
|
||||
sentinel = object()
|
||||
app.current_items = sentinel
|
||||
sb.SharePointApp._finalize_list_loading(app, [{"name": "old"}], nav_gen=2)
|
||||
self.assertIs(app.current_items, sentinel,
|
||||
"current_items was overwritten by stale result")
|
||||
app.apply_sorting.assert_not_called()
|
||||
|
||||
def test_future_gen_discards_results(self):
|
||||
"""I1: Results discarded when nav_gen > _nav_gen (shouldn't happen, but safe)."""
|
||||
app = self._make_app(3)
|
||||
sentinel = object()
|
||||
app.current_items = sentinel
|
||||
sb.SharePointApp._finalize_list_loading(app, [{"name": "future"}], nav_gen=99)
|
||||
self.assertIs(app.current_items, sentinel)
|
||||
app.apply_sorting.assert_not_called()
|
||||
|
||||
def test_gen_zero_still_discarded_when_nav_gen_nonzero(self):
|
||||
"""S-2: nav_gen=0 (old broken default) is now treated as a stale gen, not a pass."""
|
||||
app = self._make_app(1)
|
||||
sentinel = object()
|
||||
app.current_items = sentinel
|
||||
sb.SharePointApp._finalize_list_loading(app, [{"name": "zero"}], nav_gen=0)
|
||||
self.assertIs(app.current_items, sentinel,
|
||||
"nav_gen=0 should be treated as stale, not as 'no constraint'")
|
||||
app.apply_sorting.assert_not_called()
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# C1: url=None initialization in _fetch_tree_children_bg
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
class TestUrlInitialization(unittest.TestCase):
|
||||
|
||||
def test_url_initialized_to_none_before_conditional(self):
|
||||
"""C1: _fetch_tree_children_bg initializes url=None before the if block."""
|
||||
source = inspect.getsource(sb.SharePointApp._fetch_tree_children_bg)
|
||||
# Find the line that sets url = None before the if data['type'] conditional
|
||||
lines = source.splitlines()
|
||||
url_none_idx = next(
|
||||
(i for i, l in enumerate(lines) if "url = None" in l), None
|
||||
)
|
||||
site_if_idx = next(
|
||||
(i for i, l in enumerate(lines)
|
||||
if 'data[\'type\'] == "SITE"' in l or 'data["type"] == "SITE"' in l),
|
||||
None
|
||||
)
|
||||
self.assertIsNotNone(url_none_idx,
|
||||
"_fetch_tree_children_bg has no 'url = None' initializer")
|
||||
self.assertIsNotNone(site_if_idx,
|
||||
"_fetch_tree_children_bg has no SITE type conditional")
|
||||
self.assertLess(url_none_idx, site_if_idx,
|
||||
"'url = None' must appear before the if data['type'] block")
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# S2: Dead SITE branch removed from _append_list_items
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
class TestDeadSiteBranch(unittest.TestCase):
|
||||
|
||||
def test_append_list_items_has_no_site_img_branch(self):
|
||||
"""S2: _append_list_items no longer contains the dead SITE image branch."""
|
||||
source = inspect.getsource(sb.SharePointApp._append_list_items)
|
||||
self.assertNotIn(
|
||||
'item[\'type\'] == "SITE"',
|
||||
source,
|
||||
"_append_list_items still contains dead SITE branch"
|
||||
)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# S-1: is_breadcrumb parameter removed from _navigate_to_item_data
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
class TestIsBreakcrumbRemoved(unittest.TestCase):
|
||||
|
||||
def test_no_is_breadcrumb_param(self):
|
||||
"""S-1: _navigate_to_item_data no longer has an is_breadcrumb parameter."""
|
||||
sig = inspect.signature(sb.SharePointApp._navigate_to_item_data)
|
||||
self.assertNotIn(
|
||||
"is_breadcrumb", sig.parameters,
|
||||
"_navigate_to_item_data still declares is_breadcrumb"
|
||||
)
|
||||
|
||||
def test_tree_item_param_still_present(self):
|
||||
"""Regression: tree_item parameter was not accidentally removed."""
|
||||
sig = inspect.signature(sb.SharePointApp._navigate_to_item_data)
|
||||
self.assertIn("tree_item", sig.parameters)
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main(verbosity=2)
|
||||
Reference in New Issue
Block a user