diff --git a/vizro-core/changelog.d/20231001_000340_huong_li_nguyen_fix_test_interdependence.md b/vizro-core/changelog.d/20231001_000340_huong_li_nguyen_fix_test_interdependence.md new file mode 100644 index 000000000..645717527 --- /dev/null +++ b/vizro-core/changelog.d/20231001_000340_huong_li_nguyen_fix_test_interdependence.md @@ -0,0 +1,41 @@ + + + + + + + +### Fixed + +- Fix unit test interdependence issue due to shared dash.page_registry ([#84](https://github.com/mckinsey/vizro/pull/84)) + + diff --git a/vizro-core/docs/pages/development/authors.md b/vizro-core/docs/pages/development/authors.md index 50c8773d8..132016b74 100644 --- 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 +[axa99](https://github.com/axa99), [Jo Stichbury](https://github.com/stichbury), [Juan Luis Cano Rodríguez](https://github.com/astrojuanlu), [Denis Lebedev](https://github.com/DenisLebedevMcK), diff --git a/vizro-core/src/vizro/_constants.py b/vizro-core/src/vizro/_constants.py index 98f805293..ec145a974 100644 --- a/vizro-core/src/vizro/_constants.py +++ b/vizro-core/src/vizro/_constants.py @@ -6,4 +6,4 @@ ON_PAGE_LOAD_ACTION_PREFIX = "on_page_load_action" FILTER_ACTION_PREFIX = "filter_action" PARAMETER_ACTION_PREFIX = "parameter_action" -ACCORDION_TITLE = "SELECT PAGE" +ACCORDION_DEFAULT_TITLE = "SELECT PAGE" diff --git a/vizro-core/src/vizro/models/_navigation/_accordion.py b/vizro-core/src/vizro/models/_navigation/_accordion.py index e2b60687b..256069d4a 100644 --- a/vizro-core/src/vizro/models/_navigation/_accordion.py +++ b/vizro-core/src/vizro/models/_navigation/_accordion.py @@ -5,7 +5,7 @@ from dash import html from pydantic import validator -from vizro._constants import ACCORDION_TITLE, MODULE_PAGE_404 +from vizro._constants import ACCORDION_DEFAULT_TITLE from vizro.models import VizroBaseModel from vizro.models._models_utils import _log_call from vizro.models._navigation.navigation import _validate_pages @@ -27,11 +27,13 @@ class Accordion(VizroBaseModel): @_log_call def build(self): - if self.pages: - return self._create_accordion() - return self._create_default_accordion() + return self._create_accordion() - def _create_accordion_buttons(self, accordion_pages): + def _create_accordion_buttons(self, pages): + """Creates a button for each provided page.""" + # TODO: Better if we loop through pages from MM so the Accordion.build does not depend on dashboard build. + # However, this would require that only pages used in the Dashboard are registered in the MM. + # Note: Relative path currently deviates from page.path for first page. return [ dbc.Button( children=[page["name"]], @@ -40,33 +42,20 @@ def _create_accordion_buttons(self, accordion_pages): href=page["relative_path"], ) for page in dash.page_registry.values() - for accordion_page in accordion_pages - if accordion_page == page["module"] and page["module"] != MODULE_PAGE_404 + if page["module"] in pages ] - def _create_accordion_item(self, accordion_buttons, title=ACCORDION_TITLE): + def _create_accordion_item(self, accordion_buttons, title=ACCORDION_DEFAULT_TITLE): + """Creates an accordion item for each sub-group of pages.""" return dbc.AccordionItem( children=accordion_buttons, title=title.upper(), class_name="accordion_item", ) - def _create_default_accordion(self): - accordion_buttons = [ - dbc.Button( - children=[page["name"]], - key=page["relative_path"], - className="accordion-item-button", - href=page["relative_path"], - ) - for page in dash.page_registry.values() - if page["module"] != MODULE_PAGE_404 - ] - - accordion_items = [self._create_accordion_item(accordion_buttons=accordion_buttons)] - - # Don't create accordion navigation if there is only one page and one accordion item - if len(accordion_buttons) == len(accordion_items) == 1: + def _get_accordion_container(self, accordion_items, accordion_buttons): + # Return no container if there is only one page in the dashboard or no pages exist + if (len(accordion_buttons) == len(accordion_items) == 1) or not accordion_buttons: return None return html.Div( @@ -85,31 +74,16 @@ def _create_default_accordion(self): ) def _create_accordion(self): + """Creates a custom accordion only with user-provided pages.""" accordion_items = [] - if isinstance(self.pages, dict): - for title, accordion_pages in self.pages.items(): - accordion_buttons = self._create_accordion_buttons(accordion_pages=accordion_pages) - accordion_items.append(self._create_accordion_item(accordion_buttons=accordion_buttons, title=title)) + for page_group, page_members in self.pages.items(): + accordion_buttons = self._create_accordion_buttons(pages=page_members) + accordion_items.append( + self._create_accordion_item(accordion_buttons=accordion_buttons, title=page_group) + ) if isinstance(self.pages, list): - accordion_buttons = self._create_accordion_buttons(accordion_pages=self.pages) + accordion_buttons = self._create_accordion_buttons(pages=self.pages) accordion_items.append(self._create_accordion_item(accordion_buttons=accordion_buttons)) - - if len(accordion_buttons) == len(accordion_items) == 1: - return None - - return html.Div( - children=[ - dbc.Accordion( - id=self.id, - children=accordion_items, - class_name="accordion", - persistence=True, - persistence_type="session", - ), - html.Div(className="keyline"), - ], - className="nav_panel", - id=f"{self.id}_outer", - ) + return self._get_accordion_container(accordion_items=accordion_items, accordion_buttons=accordion_buttons) diff --git a/vizro-core/src/vizro/models/_navigation/navigation.py b/vizro-core/src/vizro/models/_navigation/navigation.py index 756fe8d9e..1bf765493 100644 --- a/vizro-core/src/vizro/models/_navigation/navigation.py +++ b/vizro-core/src/vizro/models/_navigation/navigation.py @@ -3,8 +3,10 @@ import warnings from typing import TYPE_CHECKING, Optional +import dash from pydantic import PrivateAttr, validator +from vizro._constants import MODULE_PAGE_404 from vizro.managers import model_manager from vizro.models import VizroBaseModel from vizro.models._models_utils import _log_call @@ -45,6 +47,9 @@ def _validate_pages(pages): f"Unknown page ID or page title provided to Navigation 'pages'. " f"Unknown pages: {unknown_pages}" ) + if pages is None: + return [page for page in dash.page_registry.keys() if page != MODULE_PAGE_404] + return pages diff --git a/vizro-core/tests/unit/vizro/conftest.py b/vizro-core/tests/unit/vizro/conftest.py index df58c49e2..f9a55eae5 100644 --- a/vizro-core/tests/unit/vizro/conftest.py +++ b/vizro-core/tests/unit/vizro/conftest.py @@ -1,4 +1,6 @@ """Fixtures to be shared across several tests.""" + +import dash import pytest import vizro.models as vm @@ -24,15 +26,23 @@ def standard_px_chart(gapminder): @pytest.fixture() -def two_pages(): - two_pages = [ - vm.Page(title="Page 1", components=[vm.Button(), vm.Button()]), - vm.Page(title="Page 2", components=[vm.Button(), vm.Button()]), - ] - return two_pages +def page1(): + return vm.Page(title="Page 1", components=[vm.Button(), vm.Button()]) + + +@pytest.fixture() +def page2(): + return vm.Page(title="Page 2", components=[vm.Button(), vm.Button()]) @pytest.fixture() -def dashboard(two_pages): - dashboard = vm.Dashboard(pages=two_pages) +def dashboard(page1, page2): + dashboard = vm.Dashboard(pages=[page1, page2]) return dashboard + + +@pytest.fixture() +def dashboard_build(dashboard): + yield dashboard.build() + del dash.page_registry["Page 1"] + del dash.page_registry["Page 2"] diff --git a/vizro-core/tests/unit/vizro/models/_navigation/conftest.py b/vizro-core/tests/unit/vizro/models/_navigation/conftest.py index 619eb1fa4..f0869309e 100644 --- a/vizro-core/tests/unit/vizro/models/_navigation/conftest.py +++ b/vizro-core/tests/unit/vizro/models/_navigation/conftest.py @@ -1,14 +1,10 @@ """Fixtures to be shared across several tests.""" + import dash_bootstrap_components as dbc import pytest from dash import html -from vizro import Vizro - - -@pytest.fixture -def dashboard_build(dashboard): - return Vizro().build(dashboard) +from vizro._constants import ACCORDION_DEFAULT_TITLE @pytest.fixture() @@ -40,7 +36,7 @@ def accordion_from_page_as_list(): accordion_items = [ dbc.AccordionItem( children=[*accordion_buttons], - title="SELECT PAGE", + title=ACCORDION_DEFAULT_TITLE, class_name="accordion_item", ) ] diff --git a/vizro-core/tests/unit/vizro/models/_navigation/test_accordion.py b/vizro-core/tests/unit/vizro/models/_navigation/test_accordion.py index d23506890..e5d3070d5 100644 --- a/vizro-core/tests/unit/vizro/models/_navigation/test_accordion.py +++ b/vizro-core/tests/unit/vizro/models/_navigation/test_accordion.py @@ -16,7 +16,7 @@ class TestAccordionInstantiation: def test_create_accordion_default(self): accordion = Accordion(id="accordion_id") assert accordion.id == "accordion_id" - assert accordion.pages is None + assert accordion.pages == ["Page 1", "Page 2"] def test_create_accordion_pages_as_list(self, pages_as_list): accordion = Accordion(pages=pages_as_list, id="accordion_id") @@ -64,6 +64,6 @@ def test_accordion_build_single_page_accordion(self): accordion = Accordion(pages=["Page 1"], id="single_accordion").build() assert accordion is None - def test_navigation_not_all_pages_included(self, dashboard_build): + def test_navigation_not_all_pages_included(self): with pytest.warns(UserWarning): Accordion(pages=["Page 1"]) diff --git a/vizro-core/tests/unit/vizro/models/_navigation/test_navigation.py b/vizro-core/tests/unit/vizro/models/_navigation/test_navigation.py index dce83ddbb..4c1b7f932 100644 --- a/vizro-core/tests/unit/vizro/models/_navigation/test_navigation.py +++ b/vizro-core/tests/unit/vizro/models/_navigation/test_navigation.py @@ -16,7 +16,7 @@ class TestNavigationInstantiation: def test_navigation_default(self): navigation = vm.Navigation(id="navigation") assert navigation.id == "navigation" - assert navigation.pages is None + assert navigation.pages == ["Page 1", "Page 2"] def test_navigation_pages_as_list(self, pages_as_list): navigation = vm.Navigation(pages=pages_as_list, id="navigation") @@ -28,7 +28,7 @@ def test_navigation_pages_as_dict(self, pages_as_dict): assert navigation.id == "navigation" assert navigation.pages == pages_as_dict - def test_navigation_not_all_pages_included(self, dashboard_build): + def test_navigation_not_all_pages_included(self): with pytest.warns(UserWarning): vm.Navigation(pages=["Page 1"]) diff --git a/vizro-core/tests/unit/vizro/models/test_dashboard.py b/vizro-core/tests/unit/vizro/models/test_dashboard.py index 7e8a977f6..5d46680a5 100644 --- a/vizro-core/tests/unit/vizro/models/test_dashboard.py +++ b/vizro-core/tests/unit/vizro/models/test_dashboard.py @@ -10,9 +10,7 @@ import vizro import vizro.models as vm -from vizro import Vizro from vizro.actions._action_loop._action_loop import ActionLoop -from vizro.managers import model_manager from vizro.models._dashboard import create_layout_page_404, update_theme @@ -31,7 +29,7 @@ def dashboard_container(): @pytest.fixture() -def mock_page_registry(): +def mock_page_registry(page1, page2): return OrderedDict( { "Page 1": { @@ -46,12 +44,12 @@ def mock_page_registry(): "description": "", "order": 0, "supplied_order": 0, - "supplied_layout": model_manager["Page 1"].build, + "supplied_layout": page1.build, "supplied_image": None, "image": None, "image_url": None, "redirect_from": None, - "layout": model_manager["Page 1"].build, + "layout": page1.build, "relative_path": "/", }, "Page 2": { @@ -66,12 +64,12 @@ def mock_page_registry(): "description": "", "order": 1, "supplied_order": 1, - "supplied_layout": model_manager["Page 2"].build, + "supplied_layout": page2.build, "supplied_image": None, "image": None, "image_url": None, "redirect_from": None, - "layout": model_manager["Page 2"].build, + "layout": page2.build, "relative_path": "/page-2", }, "not_found_404": { @@ -101,17 +99,17 @@ def mock_page_registry(): class TestDashboardInstantiation: """Tests model instantiation and the validators run at that time.""" - def test_create_dashboard_mandatory_only(self, two_pages): - dashboard = vm.Dashboard(pages=two_pages) + def test_create_dashboard_mandatory_only(self, page1, page2): + dashboard = vm.Dashboard(pages=[page1, page2]) assert hasattr(dashboard, "id") - assert dashboard.pages == two_pages + assert dashboard.pages == [page1, page2] assert dashboard.theme == "vizro_dark" assert dashboard.title is None - def test_create_dashboard_mandatory_and_optional(self, two_pages): - dashboard = vm.Dashboard(pages=two_pages, theme="vizro_light", title="Vizro") + def test_create_dashboard_mandatory_and_optional(self, page1, page2): + dashboard = vm.Dashboard(pages=[page1, page2], theme="vizro_light", title="Vizro") assert hasattr(dashboard, "id") - assert dashboard.pages == two_pages + assert dashboard.pages == [page1, page2] assert dashboard.theme == "vizro_light" assert dashboard.title == "Vizro" @@ -127,28 +125,32 @@ def test_field_invalid_pages_input_type(self): with pytest.raises(ValidationError, match="4 validation errors for Dashboard"): vm.Dashboard(pages=[vm.Button()]) - def test_field_invalid_theme_input_type(self, two_pages): + def test_field_invalid_theme_input_type(self, page1): with pytest.raises(ValidationError, match="unexpected value; permitted: 'vizro_dark', 'vizro_light'"): - vm.Dashboard(pages=two_pages, theme="not_existing") + vm.Dashboard(pages=[page1], theme="not_existing") class TestDashboardBuild: """Tests dashboard build method.""" - def test_dashboard_container(self, dashboard, dashboard_container): - app = Vizro().build(dashboard) - result = json.loads(json.dumps(app.dash.layout, cls=plotly.utils.PlotlyJSONEncoder)) + def test_dashboard_build(self, dashboard_container, dashboard_build): + result = json.loads(json.dumps(dashboard_build, cls=plotly.utils.PlotlyJSONEncoder)) expected = json.loads(json.dumps(dashboard_container, cls=plotly.utils.PlotlyJSONEncoder)) assert result == expected - def test_dashboard_page_registry(self, dashboard, mock_page_registry): - Vizro().build(dashboard) + def test_dashboard_page_registry(self, mock_page_registry, dashboard_build): result = dash.page_registry expected = mock_page_registry # Str conversion required as comparison of OrderedDict values result in False otherwise assert str(result.items()) == str(expected.items()) +@pytest.mark.parametrize("on, expected", [(True, "vizro_dark"), (False, "vizro_light")]) +def test_update_theme(on, expected): + result = update_theme(on) + assert result == expected + + def test_create_layout_page_404(): result = create_layout_page_404() result_image = result.children[0] @@ -157,9 +159,3 @@ def test_create_layout_page_404(): assert isinstance(result, html.Div) assert isinstance(result_image, html.Img) assert isinstance(result_div, html.Div) - - -@pytest.mark.parametrize("on, expected", [(True, "vizro_dark"), (False, "vizro_light")]) -def test_update_theme(on, expected): - result = update_theme(on) - assert result == expected diff --git a/vizro-core/tests/unit/vizro/models/test_page.py b/vizro-core/tests/unit/vizro/models/test_page.py index bbc9ef5a5..6505eff57 100644 --- a/vizro-core/tests/unit/vizro/models/test_page.py +++ b/vizro-core/tests/unit/vizro/models/test_page.py @@ -1,3 +1,4 @@ +import dash import pytest from pydantic import ValidationError @@ -98,3 +99,8 @@ def test_action_auto_generation_valid(self, standard_px_chart): # TODO: Add unit tests for private methods in page build +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)]) + vm.Dashboard(pages=[page]).build() + assert ("className='left_side'" not in str(page.build())) is True + del dash.page_registry["Single Page"]