Skip to content

fix(playwright): Add playwright workaround within Accordion's .expect_open() and .expect_panels() #1165

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Oct 16, 2024
Merged

fix(playwright): Add playwright workaround within Accordion's .expect_open() and .expect_panels() #1165

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
f187256
fixing accordion test and removing todo
joesho112358 Feb 29, 2024
4512291
expect_locator_values_in_list change to find elements via xpath all a…
joesho112358 Mar 3, 2024
499ce37
Merge branch 'main' into accordion-test-update
joesho112358 Oct 14, 2024
f8ff5dd
workaround for accordion tests
joesho112358 Oct 15, 2024
9a84c63
Apply suggestions from code review
schloerke Oct 15, 2024
d55b965
Try using `.to_have_text()` to see if it hangs forever
schloerke Oct 15, 2024
ee915b3
updating the alt_verify path
joesho112358 Oct 15, 2024
fb33260
Try using multiple locator assertions when `alt_verify == True`
schloerke Oct 16, 2024
9d6b8fd
Apply suggestions from code review
schloerke Oct 16, 2024
ca92697
lints
schloerke Oct 16, 2024
024089f
Update CHANGELOG.md
schloerke Oct 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 20 additions & 22 deletions tests/playwright/controls.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -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,
# )
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal of expect_locator_values_in_list is to make sure that the attr values found (e.g. data-value) on matching elements are the only values found. Order matters.

Ideally, playwright would have an expect(loc).to_have_attribute(key, array_of_values) support, but they do not. 😞

I'd like to keep the expectation (below in the try/except) as a single expectation, rather than a loop of expectations (which would be MUCH easier to reason about). (If a failure occurs, we do loop over the different possibilities in an effort to debug the situation. Time does move forward while the expectation loop churns, allowing Shiny being able to update the accordion in between tests is not ideal.)

So unfortunately, I'd like to keep this locator creation code as it was. Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, I am not sure I fully understand the reason to not loop through with the built in to_have_attribute(key, value). Could you elaborate on that?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Time does move forward in the DOM when performing multiple expectations in a loop.

It is possible for Shiny to update the items in the list in question while in the middle of the for loop of expectations. So in the first have it is listA and in the back half it is listB. This makes debugging this rare situation really hard.

Also detecting items that are not in the list becomes difficult. I wouldn't know how to check for it as I wouldn't be able to guarantee that the K items found are the original set of K we're looking for.

To get around this difficulty, I use a loop to construct a very complicated locator. If this locator exists, then we can guarantee that the items in the list are the only items and their order is preserved.

Pseudo code for an expect(els_loc).to_have_attribute(attrs):

  • for each el, make sure the corresponding attr matches.
  • find all of the matching els; Verify the count is the expected count

If we can do this in a single expectation, then the expectation is playwrite-like in that it can be repeated until the expectation is satisfied.

If we can create another single expectation that achieves the same logic and doesn't block forever in the accordion test, I'm all for it!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thanks for the explanation. Will think about it!


# 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)
Expand Down
3 changes: 0 additions & 3 deletions tests/playwright/shiny/components/accordion/test_accordion.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
[
Expand Down