Some playwright methods#303
Merged
digitronik merged 1 commit intoRedHatQE:mainfrom May 3, 2026
Merged
Conversation
Reviewer's GuideAdds higher-level Playwright helpers for screenshots, page content, cookies, keyboard input, and network interception, plus corresponding tests and deprecates the old save_screenshot API in favor of a unified screenshot method. Sequence diagram for Browser.screenshot wrappersequenceDiagram
actor TestCode
participant Browser
participant Page
TestCode->>Browser: screenshot(path, full_page, type, quality, animations, scale, kwargs)
Browser->>Browser: logger.debug(...)
Browser->>Browser: build opts dict
alt path is not None
Browser->>Browser: opts["path"] = path
end
alt quality is not None
Browser->>Browser: opts["quality"] = quality
end
Browser->>Page: screenshot(**opts)
Page-->>Browser: image_bytes
Browser-->>TestCode: image_bytes
Sequence diagram for Browser.expect_request context usagesequenceDiagram
actor TestCode
participant Browser
participant Page
participant ContextManager as ExpectRequestContext
TestCode->>Browser: expect_request(url_or_predicate, timeout)
Browser->>Browser: logger.debug(...)
Browser->>Page: expect_request(url_or_predicate, timeout)
Page-->>Browser: ExpectRequestContext
Browser-->>TestCode: ExpectRequestContext
rect rgb(230,230,230)
TestCode->>ContextManager: __enter__()
Note over TestCode,ContextManager: Test action inside with block
TestCode->>Page: trigger UI action that makes network request
Page-->>ContextManager: matching Request captured
TestCode->>ContextManager: __exit__()
end
ContextManager-->>TestCode: value (Request)
Class diagram for new Playwright helper methods on BrowserclassDiagram
class Browser {
+logger
+page
+url
+screenshot(path, full_page, type, quality, animations, scale, kwargs) bytes
+save_screenshot(filename) void
+page_content str
+clear_cookies() void
+add_cookie(cookie) void
+get_cookies(urls) List_Dict
+press_key(key, locator, args, kwargs) void
+expect_request(url_or_predicate, timeout) ExpectRequestContext
+expect_response(url_or_predicate, timeout) ExpectResponseContext
}
class Page {
+context Context
+keyboard Keyboard
+screenshot(opts) bytes
+content() str
+expect_request(url_or_predicate, timeout) ExpectRequestContext
+expect_response(url_or_predicate, timeout) ExpectResponseContext
}
class Context {
+browser BrowserEngine
+clear_cookies() void
+add_cookies(cookies) void
+cookies(urls) List_Dict
}
class Keyboard {
+press(key) void
}
class ExpectRequestContext {
+value Request
}
class ExpectResponseContext {
+value Response
}
class Request {
+url str
}
class Response {
+url str
+status int
+ok bool
}
class BrowserEngine {
+version str
}
Browser *-- Page
Page *-- Context
Page *-- Keyboard
Context o-- BrowserEngine
ExpectRequestContext o-- Request
ExpectResponseContext o-- Response
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 2 issues, and left some high level feedback:
- Consider renaming the
typeparameter inscreenshotto something likeimage_typeorformatto avoid shadowing the built-intypeand make its purpose clearer. - The
clear_cookiesmethod logs at INFO level on every call, which could be noisy in test-heavy or high-traffic usage; consider downgrading this to DEBUG unless cookie clearing is an exceptional operation in your context. - In
test_screenshot_saves_to_file, usingtempfile._get_default_tempdir()relies on a private API; prefertempfile.gettempdir()ortempfile.NamedTemporaryFileto avoid depending on an internal function.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider renaming the `type` parameter in `screenshot` to something like `image_type` or `format` to avoid shadowing the built-in `type` and make its purpose clearer.
- The `clear_cookies` method logs at INFO level on every call, which could be noisy in test-heavy or high-traffic usage; consider downgrading this to DEBUG unless cookie clearing is an exceptional operation in your context.
- In `test_screenshot_saves_to_file`, using `tempfile._get_default_tempdir()` relies on a private API; prefer `tempfile.gettempdir()` or `tempfile.NamedTemporaryFile` to avoid depending on an internal function.
## Individual Comments
### Comment 1
<location path="src/widgetastic/browser.py" line_range="434-443" />
<code_context>
+ def add_cookie(self, cookie: Dict[str, Any]) -> None:
</code_context>
<issue_to_address>
**issue:** Align the documented requirement for `url`/`domain` with the actual behavior that auto-fills `url`.
The docstring states that either `url` or `domain` is required and that the current page URL is used only if neither is provided, but the implementation always auto-fills `url` from `self.url` when both are missing. Consider either enforcing the documented requirement by raising in that case, or updating the docstring to clarify that `url`/`domain` are optional and default to the current page. The current mismatch makes the method’s contract misleading.
</issue_to_address>
### Comment 2
<location path="src/widgetastic/browser.py" line_range="1321-1330" />
<code_context>
+ def press_key(self, key: str, locator: LocatorAlias = None, *args, **kwargs) -> None:
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Clarify or constrain how `*args`/`**kwargs` are meant to be used with `press_key`, since they’re silently ignored when no locator is provided.
Because `locator` defaults to `None`, any `*args`/`**kwargs` are only forwarded to `element()` when a locator is provided. Calls like `press_key("Enter", some_timeout)` would silently ignore `some_timeout`, which is easy to miss and can introduce subtle bugs. Please either adjust the API (e.g. make `locator` keyword-only or accept only `**element_kwargs`) or raise an error when extra arguments are passed without a locator so misuse is surfaced early.
Suggested implementation:
```python
def press_key(
self,
key: str,
locator: LocatorAlias = None,
**element_kwargs,
) -> None:
"""Press a keyboard key, optionally targeting a specific element.
When a locator is provided the key press is scoped to that element
(``Locator.press``). Without a locator the key is sent to whatever
element currently has focus (``page.keyboard.press``).
Any additional keyword arguments in ``element_kwargs`` are forwarded
to :meth:`element` when a ``locator`` is provided (for example, to
control waiting/timeout behavior). Passing extra keyword arguments
without a locator has no effect and is considered a misuse; callers
should always provide ``locator=...`` when supplying ``element_kwargs``.
Args:
key: Key name — e.g. ``"Enter"``, ``"Escape"``, ``"Space"``,
``"Backspace"``, ``"ArrowDown"``, ``"Control+A"``.
locator: Optional element locator to press the key on.
element_kwargs: Additional keyword arguments forwarded to
:meth:`element` when ``locator`` is provided.
```
To fully enforce the behavior described in the docstring and surface misuse early, you should also:
1. In the body of `press_key`, add a guard at the top of the function:
```python
if locator is None and element_kwargs:
raise TypeError(
"press_key() received keyword arguments without a locator; "
"pass 'locator=...' when providing element lookup arguments."
)
```
2. Update any internal calls to `press_key` that were passing positional arguments after `key` (other than the locator) so they now pass keyword-only arguments via `element_kwargs` (e.g. `press_key("Enter", locator=foo, timeout=5)`).
3. Ensure that wherever `element()` is called inside `press_key`, it is updated to accept `**element_kwargs` instead of the previous `*args, **kwargs` signature (if applicable).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #303 +/- ##
==========================================
- Coverage 92.99% 92.82% -0.18%
==========================================
Files 19 19
Lines 2698 2744 +46
==========================================
+ Hits 2509 2547 +38
- Misses 189 197 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
digitronik
force-pushed
the
some_playwright_methods
branch
6 times, most recently
from
1076887 to
990fe9a
Compare
Signed-off-by: Nikhil Dhandre <ndhandre@redhat.com>
digitronik
force-pushed
the
some_playwright_methods
branch
from
990fe9a to
61d5b3d
Compare
mshriver
reviewed
May 6, 2026
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
2 participants
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 higher-level Playwright browser utilities for screenshots, page content, cookies, keyboard input, and network interception.
New Features:
Enhancements:
Tests: