[playwright] Menu widgets migration#102
Merged
Merged
Conversation
Collaborator
|
@sourcery-ai review |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideMigrates menu widget tests to a unified module-level view fixture eliminating manual URL changes, refactors Widgetastic PatternFly5 components to use browser.element and is_checked(), and simplifies the test_group_dropdown component path. Class diagram for updated browser element usage in menu widgetsclassDiagram
class Browser {
+element(locator, parent)
+click(element)
}
class Dropdown {
+open()
BUTTON_LOCATOR
browser : Browser
}
class Menu {
+read()
browser : Browser
}
class Select {
+read()
browser : Browser
}
Dropdown --> Browser
Menu --> Browser
Select --> Browser
Class diagram for updated input selection logic in Menu and SelectclassDiagram
class Menu {
+read()
}
class Select {
+read()
}
class Input {
+is_checked()
}
Menu --> Input : uses is_checked()
Select --> Input : uses is_checked()
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The new
viewfixture no longer navigates to specific sub-URLs for each select variant—ensure your test harness still loads the correct “single”, “checkbox”, and “typeahead” pages or reintroduce per-fixture navigation. - Switching from
browser.wait_for_elementtobrowser.elementremoves the implicit wait—consider adding an explicit wait or assertion to avoid flakiness when clicking the dropdown button. - You changed the
TESTING_PAGE_COMPONENTfortest_group_dropdown—verify the generic path still loads the grouped items example and update it if it now points to the wrong page.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `view` fixture no longer navigates to specific sub-URLs for each select variant—ensure your test harness still loads the correct “single”, “checkbox”, and “typeahead” pages or reintroduce per-fixture navigation.
- Switching from `browser.wait_for_element` to `browser.element` removes the implicit wait—consider adding an explicit wait or assertion to avoid flakiness when clicking the dropdown button.
- You changed the `TESTING_PAGE_COMPONENT` for `test_group_dropdown`—verify the generic path still loads the grouped items example and update it if it now points to the wrong page.
## Individual Comments
### Comment 1
<location> `src/widgetastic_patternfly5/components/menus/dropdown.py:90` </location>
<code_context>
# self.browser.click(self.BUTTON_LOCATOR)
# return self.is_open
- el = self.browser.wait_for_element(self.BUTTON_LOCATOR)
+ el = self.browser.element(self.BUTTON_LOCATOR)
self.browser.click(el)
return self.is_open
</code_context>
<issue_to_address>
**issue (bug_risk):** Switching from wait_for_element to element may reduce robustness against slow-loading elements.
This change assumes BUTTON_LOCATOR is always present, which may cause failures if the UI loads slowly. Please ensure this is safe for all scenarios.
</issue_to_address>
### Comment 2
<location> `src/widgetastic_patternfly5/components/menus/select.py:160` </location>
<code_context>
selected[item] = self.browser.element(
parent=el, locator=".//input"
- ).is_selected()
+ ).is_checked()
except NoSuchElementException:
selected[item] = False
</code_context>
<issue_to_address>
**question (bug_risk):** Switching to is_checked may not be appropriate for all input types.
Ensure that all input elements in the menu are checkboxes or radio buttons before relying on is_checked, as it may not work correctly for other input types.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
mshriver
reviewed
Oct 16, 2025
mshriver
approved these changes
Oct 16, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Refactor menu widget tests to use a unified module-scoped view fixture, update widget APIs for more reliable element handling and selection state, and adjust test component paths.
Bug Fixes:
Enhancements:
Tests: