Updates for PF6 unit tests#126
Open
mshriver wants to merge 1 commit into
Open
Conversation
Reviewer's GuideRefactors CI to use Hatch-based test environments and Playwright management, adds Hatch test/lint environments and pytest config, and introduces several Playwright/PF6‑oriented robustness fixes across widgets and tests (menus, radios, selects, navigation, sliders, chips, and alert tests). Sequence diagram for PF6 Radio.fill interaction and event dispatchsequenceDiagram
actor Test
participant Radio
participant Browser
participant Element
Test->>Radio: fill(values)
Radio->>Radio: selected
Radio->>Browser: is_checked(RADIO_LOC)
Browser-->>Radio: checked_state
alt [values == selected]
Radio-->>Test: return False
else [values is truthy and != selected]
Radio->>Browser: element(RADIO_LOC)
Browser-->>Radio: Element
Radio->>Element: evaluate(js_set_checked_and_dispatch_events)
Radio-->>Test: return True
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
ChipCategory.close(or the relevant chip category class), switching fromself.close_button.click()toself.browser.element(CATEGORY_CLOSE)removes scoping to the widget root and may click the wrong close button when multiple chip categories are present; consider resolvingCATEGORY_CLOSErelative to the component root instead of via the global browser. - In
Select.fill, deriving a CSS selector by string-splittingCREATE_ITEM_LOCATORto extract an@idassumes a very specific XPath shape and will break if the locator changes format; it would be more robust either to use the existing locator directly with Playwright or to store the bare ID/name separately rather than parsing it from the string.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `ChipCategory.close` (or the relevant chip category class), switching from `self.close_button.click()` to `self.browser.element(CATEGORY_CLOSE)` removes scoping to the widget root and may click the wrong close button when multiple chip categories are present; consider resolving `CATEGORY_CLOSE` relative to the component root instead of via the global browser.
- In `Select.fill`, deriving a CSS selector by string-splitting `CREATE_ITEM_LOCATOR` to extract an `@id` assumes a very specific XPath shape and will break if the locator changes format; it would be more robust either to use the existing locator directly with Playwright or to store the bare ID/name separately rather than parsing it from the string.
## Individual Comments
### Comment 1
<location path="src/widgetastic_patternfly5/components/forms/radio.py" line_range="52-61" />
<code_context>
def fill(self, values):
"""Can only handle `True` to check the radio, nature of individual radio button"""
- return self.radio.fill(values)
+ if values == self.selected:
+ return False
+ if values:
+ el = self.browser.element(self.RADIO_LOC)
+ el.evaluate(
+ "e => {"
+ " const nativeSetter = Object.getOwnPropertyDescriptor("
+ " window.HTMLInputElement.prototype, 'checked'"
+ " ).set;"
+ " nativeSetter.call(e, true);"
+ " e.dispatchEvent(new Event('click', {bubbles: true}));"
+ " e.dispatchEvent(new Event('input', {bubbles: true}));"
+ " e.dispatchEvent(new Event('change', {bubbles: true}));"
+ "}"
+ )
+ return True
</code_context>
<issue_to_address>
**issue (bug_risk):** Clarify behavior when `values` is `False` for an already-checked radio.
With the current logic, calling `fill(False)` on a selected radio skips the early return, skips the `if values:` block, and still returns `True`, suggesting a change occurred when nothing actually changed.
Consider either treating `False` as a no-op that returns `False`, or explicitly disallowing `False` (e.g., by raising) and documenting that `fill` only supports `True`. Otherwise, callers may misinterpret the return value and assume the radio was updated when it was not.
</issue_to_address>
### Comment 2
<location path="src/widgetastic_patternfly5/components/menus/select.py" line_range="225-233" />
<code_context>
- self.root_browser.click(self.CREATE_ITEM_LOCATOR)
+ from wait_for import wait_for as _wait_for
+
+ _id_attr = self.CREATE_ITEM_LOCATOR.split("@id='")[1].rstrip("']")
+ create_css = "#" + _id_attr
+ page = self.browser.element(".").page
+ _wait_for(
+ lambda: page.locator(create_css).count() > 0,
+ timeout=3,
+ delay=0.2,
+ )
+ page.locator(create_css).click()
return True
else:
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid brittle string parsing of `CREATE_ITEM_LOCATOR` to derive the CSS selector.
This assumes `CREATE_ITEM_LOCATOR` is always an XPath with `@id='…'` in a specific format. Any change (quotes, extra predicates, non-XPath locator) will either raise `IndexError` or yield a wrong selector.
Instead, either:
- Use the locator directly via the browser abstraction (e.g. `self.root_browser.click(self.CREATE_ITEM_LOCATOR)` with waiting), or
- Locate the element with the existing locator and call `get_attribute('id')` to derive `#id`.
That avoids coupling this code to a specific string representation of the locator.
</issue_to_address>
### Comment 3
<location path="src/widgetastic_patternfly5/components/chip.py" line_range="249-250" />
<code_context>
def close(self):
- self.close_button.click()
+ close_el = self.browser.element(CATEGORY_CLOSE)
+ close_el.dispatch_event("click")
@classmethod
</code_context>
<issue_to_address>
**issue (bug_risk):** Use the class attribute for `CATEGORY_CLOSE` and leverage the existing `close_button` widget.
`CATEGORY_CLOSE` is not defined in this scope and should be referenced as the class attribute (e.g. `self.CATEGORY_CLOSE`), otherwise this will raise a `NameError`.
Also, the previous version used `self.close_button`, which likely wraps the correct locator and behavior. To stay consistent with the existing API and avoid regressions, consider either:
- `close_el = self.browser.element(self.CATEGORY_CLOSE)`, or
- Preferably, `self.close_button.dispatch_event("click")` to reuse the widget abstraction.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+52
to
+61
| if values == self.selected: | ||
| return False | ||
| if values: | ||
| el = self.browser.element(self.RADIO_LOC) | ||
| el.evaluate( | ||
| "e => {" | ||
| " const nativeSetter = Object.getOwnPropertyDescriptor(" | ||
| " window.HTMLInputElement.prototype, 'checked'" | ||
| " ).set;" | ||
| " nativeSetter.call(e, true);" |
There was a problem hiding this comment.
issue (bug_risk): Clarify behavior when values is False for an already-checked radio.
With the current logic, calling fill(False) on a selected radio skips the early return, skips the if values: block, and still returns True, suggesting a change occurred when nothing actually changed.
Consider either treating False as a no-op that returns False, or explicitly disallowing False (e.g., by raising) and documenting that fill only supports True. Otherwise, callers may misinterpret the return value and assume the radio was updated when it was not.
Comment on lines
+225
to
+233
| _id_attr = self.CREATE_ITEM_LOCATOR.split("@id='")[1].rstrip("']") | ||
| create_css = "#" + _id_attr | ||
| page = self.browser.element(".").page | ||
| _wait_for( | ||
| lambda: page.locator(create_css).count() > 0, | ||
| timeout=3, | ||
| delay=0.2, | ||
| ) | ||
| page.locator(create_css).click() |
There was a problem hiding this comment.
suggestion (bug_risk): Avoid brittle string parsing of CREATE_ITEM_LOCATOR to derive the CSS selector.
This assumes CREATE_ITEM_LOCATOR is always an XPath with @id='…' in a specific format. Any change (quotes, extra predicates, non-XPath locator) will either raise IndexError or yield a wrong selector.
Instead, either:
- Use the locator directly via the browser abstraction (e.g.
self.root_browser.click(self.CREATE_ITEM_LOCATOR)with waiting), or - Locate the element with the existing locator and call
get_attribute('id')to derive#id.
That avoids coupling this code to a specific string representation of the locator.
Comment on lines
+249
to
+250
| close_el = self.browser.element(CATEGORY_CLOSE) | ||
| close_el.dispatch_event("click") |
There was a problem hiding this comment.
issue (bug_risk): Use the class attribute for CATEGORY_CLOSE and leverage the existing close_button widget.
CATEGORY_CLOSE is not defined in this scope and should be referenced as the class attribute (e.g. self.CATEGORY_CLOSE), otherwise this will raise a NameError.
Also, the previous version used self.close_button, which likely wraps the correct locator and behavior. To stay consistent with the existing API and avoid regressions, consider either:
close_el = self.browser.element(self.CATEGORY_CLOSE), or- Preferably,
self.close_button.dispatch_event("click")to reuse the widget abstraction.
There was a problem hiding this comment.
Pull request overview
This pull request updates the library’s PatternFly widget interactions and the accompanying Playwright-based test suite to be more reliable across PatternFly v5 and v6, while also standardizing local/CI test execution through Hatch environments.
Changes:
- Improve widget interaction robustness (dropdown/menu selection, radio selection, slider input updates, chip-group close behavior) to better align with PF6 DOM/event behavior.
- Stabilize UI tests by scoping locators to PF demo sections and using PF5/PF6-safe locators.
- Add Hatch-managed test/lint environments and refactor GitHub Actions CI to run the PF5/PF6 matrix via Hatch scripts.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| testing/conftest.py | Sets Playwright context default timeout to improve test reliability. |
| testing/components/test_alert.py | Scopes alert selection to the demo section to avoid site-wide PF6 alerts. |
| testing/components/menus/test_dropdown_disabled.py | Updates dropdown root locator to work across PF5/PF6 markup differences. |
| src/widgetastic_patternfly5/components/slider.py | Adjusts InputSlider filling to be more event-accurate and adds a post-fill wait. |
| src/widgetastic_patternfly5/components/navigation.py | Changes navigation expansion to click the actionable element via dispatched click. |
| src/widgetastic_patternfly5/components/menus/select.py | Makes typeahead “create item” selection more Playwright-friendly with explicit waiting. |
| src/widgetastic_patternfly5/components/menus/menu.py | Improves always-open menu item lookup and adds retry loops for checkbox select/deselect. |
| src/widgetastic_patternfly5/components/menus/dropdown.py | Adds retry/wait logic to dropdown open to reduce flakiness. |
| src/widgetastic_patternfly5/components/forms/radio.py | Reworks radio selection to operate on the underlying input and dispatch DOM events. |
| src/widgetastic_patternfly5/components/chip.py | Changes category chip-group close to use an event dispatch approach. |
| pyproject.toml | Adds Playwright dev dependency; introduces Hatch envs/scripts; configures pytest defaults. |
| .github/workflows/tests.yaml | Migrates CI to Hatch-managed envs and scripts; updates PF version matrix handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
248
to
+250
| def close(self): | ||
| self.close_button.click() | ||
| close_el = self.browser.element(CATEGORY_CLOSE) | ||
| close_el.dispatch_event("click") |
Comment on lines
50
to
+54
| def fill(self, values): | ||
| """Can only handle `True` to check the radio, nature of individual radio button""" | ||
| return self.radio.fill(values) | ||
| if values == self.selected: | ||
| return False | ||
| if values: |
Comment on lines
100
to
102
| if self.text == value: | ||
| return False | ||
| el = self.browser.element(self.INPUT) |
Comment on lines
+223
to
+233
| from wait_for import wait_for as _wait_for | ||
|
|
||
| _id_attr = self.CREATE_ITEM_LOCATOR.split("@id='")[1].rstrip("']") | ||
| create_css = "#" + _id_attr | ||
| page = self.browser.element(".").page | ||
| _wait_for( | ||
| lambda: page.locator(create_css).count() > 0, | ||
| timeout=3, | ||
| delay=0.2, | ||
| ) | ||
| page.locator(create_css).click() |
mshriver
force-pushed
the
pf6-test-fixes-hatch
branch
from
9f0fd29 to
84fc0eb
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #126 +/- ##
==========================================
- Coverage 91.19% 90.55% -0.64%
==========================================
Files 38 38
Lines 2191 2234 +43
==========================================
+ Hits 1998 2023 +25
- Misses 193 211 +18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Co-authored-by: Claude <noreply@anthropic.com>
mshriver
force-pushed
the
pf6-test-fixes-hatch
branch
from
84fc0eb to
540f821
Compare
Comment on lines
50
to
+54
| def fill(self, values): | ||
| """Can only handle `True` to check the radio, nature of individual radio button""" | ||
| return self.radio.fill(values) | ||
| if values == self.selected: | ||
| return False | ||
| if values: |
Comment on lines
+124
to
+127
| try: | ||
| current_item = self.browser.element(self.SUB_ITEMS_ROOT, parent=li) | ||
| except NoSuchElementException: | ||
| raise |
Comment on lines
+224
to
+226
| _id_attr = self.CREATE_ITEM_LOCATOR.split("@id='")[1].rstrip("']") | ||
| create_css = "#" + _id_attr | ||
| page = self.browser.element(".").page |
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
Update Playwright-based testing and widget interactions to be more robust and compatible with PatternFly 5 and 6, and standardize test execution via Hatch environments.
Bug Fixes:
Enhancements:
Build:
CI:
Tests: