Updates for PF6 unit tests#126
Merged
Merged
Conversation
Reviewer's GuideRefactors Playwright-based UI tests and related widgets to be more robust and PF6‑compatible, and standardizes local/CI test execution through Hatch-managed environments and scripts. Sequence diagram for PF6-compatible radio.fill interactionsequenceDiagram
actor Test
participant Radio
participant Browser
participant InputElement
Test->>Radio: fill(True)
Radio->>Radio: selected
alt already selected or False
Radio-->>Test: False
else needs selection
Radio->>Browser: element(RADIO_LOC)
Browser-->>Radio: InputElement
Radio->>InputElement: evaluate(js_checked_and_events)
InputElement->>InputElement: dispatchEvent(click)
InputElement->>InputElement: dispatchEvent(input)
InputElement->>InputElement: dispatchEvent(change)
Radio-->>Test: 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.
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.
mshriver
force-pushed
the
pf6-test-fixes-hatch
branch
from
June 3, 2026 09:27
9f0fd29 to
84fc0eb
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #126 +/- ##
==========================================
- Coverage 91.23% 90.64% -0.60%
==========================================
Files 38 38
Lines 2191 2244 +53
==========================================
+ Hits 1999 2034 +35
- Misses 192 210 +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:
|
mshriver
force-pushed
the
pf6-test-fixes-hatch
branch
from
June 3, 2026 14:05
84fc0eb to
540f821
Compare
mshriver
force-pushed
the
pf6-test-fixes-hatch
branch
from
July 21, 2026 14:00
540f821 to
872aaf6
Compare
Collaborator
Author
|
@sourcery-ai review |
Collaborator
Author
|
@copilot review |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
Chip.close, callingself.browser.element(self.close_button)is likely incorrect sinceclose_buttonis a widget rather than a locator; consider dispatching the click onself.close_button's underlying element/locator instead (similar to the previous implementation). - Several new
wait_forcalls use hard-coded timeouts and delays (e.g., 10s/0.5s); consider centralizing these values or tuning them per widget to avoid unnecessarily lengthening test runs while still preserving robustness.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `Chip.close`, calling `self.browser.element(self.close_button)` is likely incorrect since `close_button` is a widget rather than a locator; consider dispatching the click on `self.close_button`'s underlying element/locator instead (similar to the previous implementation).
- Several new `wait_for` calls use hard-coded timeouts and delays (e.g., 10s/0.5s); consider centralizing these values or tuning them per widget to avoid unnecessarily lengthening test runs while still preserving robustness.
## Individual Comments
### Comment 1
<location path="src/widgetastic_patternfly5/components/chip.py" line_range="251" />
<code_context>
def close(self):
- self.close_button.click()
+ self.browser.element(self.close_button).dispatch_event("click")
@classmethod
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `self.close_button` as the locator for `browser.element` is likely incorrect and may break closing behavior.
Previously `self.close_button.click()` invoked the click directly on the widget. Now `self.close_button` is passed to `browser.element`, which usually expects a locator/selector, not a widget, so this may fail at runtime or target the wrong element. Please either trigger the click on `self.close_button` itself (e.g. `self.close_button.dispatch_event("click")`, if available) or pass an appropriate locator for the close button into `browser.element` before dispatching the event.
</issue_to_address>
### Comment 2
<location path="src/widgetastic_patternfly5/components/forms/radio.py" line_range="57-66" />
<code_context>
+ Returns:
+ ``True`` if the state changed, ``False`` otherwise.
+ """
+ if not values or self.selected:
+ return False
+ 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):** The new `fill` implementation does not take `disabled` radios into account and may force-change their state.
Previously, `fill` delegated to `self.radio.fill`, which likely respected disabled state. This implementation unconditionally sets `checked = true` via JS whenever `values` is truthy and the radio isn’t already selected, ignoring `self.disabled`. That can change the state of a disabled radio, which is incorrect and may hide UI problems. Please add a guard for `self.disabled` (e.g., return `False` or raise) before mutating the element.
</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
force-pushed
the
pf6-test-fixes-hatch
branch
from
July 21, 2026 15:05
7d79082 to
08de899
Compare
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Claude <noreply@anthropic.com>
mshriver
force-pushed
the
pf6-test-fixes-hatch
branch
from
July 21, 2026 16:11
08de899 to
140b6f2
Compare
In Playwright, set_default_timeout() applies to ALL operations including navigation. The 5s default is too aggressive for page.goto() calls with wait_until='networkidle' against external patternfly.org sites in CI environments where network latency is higher. Use set_default_navigation_timeout(30_000) explicitly so navigation gets a 30s timeout while element interactions keep the 5s timeout. Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Claude <noreply@anthropic.com>
mshriver
force-pushed
the
pf6-test-fixes-hatch
branch
from
July 21, 2026 17:27
ad2a541 to
5d6e58c
Compare
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
Integrate Hatch-based test environments and Playwright browser setup into CI while updating widgets and tests for PatternFly 6 compatibility.
New Features:
Enhancements:
Build:
Tests: