diff --git a/tests/test_review_fixes.py b/tests/test_review_fixes.py new file mode 100644 index 0000000..96a38de --- /dev/null +++ b/tests/test_review_fixes.py @@ -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)