Enable _clear_state and ensure test independence (#120)

Signed-off-by: Maximilian Schulz <[email protected]> Co-authored-by: huong-li-nguyen <[email protected]> Co-authored-by: Antony Milne <[email protected]>
mckinsey · Oct 23, 2023 · f391d06 · f391d06
1 parent 840d1a4
commit f391d06
Show file tree

Hide file tree

Showing 16 changed files with 91 additions and 32 deletions.
diff --git a/.github/images/tech_logos.png b/.github/images/tech_logos.png
diff --git a/README.md b/README.md
@@ -48,7 +48,7 @@ Vizro is a toolkit for creating modular data visualization applications
 </p>
 
 <p align="center">
-<img src="https://raw.githubusercontent.com/mckinsey/vizro/main/.github/images/tech_logos.png" width="270"/>
+<img src="https://raw.githubusercontent.com/mckinsey/vizro/main/.github/images/tech_logos.png" width="300"/>
 </p>
 
 ## What is Vizro?

diff --git a/pyproject.toml b/pyproject.toml
@@ -14,7 +14,7 @@ target-version = ["py38"]
 
 [tool.codespell]
 builtin = "clear,rare,en-GB_to_en-US"
-ignore-words-list = "grey"
+ignore-words-list = "grey,ned"
 
 [tool.mypy]
 # strict checks : strict = true

diff --git a/...re/changelog.d/20231019_130629_maximilian_schulz_implement_improved_clearing.md b/...re/changelog.d/20231019_130629_maximilian_schulz_implement_improved_clearing.md
@@ -0,0 +1,41 @@
+<!--
+A new scriv changelog fragment.
+
+Uncomment the section that is right (remove the HTML comment wrapper).
+-->
+
+<!--
+### Removed
+
+- A bullet item for the Removed category.
+
+-->
+<!--
+### Added
+
+- A bullet item for the Added category.
+
+-->
+
+### Changed
+
+- Update warning for duplicated IDs in `data_manager` and `model_manager` to now recommend `Vizro._reset()` as a potential fix when working in a Jupyter notebook ([#120](https://github.com/mckinsey/vizro/pull/120))
+
+<!--
+### Deprecated
+
+- A bullet item for the Deprecated category.
+
+-->
+<!--
+### Fixed
+
+- A bullet item for the Fixed category.
+
+
+<!--
+### Security
+
+- A bullet item for the Security category.
+
+-->
diff --git a/vizro-core/docs/pages/development/authors.md b/vizro-core/docs/pages/development/authors.md
@@ -14,6 +14,7 @@
 
 ## Previous team members and code contributors
 
+[Ned Letcher](https://github.com/ned2),
 Natalia Kurakina,
 [Leon Nallamuthu](https://github.com/leonnallamuthu),
 [axa99](https://github.com/axa99),

diff --git a/vizro-core/src/vizro/_vizro.py b/vizro-core/src/vizro/_vizro.py
@@ -3,8 +3,8 @@
 from pathlib import Path
 from typing import Dict, List, Tuple
 
+import dash
 import flask
-from dash import Dash
 
 from vizro._constants import STATIC_URL_PREFIX
 from vizro.managers import data_manager, model_manager
@@ -22,7 +22,7 @@ class Vizro:
     def __init__(self):
         """Initializes Dash."""
         _js, _css = _append_styles(self._lib_assets_folder, STATIC_URL_PREFIX)
-        self.dash = Dash(
+        self.dash = dash.Dash(
             use_pages=True,
             pages_folder="",
             external_scripts=_js,
@@ -76,6 +76,18 @@ def _pre_build():
             if hasattr(model, "pre_build"):
                 model.pre_build()
 
+    @staticmethod
+    def _reset():
+        """Private method that clears all state in the vizro app."""
+        data_manager._clear()
+        model_manager._clear()
+        dash._callback.GLOBAL_CALLBACK_LIST = []
+        dash._callback.GLOBAL_CALLBACK_MAP = {}
+        dash._callback.GLOBAL_INLINE_SCRIPTS = []
+        dash.page_registry.clear()
+        dash._pages.CONFIG.clear()
+        dash._pages.CONFIG.__dict__.clear()
+
 
 def _append_styles(walk_dir: str, url_prefix: str) -> Tuple[List[Dict[str, str]], List[str]]:
     """Append vizro css and js resources."""

diff --git a/vizro-core/src/vizro/managers/_data_manager.py b/vizro-core/src/vizro/managers/_data_manager.py
@@ -56,7 +56,7 @@ def _add_component(self, component_id: ComponentID, dataset_name: DatasetName):
                 f"Component with id={component_id} already exists and is mapped to dataset "
                 f"{self.__component_to_original[component_id]}. Components must uniquely map to a dataset across the "
                 f"whole dashboard. If you are working from a Jupyter Notebook, please either restart the kernel, or "
-                f"use 'from vizro.managers import data_manager; data_manager._reset()`."
+                f"use 'from vizro import Vizro; Vizro._reset()`."
             )
         self.__component_to_original[component_id] = dataset_name
 
@@ -81,7 +81,7 @@ def _has_registered_data(self, component_id: ComponentID) -> bool:
         except KeyError:
             return False
 
-    def _reset(self):
+    def _clear(self):
         self.__init__()  # type: ignore[misc]
 
 

diff --git a/vizro-core/src/vizro/managers/_model_manager.py b/vizro-core/src/vizro/managers/_model_manager.py
@@ -33,7 +33,7 @@ def __setitem__(self, model_id: ModelID, model: Model):
             raise DuplicateIDError(
                 f"Model with id={model_id} already exists. Models must have a unique id across the whole dashboard. "
                 f"If you are working from a Jupyter Notebook, please either restart the kernel, or "
-                f"use 'from vizro.managers import model_manager; model_manager._reset()`."
+                f"use 'from vizro import Vizro; Vizro._reset()`."
             )
         self.__models[model_id] = model
 
@@ -58,7 +58,7 @@ def _items_with_type(self, model_type: Type[Model]) -> Generator[Tuple[ModelID,
     def _generate_id() -> ModelID:
         return ModelID(str(uuid.UUID(int=rd.getrandbits(128))))
 
-    def _reset(self):
+    def _clear(self):
         self.__init__()  # type: ignore[misc]
 
 

diff --git a/vizro-core/tests/conftest.py b/vizro-core/tests/conftest.py
@@ -1,10 +1,19 @@
 import pytest
 
-from vizro.managers import data_manager, model_manager
+from vizro import Vizro
 
 
 @pytest.fixture(autouse=True)
 def reset_managers():
+    # this ensures that the managers are reset before and after each test
+    # the reset BEFORE all tests is important because at pytest test collection, fixtures are evaluated and hence
+    # the model_manager may be populated with models from other tests
+    Vizro._reset()
     yield
-    model_manager._reset()
-    data_manager._reset()
+    Vizro._reset()
+
+
+@pytest.fixture()
+def vizro_app():
+    """Fixture to instantiate Vizro/Dash app. Required when needing to register pages."""
+    yield Vizro()
diff --git a/vizro-core/tests/unit/vizro/actions/_action_loop/test_get_action_loop_components.py b/vizro-core/tests/unit/vizro/actions/_action_loop/test_get_action_loop_components.py
@@ -2,7 +2,6 @@
 
 import json
 
-import dash
 import plotly
 import pytest
 from dash import dcc, html
@@ -103,8 +102,7 @@ def managers_one_page_two_components_two_controls():
         ]
     )
 
-    yield Vizro._pre_build()
-    del dash.page_registry["test_page"]
+    Vizro._pre_build()
 
 
 @pytest.fixture
@@ -120,14 +118,13 @@ def managers_one_page_no_actions():
         ]
     )
 
-    yield Vizro._pre_build()
-    del dash.page_registry["test_page_no_actions"]
+    Vizro._pre_build()
 
 
 class TestGetActionLoopComponents:
     """Tests getting required components for the action loop."""
 
-    @pytest.mark.usefixtures("managers_one_page_no_actions")
+    @pytest.mark.usefixtures("vizro_app", "managers_one_page_no_actions")
     def test_no_components(self):
         result = _get_action_loop_components()
         result = json.loads(json.dumps(result, cls=plotly.utils.PlotlyJSONEncoder))
@@ -136,7 +133,7 @@ def test_no_components(self):
 
         assert result == expected
 
-    @pytest.mark.usefixtures("managers_one_page_two_components_two_controls")
+    @pytest.mark.usefixtures("vizro_app", "managers_one_page_two_components_two_controls")
     @pytest.mark.parametrize(
         "gateway_components, "
         "action_trigger_components, "

diff --git a/vizro-core/tests/unit/vizro/conftest.py b/vizro-core/tests/unit/vizro/conftest.py
@@ -1,6 +1,5 @@
 """Fixtures to be shared across several tests."""
 
-import dash
 import pytest
 
 import vizro.models as vm
@@ -43,6 +42,4 @@ def dashboard(page1, page2):
 
 @pytest.fixture()
 def dashboard_prebuild(dashboard):
-    yield dashboard.pre_build()
-    del dash.page_registry["Page 1"]
-    del dash.page_registry["Page 2"]
+    dashboard.pre_build()
diff --git a/vizro-core/tests/unit/vizro/models/_navigation/test_accordion.py b/vizro-core/tests/unit/vizro/models/_navigation/test_accordion.py
@@ -10,7 +10,7 @@
 from vizro.models._navigation._accordion import Accordion
 
 
-@pytest.mark.usefixtures("dashboard_prebuild")
+@pytest.mark.usefixtures("vizro_app", "dashboard_prebuild")
 class TestAccordionInstantiation:
     """Tests accordion model instantiation."""
 
@@ -43,7 +43,7 @@ def test_invalid_field_pages_wrong_input_type(self):
             Accordion(pages=[vm.Page(title="Page 3", components=[vm.Button()])])
 
 
-@pytest.mark.usefixtures("dashboard_prebuild")
+@pytest.mark.usefixtures("vizro_app", "dashboard_prebuild")
 class TestAccordionBuild:
     """Tests accordion build method."""
 

diff --git a/vizro-core/tests/unit/vizro/models/_navigation/test_navigation.py b/vizro-core/tests/unit/vizro/models/_navigation/test_navigation.py
@@ -10,7 +10,7 @@
 from vizro.models._navigation._accordion import Accordion
 
 
-@pytest.mark.usefixtures("dashboard_prebuild")
+@pytest.mark.usefixtures("vizro_app", "dashboard_prebuild")
 class TestNavigationInstantiation:
     """Tests navigation model instantiation ."""
 
@@ -45,7 +45,7 @@ def test_navigation_invalid_pages_unknown_page(self):
             vm.Navigation(pages=["Test"], id="navigation")
 
 
-@pytest.mark.usefixtures("dashboard_prebuild")
+@pytest.mark.usefixtures("vizro_app", "dashboard_prebuild")
 @pytest.mark.parametrize("pages", [["Page 1", "Page 2"], {"SELECT PAGE": ["Page 1", "Page 2"]}])
 def test_navigation_pre_build(pages):
     navigation = vm.Navigation(pages=pages, id="navigation")
@@ -57,7 +57,7 @@ def test_navigation_pre_build(pages):
     assert navigation._selector.pages == {"SELECT PAGE": ["Page 1", "Page 2"]}
 
 
-@pytest.mark.usefixtures("dashboard_prebuild")
+@pytest.mark.usefixtures("vizro_app", "dashboard_prebuild")
 @pytest.mark.parametrize("pages", [["Page 1", "Page 2"], {"Page 1": ["Page 1"], "Page 2": ["Page 2"]}])
 def test_navigation_build(pages):
     navigation = vm.Navigation(pages=pages)

diff --git a/vizro-core/tests/unit/vizro/models/test_dashboard.py b/vizro-core/tests/unit/vizro/models/test_dashboard.py
@@ -133,7 +133,9 @@ def test_field_invalid_theme_input_type(self, page1):
 class TestDashboardPreBuild:
     """Tests dashboard pre_build method."""
 
-    def test_dashboard_page_registry(self, mock_page_registry, dashboard_prebuild):
+    @pytest.mark.usefixtures("vizro_app")
+    def test_dashboard_page_registry(self, dashboard, mock_page_registry):
+        dashboard.pre_build()
         result = dash.page_registry
         expected = mock_page_registry
         # Str conversion required as comparison of OrderedDict values result in False otherwise
@@ -149,6 +151,7 @@ def test_create_layout_page_404(self):
         assert isinstance(result_div, html.Div)
 
 
+@pytest.mark.usefixtures("vizro_app")
 class TestDashboardBuild:
     """Tests dashboard build method."""
 

diff --git a/vizro-core/tests/unit/vizro/models/test_page.py b/vizro-core/tests/unit/vizro/models/test_page.py
@@ -1,6 +1,5 @@
 import re
 
-import dash
 import pandas as pd
 import pytest
 from pydantic import ValidationError
@@ -144,10 +143,10 @@ def test_action_auto_generation_valid(self, standard_px_chart):
 
 
 # TODO: Add unit tests for private methods in page build
+@pytest.mark.usefixtures("vizro_app")
 def test_page_build_left_side_removed(standard_px_chart):
     page = vm.Page(title="Single Page", components=[vm.Graph(id="scatter_chart", figure=standard_px_chart)])
     dashboard = vm.Dashboard(pages=[page])
     dashboard.pre_build()
     dashboard.navigation.pre_build()
     assert "className='left_side'" not in str(page.build())
-    del dash.page_registry["Single Page"]
diff --git a/vizro-core/tests/unit/vizro/themes/test_theme.py b/vizro-core/tests/unit/vizro/themes/test_theme.py
@@ -12,7 +12,7 @@
 @pytest.fixture
 def template():
     """Fixture for template."""
-    yield np.random.choice([themes.dark, themes.light], 1)[0]
+    return np.random.choice([themes.dark, themes.light], 1)[0]
 
 
 @pytest.fixture
@@ -25,7 +25,7 @@ def chart_data():
             "y": np.random.normal(0, 200, data_points),
         }
     )
-    yield data
+    return data
 
 
 def test_template(template):