From f187256de909c4bfbb4e30c6632c9651c9bac11c Mon Sep 17 00:00:00 2001 From: Joe Schulte Date: Wed, 28 Feb 2024 23:29:49 -0500 Subject: [PATCH 01/10] fixing accordion test and removing todo --- tests/playwright/controls.py | 42 +++++++++---------- .../components/accordion/test_accordion.py | 3 -- 2 files changed, 20 insertions(+), 25 deletions(-) diff --git a/tests/playwright/controls.py b/tests/playwright/controls.py index 9a92db482..2a143fbb8 100644 --- a/tests/playwright/controls.py +++ b/tests/playwright/controls.py @@ -13,7 +13,7 @@ from playwright.sync_api import FilePayload, FloatRect, Locator, Page, Position from playwright.sync_api import expect as playwright_expect -# Import `shiny`'s typing extentions. +# Import `shiny`'s typing extensions. # Since this is a private file, tell pyright to ignore the import # (Imports split over many import statements due to auto formatting) from shiny._typing_extensions import ( @@ -1090,31 +1090,29 @@ def expect_locator_values_in_list( loc_container_orig = loc_container # Find all items in set - for item, i in zip(arr, range(len(arr))): - # Get all elements of type - has_locator = loc_item - # Get the `n`th matching element - has_locator = has_locator.nth(i) - # Make sure that element has the correct attribute value - has_locator = has_locator.locator( - f"xpath=self::*[{_xpath_match_str(key, item)}]" - ) - - # Given the container, make sure it contains this locator - loc_container = loc_container.locator( - # Return self - "xpath=.", - has=has_locator, - ) - - # Make sure other items are not in set - # If we know all elements are contained in the container, - # and all elements all unique, then it should have a count of `len(arr)` + for i in range(len(arr)): + # Get ith element of type + loc_item.nth(i) + # # Make sure that element has the correct attribute value + # has_locator = has_locator.locator( + # f"xpath=self::*[{_xpath_match_str(key, item)}]" + # ) + # + # # Given the container, make sure it contains this locator + # loc_container = loc_container.locator( + # # Return self + # "xpath=.", + # has=has_locator, + # ) + + # Make sure other items are not in the set. + # If we know all elements are contained in the container + # and all elements are unique, then it should have a count of `len(arr)` loc_inputs = loc_container.locator(loc_item) try: playwright_expect(loc_inputs).to_have_count(len(arr), timeout=timeout) except AssertionError as e: - # Debug expections + # Debug expectations # Expecting container to exist (count = 1) playwright_expect(loc_container_orig).to_have_count(1, timeout=timeout) diff --git a/tests/playwright/shiny/components/accordion/test_accordion.py b/tests/playwright/shiny/components/accordion/test_accordion.py index f47786728..1510b9997 100644 --- a/tests/playwright/shiny/components/accordion/test_accordion.py +++ b/tests/playwright/shiny/components/accordion/test_accordion.py @@ -77,9 +77,6 @@ def test_accordion(page: Page, local_app: ShinyAppProc) -> None: "input.acc(): ('updated_section_a', 'Section C', 'Section D')" ) - # TODO-karan-future; Remove return when test is able to pass. Currently it hangs indefinitely and no notification as to why. - return - toggle_efg_button.click() acc.expect_panels( [ From 4512291dfb360ef323865396b4da28cf36d127b6 Mon Sep 17 00:00:00 2001 From: Joe Schulte Date: Sun, 3 Mar 2024 14:01:58 -0500 Subject: [PATCH 02/10] expect_locator_values_in_list change to find elements via xpath all at once --- tests/playwright/controls.py | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/tests/playwright/controls.py b/tests/playwright/controls.py index 2a143fbb8..27341ea84 100644 --- a/tests/playwright/controls.py +++ b/tests/playwright/controls.py @@ -1090,20 +1090,18 @@ def expect_locator_values_in_list( loc_container_orig = loc_container # Find all items in set - for i in range(len(arr)): - # Get ith element of type - loc_item.nth(i) - # # Make sure that element has the correct attribute value - # has_locator = has_locator.locator( - # f"xpath=self::*[{_xpath_match_str(key, item)}]" - # ) - # - # # Given the container, make sure it contains this locator - # loc_container = loc_container.locator( - # # Return self - # "xpath=.", - # has=has_locator, - # ) + xpath = f"""xpath=self::*[{ + " or ".join([ + _xpath_match_str(key, item) + for item in arr + ]) + }]""" + + loc_container = loc_container.locator( + # Return self + "xpath=.", + has=loc_item.locator(xpath), + ) # Make sure other items are not in the set. # If we know all elements are contained in the container From f8ff5ddd48c8ce68c88c88d1ab710b4425032314 Mon Sep 17 00:00:00 2001 From: Joe Schulte Date: Tue, 15 Oct 2024 09:43:29 -0400 Subject: [PATCH 03/10] workaround for accordion tests --- shiny/playwright/controller/_accordion.py | 12 ++++-------- shiny/playwright/controller/_expect.py | 11 +++++++++-- .../shiny/components/accordion/test_accordion.py | 6 ++++-- 3 files changed, 17 insertions(+), 12 deletions(-) diff --git a/shiny/playwright/controller/_accordion.py b/shiny/playwright/controller/_accordion.py index 1e541d479..9d8e38d23 100644 --- a/shiny/playwright/controller/_accordion.py +++ b/shiny/playwright/controller/_accordion.py @@ -238,10 +238,7 @@ def expect_width(self, value: StyleValue, *, timeout: Timeout = None) -> None: _expect_style_to_have_value(self.loc_container, "width", value, timeout=timeout) def expect_open( - self, - value: list[PatternOrStr], - *, - timeout: Timeout = None, + self, value: list[PatternOrStr], *, timeout: Timeout = None, **kwargs ) -> None: expect_locator_values_in_list( page=self.page, @@ -255,13 +252,11 @@ def expect_open( arr=value, key="data-value", timeout=timeout, + **kwargs, ) def expect_panels( - self, - value: list[PatternOrStr], - *, - timeout: Timeout = None, + self, value: list[PatternOrStr], *, timeout: Timeout = None, **kwargs ) -> None: """ Expects the accordion to have the specified panels. @@ -281,6 +276,7 @@ def expect_panels( arr=value, key="data-value", timeout=timeout, + **kwargs, ) def set( diff --git a/shiny/playwright/controller/_expect.py b/shiny/playwright/controller/_expect.py index 48fef0645..e64f8c942 100644 --- a/shiny/playwright/controller/_expect.py +++ b/shiny/playwright/controller/_expect.py @@ -1,5 +1,7 @@ from __future__ import annotations +from time import sleep + from playwright.sync_api import Locator, Page from playwright.sync_api import expect as playwright_expect @@ -146,6 +148,7 @@ def expect_locator_values_in_list( is_checked: bool | MISSING_TYPE = MISSING, timeout: Timeout = None, key: str = "value", + **kwargs, ) -> None: """ Expect the locator to contain the values in the list. @@ -218,9 +221,13 @@ def expect_locator_values_in_list( # and all elements all unique, then it should have a count of `len(arr)` loc_inputs = loc_container.locator(loc_item) try: - playwright_expect(loc_inputs).to_have_count(len(arr), timeout=timeout) + if kwargs.get("alt_verify"): + sleep(1) # to make up for not using to_have_count + assert loc_inputs.count() == len(arr) + else: + playwright_expect(loc_inputs).to_have_count(len(arr), timeout=timeout) except AssertionError as e: - # Debug expections + # Debug expectations # Expecting container to exist (count = 1) playwright_expect(loc_container_orig).to_have_count(1, timeout=timeout) diff --git a/tests/playwright/shiny/components/accordion/test_accordion.py b/tests/playwright/shiny/components/accordion/test_accordion.py index e3c07f1c9..ec7ec78e2 100644 --- a/tests/playwright/shiny/components/accordion/test_accordion.py +++ b/tests/playwright/shiny/components/accordion/test_accordion.py @@ -89,7 +89,8 @@ def test_accordion(page: Page, local_app: ShinyAppProc) -> None: "Section E", "Section F", "Section G", - ] + ], + alt_verify=True, ) acc.expect_open( [ @@ -99,7 +100,8 @@ def test_accordion(page: Page, local_app: ShinyAppProc) -> None: "Section E", "Section F", "Section G", - ] + ], + alt_verify=True, ) # Should be uncommented once https://github.com/rstudio/bslib/issues/565 is fixed # output_txt_verbatim.expect_value( From 9a84c638abeb06d89bbe95fe23d068516571f206 Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Tue, 15 Oct 2024 17:38:07 -0400 Subject: [PATCH 04/10] Apply suggestions from code review --- shiny/playwright/controller/_accordion.py | 9 ++++++--- shiny/playwright/controller/_expect.py | 4 +--- .../shiny/components/accordion/test_accordion.py | 2 -- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/shiny/playwright/controller/_accordion.py b/shiny/playwright/controller/_accordion.py index 9d8e38d23..3cad3f9d4 100644 --- a/shiny/playwright/controller/_accordion.py +++ b/shiny/playwright/controller/_accordion.py @@ -238,7 +238,10 @@ def expect_width(self, value: StyleValue, *, timeout: Timeout = None) -> None: _expect_style_to_have_value(self.loc_container, "width", value, timeout=timeout) def expect_open( - self, value: list[PatternOrStr], *, timeout: Timeout = None, **kwargs + self, + value: list[PatternOrStr], + *, + timeout: Timeout = None, ) -> None: expect_locator_values_in_list( page=self.page, @@ -252,7 +255,7 @@ def expect_open( arr=value, key="data-value", timeout=timeout, - **kwargs, + alt_verify=True, ) def expect_panels( @@ -276,7 +279,7 @@ def expect_panels( arr=value, key="data-value", timeout=timeout, - **kwargs, + alt_verify=True, ) def set( diff --git a/shiny/playwright/controller/_expect.py b/shiny/playwright/controller/_expect.py index e64f8c942..e64501ccc 100644 --- a/shiny/playwright/controller/_expect.py +++ b/shiny/playwright/controller/_expect.py @@ -1,7 +1,5 @@ from __future__ import annotations -from time import sleep - from playwright.sync_api import Locator, Page from playwright.sync_api import expect as playwright_expect @@ -148,7 +146,7 @@ def expect_locator_values_in_list( is_checked: bool | MISSING_TYPE = MISSING, timeout: Timeout = None, key: str = "value", - **kwargs, + alt_verify: bool = False, ) -> None: """ Expect the locator to contain the values in the list. diff --git a/tests/playwright/shiny/components/accordion/test_accordion.py b/tests/playwright/shiny/components/accordion/test_accordion.py index ec7ec78e2..f2d366c4b 100644 --- a/tests/playwright/shiny/components/accordion/test_accordion.py +++ b/tests/playwright/shiny/components/accordion/test_accordion.py @@ -90,7 +90,6 @@ def test_accordion(page: Page, local_app: ShinyAppProc) -> None: "Section F", "Section G", ], - alt_verify=True, ) acc.expect_open( [ @@ -101,7 +100,6 @@ def test_accordion(page: Page, local_app: ShinyAppProc) -> None: "Section F", "Section G", ], - alt_verify=True, ) # Should be uncommented once https://github.com/rstudio/bslib/issues/565 is fixed # output_txt_verbatim.expect_value( From d55b965316b99c971ccc387fd50cdcf7d8f1b1e1 Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Tue, 15 Oct 2024 17:38:54 -0400 Subject: [PATCH 05/10] Try using `.to_have_text()` to see if it hangs forever --- shiny/playwright/controller/_expect.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/shiny/playwright/controller/_expect.py b/shiny/playwright/controller/_expect.py index e64501ccc..f5f262624 100644 --- a/shiny/playwright/controller/_expect.py +++ b/shiny/playwright/controller/_expect.py @@ -220,8 +220,11 @@ def expect_locator_values_in_list( loc_inputs = loc_container.locator(loc_item) try: if kwargs.get("alt_verify"): - sleep(1) # to make up for not using to_have_count - assert loc_inputs.count() == len(arr) + import re + + anything = re.compile(r".*") + any_text_values = [anything for _ in range(len(arr))] + playwright_expect(loc_inputs).to_have_text(any_text_values, timeout = timeout) else: playwright_expect(loc_inputs).to_have_count(len(arr), timeout=timeout) except AssertionError as e: From ee915b3b1ed792f716ff64540364671bf9ec0a02 Mon Sep 17 00:00:00 2001 From: Joe Schulte Date: Tue, 15 Oct 2024 18:02:22 -0400 Subject: [PATCH 06/10] updating the alt_verify path --- shiny/playwright/controller/_expect.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/shiny/playwright/controller/_expect.py b/shiny/playwright/controller/_expect.py index f5f262624..dd1ece2fb 100644 --- a/shiny/playwright/controller/_expect.py +++ b/shiny/playwright/controller/_expect.py @@ -219,12 +219,12 @@ def expect_locator_values_in_list( # and all elements all unique, then it should have a count of `len(arr)` loc_inputs = loc_container.locator(loc_item) try: - if kwargs.get("alt_verify"): + if alt_verify: import re - + anything = re.compile(r".*") any_text_values = [anything for _ in range(len(arr))] - playwright_expect(loc_inputs).to_have_text(any_text_values, timeout = timeout) + playwright_expect(loc_inputs).to_have_text(any_text_values, timeout=timeout) else: playwright_expect(loc_inputs).to_have_count(len(arr), timeout=timeout) except AssertionError as e: From fb33260f2dddcde86a4d6d0333f51275a286c642 Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Wed, 16 Oct 2024 14:59:33 -0400 Subject: [PATCH 07/10] Try using multiple locator assertions when `alt_verify == True` --- shiny/playwright/controller/_expect.py | 53 +++++++++++++++----------- 1 file changed, 30 insertions(+), 23 deletions(-) diff --git a/shiny/playwright/controller/_expect.py b/shiny/playwright/controller/_expect.py index dd1ece2fb..ccecd9b50 100644 --- a/shiny/playwright/controller/_expect.py +++ b/shiny/playwright/controller/_expect.py @@ -172,6 +172,11 @@ def expect_locator_values_in_list( The timeout for the expectation. Defaults to `None`. key The key. Defaults to `"value"`. + alt_verify + Determines if multiple expectations should be performed. + Defaults to `False`, a single (and very complicated) locator is asserting. + `True` will perform multiple assersions, which have the possibility of being invalid. + Use in playwright bug situations only. """ # Make sure the locator has exactly `arr` values @@ -196,6 +201,28 @@ def expect_locator_values_in_list( return loc_container_orig = loc_container + def perform_multiple_assertions(): + # Expecting container to exist (count = 1) + playwright_expect(loc_container_orig).to_have_count(1, timeout=timeout) + + # Expecting the container to contain {len(arr)} items + playwright_expect(loc_container_orig.locator(loc_item)).to_have_count( + len(arr), timeout=timeout + ) + + for item, i in zip(arr, range(len(arr))): + # Expecting item `{i}` to be `{item}` + playwright_expect( + loc_container_orig.locator(loc_item).nth(i) + ).to_have_attribute(key, item, timeout=timeout) + return + + if alt_verify: + # Accordion has issues where the single locator assertion waits forever within playwright. + # Perform multiple assertions until playwright fixes bug. + perform_multiple_assertions() + return + # Find all items in set for item, i in zip(arr, range(len(arr))): # Get all elements of type @@ -219,30 +246,10 @@ def expect_locator_values_in_list( # and all elements all unique, then it should have a count of `len(arr)` loc_inputs = loc_container.locator(loc_item) try: - if alt_verify: - import re - - anything = re.compile(r".*") - any_text_values = [anything for _ in range(len(arr))] - playwright_expect(loc_inputs).to_have_text(any_text_values, timeout=timeout) - else: - playwright_expect(loc_inputs).to_have_count(len(arr), timeout=timeout) + playwright_expect(loc_inputs).to_have_count(len(arr), timeout=timeout) except AssertionError as e: # Debug expectations - - # Expecting container to exist (count = 1) - playwright_expect(loc_container_orig).to_have_count(1, timeout=timeout) - - # Expecting the container to contain {len(arr)} items - playwright_expect(loc_container_orig.locator(loc_item)).to_have_count( - len(arr), timeout=timeout - ) - - for item, i in zip(arr, range(len(arr))): - # Expecting item `{i}` to be `{item}` - playwright_expect( - loc_container_orig.locator(loc_item).nth(i) - ).to_have_attribute(key, item, timeout=timeout) - + perform_multiple_assertions() + # Could not find the reason why. Raising the original error. raise e From 9d6b8fd33f967b7e0be3d2f9b2b89a0d4147f9e3 Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Wed, 16 Oct 2024 15:08:41 -0400 Subject: [PATCH 08/10] Apply suggestions from code review --- shiny/playwright/controller/_expect.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/shiny/playwright/controller/_expect.py b/shiny/playwright/controller/_expect.py index ccecd9b50..e286889a7 100644 --- a/shiny/playwright/controller/_expect.py +++ b/shiny/playwright/controller/_expect.py @@ -174,8 +174,8 @@ def expect_locator_values_in_list( The key. Defaults to `"value"`. alt_verify Determines if multiple expectations should be performed. - Defaults to `False`, a single (and very complicated) locator is asserting. - `True` will perform multiple assersions, which have the possibility of being invalid. + Defaults to `False`, a single (and very complicated) locator is asserted. + `True` will perform multiple assertions, which have the possibility of being invalid. Use in playwright bug situations only. """ # Make sure the locator has exactly `arr` values @@ -219,7 +219,7 @@ def perform_multiple_assertions(): if alt_verify: # Accordion has issues where the single locator assertion waits forever within playwright. - # Perform multiple assertions until playwright fixes bug. + # Perform multiple assertions until playwright fixes bug. perform_multiple_assertions() return @@ -250,6 +250,5 @@ def perform_multiple_assertions(): except AssertionError as e: # Debug expectations perform_multiple_assertions() - # Could not find the reason why. Raising the original error. raise e From ca9269740dacfa7f347ec529842884bc48121a7a Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Wed, 16 Oct 2024 15:21:51 -0400 Subject: [PATCH 09/10] lints --- shiny/playwright/controller/_accordion.py | 5 ++++- shiny/playwright/controller/_expect.py | 1 + shiny/render/_data_frame_utils/_tbl_data.py | 17 +++++++++++++---- 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/shiny/playwright/controller/_accordion.py b/shiny/playwright/controller/_accordion.py index 3cad3f9d4..5995a28d2 100644 --- a/shiny/playwright/controller/_accordion.py +++ b/shiny/playwright/controller/_accordion.py @@ -259,7 +259,10 @@ def expect_open( ) def expect_panels( - self, value: list[PatternOrStr], *, timeout: Timeout = None, **kwargs + self, + value: list[PatternOrStr], + *, + timeout: Timeout = None, ) -> None: """ Expects the accordion to have the specified panels. diff --git a/shiny/playwright/controller/_expect.py b/shiny/playwright/controller/_expect.py index e286889a7..2e4c81082 100644 --- a/shiny/playwright/controller/_expect.py +++ b/shiny/playwright/controller/_expect.py @@ -250,5 +250,6 @@ def perform_multiple_assertions(): except AssertionError as e: # Debug expectations perform_multiple_assertions() + # Could not find the reason why. Raising the original error. raise e diff --git a/shiny/render/_data_frame_utils/_tbl_data.py b/shiny/render/_data_frame_utils/_tbl_data.py index 957331965..399891ff7 100644 --- a/shiny/render/_data_frame_utils/_tbl_data.py +++ b/shiny/render/_data_frame_utils/_tbl_data.py @@ -1,6 +1,6 @@ from __future__ import annotations -from typing import Any, List, TypedDict, cast +from typing import TYPE_CHECKING, Any, List, TypedDict, cast import narwhals.stable.v1 as nw import orjson @@ -33,6 +33,9 @@ "subset_frame", ) +if TYPE_CHECKING: + import pandas as pd + ######################################################################################## # Narwhals # @@ -81,15 +84,20 @@ def as_data_frame( except TypeError as e: try: compatible_data = compatible_to_pandas(data) - return nw.from_native(compatible_data, eager_only=True) + ret: DataFrame[pd.DataFrame] = nw.from_native( + compatible_data, eager_only=True + ) + # Cast internal data as `IntoDataFrameT` type. + # A warning has already been given to the user, so this is tolerable. + return cast(DataFrame[IntoDataFrameT], ret) except TypeError: # Couldn't convert to pandas, so raise the original error raise e def compatible_to_pandas( - data: IntoDataFrameT, -) -> IntoDataFrameT: + data: IntoDataFrame, +) -> pd.DataFrame: """ Convert data to pandas, if possible. @@ -108,6 +116,7 @@ def compatible_to_pandas( stacklevel=3, ) return data.to_pandas() + # pyright: ignore[reportReturnType] raise TypeError(f"Unsupported data type: {type(data)}") From 024089f6b681eb5eb7d1b6a529a994e1b8e432c5 Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Wed, 16 Oct 2024 15:30:34 -0400 Subject: [PATCH 10/10] Update CHANGELOG.md --- CHANGELOG.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d12eb15c2..55afe614b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -76,7 +76,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * Fixed output controller `OutputDataFrame` in `shiny.playwright.controller` to correctly assert the number of rows in `.expect_nrow()` as the total number of virtual rows, not the number of currently displaying rows. (#1719) -* Fixed issue where `@render.download` did not respect the module namespacing. (Thanks, @nsiicm0) (#1732) +* Fixed issue where `@render.download` did not respect the module namespacing. (Thanks, @nsiicm0!) (#1732) + +* Added workaround in `Accordion` in `shiny.playwright.controller` where `.expect_open()` and `.expect_panels()` would hang while resolving a playwright locator. (Thanks, @joesho112358!) (#1165) ## [1.1.0] - 2024-09-03