Playwright test dependencies OS install#131
Merged
Merged
Conversation
Reviewer's GuideThis PR migrates Playwright test setup and execution to Hatch-managed environments, adds OS-aware Playwright dependency installation, and makes multiple widget locators and interactions robust across PatternFly 5/6 and different browsers. Sequence diagram for robust Dropdown.open interaction with wait_forsequenceDiagram
actor Tester
participant Dropdown
participant browser
participant wait_for as _wait_for
Tester->>Dropdown: open()
Dropdown->>wait_for: _wait_for(_click, timeout=10, delay=0.5)
loop until is_open or timeout
wait_for->>Dropdown: _click()
Dropdown->>browser: click(BUTTON_LOCATOR)
Dropdown->>browser: is_open
browser-->>Dropdown: is_open
Dropdown-->>wait_for: is_open
end
Dropdown-->>Tester: is_open
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 1 security issue, 3 other issues, and left some high level feedback:
Security issues:
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
General comments:
- In
Chip.close,self.browser.element(self.close_button).dispatch_event("click")assumesself.close_buttonis a locator rather than a widget; ifclose_buttonis a widget, you likely want to dispatch the event on its underlying element/locator or keep using its.click()method to avoid type/lookup issues. - The new
Radio.fillimplementation uses a hard-coded JS snippet withevaluateand synthetic events; consider centralizing this pattern or wrapping it in a helper to avoid duplication and make it easier to adjust if Playwright’s evaluation/interaction semantics change.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `Chip.close`, `self.browser.element(self.close_button).dispatch_event("click")` assumes `self.close_button` is a locator rather than a widget; if `close_button` is a widget, you likely want to dispatch the event on its underlying element/locator or keep using its `.click()` method to avoid type/lookup issues.
- The new `Radio.fill` implementation uses a hard-coded JS snippet with `evaluate` and synthetic events; consider centralizing this pattern or wrapping it in a helper to avoid duplication and make it easier to adjust if Playwright’s evaluation/interaction semantics change.
## Individual Comments
### Comment 1
<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):** Radio.fill should probably respect disabled state before changing selection.
This implementation bypasses `self.radio.fill` and directly manipulates the DOM without checking `self.disabled` or `el.is_enabled()`. As a result, disabled radios can still be programmatically activated, which is inconsistent with other widgets and typical `fill` semantics. Please short‑circuit when the control is disabled and return `False` instead of dispatching events in that case.
</issue_to_address>
### Comment 2
<location path="src/widgetastic_patternfly5/components/menus/dropdown.py" line_range="169-178" />
<code_context>
- # input element don't have such disabled attributes it at level of session.
is_el_enabled = el.is_enabled()
else:
+ el_classes = self.browser.classes(el)
aria_disabled = str(self.browser.get_attribute("aria-disabled", el)).lower()
+ # PF6 puts aria-disabled on a child element (button/a) rather than the li
+ if aria_disabled != "true":
+ child_els = self.browser.elements(".//*[@aria-disabled='true']", parent=el)
+ if child_els:
+ aria_disabled = "true"
is_el_enabled = (
- "pf-m-disabled" not in self.browser.classes(el) and aria_disabled != "true"
+ "pf-m-disabled" not in el_classes
+ and "pf-m-aria-disabled" not in el_classes
+ and aria_disabled != "true"
)
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Checkbox handling in item_enabled ignores aria-disabled and modifier classes.
In the `type == 'checkbox'` branch, we only use `el.is_enabled()` and skip the ARIA / `pf-m-disabled` / `pf-m-aria-disabled` checks. Since PatternFly can represent disabled state via ARIA or CSS rather than the native `disabled` attribute, a checkbox that is visually/semantically disabled may still be treated as enabled. Please apply the same ARIA/class-based logic to checkboxes (or factor it out for reuse) so disabled checkboxes are not reported as enabled.
Suggested implementation:
```python
_wait_for(_click, timeout=10, delay=0.5)
return self.is_open
def close(self, ignore_nonpresent=False):
el = self.item_element(item, close=False, **kwargs)
if self.browser.get_attribute("type", el) == "checkbox":
el_classes = self.browser.classes(el)
aria_disabled = str(self.browser.get_attribute("aria-disabled", el)).lower()
# PF6 puts aria-disabled on a child element (button/a) rather than the li
if aria_disabled != "true":
child_els = self.browser.elements(".//*[@aria-disabled='true']", parent=el)
if child_els:
aria_disabled = "true"
is_el_enabled = (
el.is_enabled()
and "pf-m-disabled" not in el_classes
and "pf-m-aria-disabled" not in el_classes
and aria_disabled != "true"
)
else:
from wait_for import wait_for as _wait_for
from widgetastic.exceptions import NoSuchElementException
from widgetastic.utils import ParametrizedLocator
from widgetastic.widget import Widget
if self.is_open:
return
```
You may want to:
1. Factor the ARIA/class-based enabled check into a small helper (e.g. `_is_item_enabled(el)`) and reuse it in both the checkbox and non-checkbox branches to avoid duplication.
2. Ensure that in the non-checkbox branch, the same `el_classes` / `aria_disabled` logic is being used consistently (if the code there diverged or changed elsewhere in the file, align it with this implementation).
</issue_to_address>
### Comment 3
<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 close_button directly as a locator for browser.element may be fragile.
The previous `self.close_button.click()` relied on the widget’s own click behavior. Now `self.close_button` is passed to `browser.element`, which usually expects a locator/XPath, not a widget instance. If `close_button` isn’t guaranteed to be a valid locator or resolvable by `browser.element`, this could fail at runtime. Consider dispatching the event on the underlying element (e.g. `self.close_button._widget` / `self.close_button.element`) or using the locator that `close_button` wraps instead.
</issue_to_address>
### Comment 4
<location path="scripts/install_playwright_deps.py" line_range="47" />
<code_context>
subprocess.run(cmd, check=True)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</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
pw-tests-os-install
branch
2 times, most recently
from
July 21, 2026 14:54
633f413 to
aa3e962
Compare
the playwright dependency installer only handles apt-get and explicitly will not be handling any other operating systems provide a script, run through hatch, to handle playwright dependency install Co-authored-by: Claude <noreply@anthropic.com>
mshriver
force-pushed
the
pw-tests-os-install
branch
from
July 21, 2026 15:05
1d6890a to
9bb26bd
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
1 participant
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
Introduce Hatch-based Playwright test environments and improve PatternFly widget/browser compatibility and reliability across components and tests.
New Features:
Enhancements:
Build:
Tests:
Chores: