feat(element): add timeout parameter to click() and click_using_js()#400
Open
gab-i-alves wants to merge 6 commits into
Open
feat(element): add timeout parameter to click() and click_using_js()#400gab-i-alves wants to merge 6 commits into
gab-i-alves wants to merge 6 commits into
Conversation
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_web_element.py (1)
730-757: Add parity test for “already visible + timeout” onclick_using_js.You already validate this optimization for
click(...)(Line 839+). Adding the same assertion forclick_using_js(...)would prevent future regressions in the JS path.✅ Optional test addition
+ `@pytest.mark.asyncio` + async def test_click_using_js_visible_ignores_timeout(self, web_element): + """Already-visible element should not wait even when timeout is provided.""" + web_element.is_visible = AsyncMock(return_value=True) + web_element.scroll_into_view = AsyncMock() + web_element.wait_until = AsyncMock() + web_element.execute_script = AsyncMock( + return_value={'result': {'result': {'value': True}}} + ) + + await web_element.click_using_js(timeout=5) + + web_element.wait_until.assert_not_called()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_web_element.py` around lines 730 - 757, Add a parity test to ensure click_using_js short-circuits when the element is already visible even if a timeout is passed: create a test (e.g. test_click_using_js_visible_with_timeout_does_not_wait) that sets web_element.is_visible = AsyncMock(return_value=True), web_element.scroll_into_view = AsyncMock(), web_element.wait_until = AsyncMock(), web_element.execute_script = AsyncMock(return_value={'result': {'result': {'value': True}}}), call await web_element.click_using_js(timeout=5), and assert that web_element.wait_until.assert_not_called() and that web_element.execute_script (and optionally web_element.scroll_into_view) was called as expected to mirror the existing click(...) optimization.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pydoll/elements/web_element.py`:
- Around line 552-555: The code treats any truthy timeout (including negative
numbers) as a wait request; add explicit validation to reject negative timeout
values by checking the timeout parameter before branching: if timeout is not
None and timeout < 0 raise a ValueError with a clear message (e.g., "timeout
must be non-negative"); then keep the existing logic that calls wait_until when
timeout > 0 and raises ElementNotVisible() when timeout is zero/None. Apply this
validation in both places where timeout is checked (the branch that calls
wait_until and the similar block around lines 597-600) and reference the
wait_until call, the timeout parameter, and ElementNotVisible in those methods.
---
Nitpick comments:
In `@tests/test_web_element.py`:
- Around line 730-757: Add a parity test to ensure click_using_js short-circuits
when the element is already visible even if a timeout is passed: create a test
(e.g. test_click_using_js_visible_with_timeout_does_not_wait) that sets
web_element.is_visible = AsyncMock(return_value=True),
web_element.scroll_into_view = AsyncMock(), web_element.wait_until =
AsyncMock(), web_element.execute_script = AsyncMock(return_value={'result':
{'result': {'value': True}}}), call await web_element.click_using_js(timeout=5),
and assert that web_element.wait_until.assert_not_called() and that
web_element.execute_script (and optionally web_element.scroll_into_view) was
called as expected to mirror the existing click(...) optimization.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9e07631e-832c-4f90-8e39-8e2ef7878c75
📒 Files selected for processing (2)
pydoll/elements/web_element.pytests/test_web_element.py
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/test_web_element.py (3)
737-751: Assert post-wait click execution, not only the wait call.On Line 746 and Line 748, the test proves waiting happened, but not that the click actually proceeded afterward.
Suggested assertion additions
await web_element.click_using_js(timeout=5) web_element.wait_until.assert_called_once_with( is_visible=True, timeout=5 ) + web_element.scroll_into_view.assert_called_once() + web_element.execute_script.assert_called_once()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_web_element.py` around lines 737 - 751, The test test_click_using_js_not_visible_with_timeout_waits currently only asserts web_element.wait_until was called; update it to also assert the JavaScript click actually ran after the wait by asserting web_element.execute_script (or the method that performs the JS click used by click_using_js) was called once with the expected script/arguments after invoking web_element.click_using_js(timeout=5); reference the test function name test_click_using_js_not_visible_with_timeout_waits and the mocks web_element.wait_until and web_element.execute_script to add the post-wait assertion.
835-854: Strengthen this test to verify the click path runs after waiting.On Line 849 and Line 851, the wait contract is checked, but the actual click execution is not asserted.
Suggested assertion additions
with patch('asyncio.sleep'): await web_element.click(timeout=5) web_element.wait_until.assert_called_once_with( is_visible=True, timeout=5 ) + web_element.scroll_into_view.assert_called_once() + assert web_element._connection_handler.execute_command.call_count == 3🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_web_element.py` around lines 835 - 854, The test test_click_not_visible_with_timeout_waits should also assert that the click path runs after waiting: after awaiting web_element.click(timeout=5) verify that web_element.wait_until was called (already present) and additionally assert that web_element.scroll_into_view was called and that web_element._connection_handler.execute_command was invoked for the click action (e.g., verify execute_command.call_count increased and/or that one of the calls contains the expected click-related payload). Update the test to check scroll_into_view.assert_called_once() and inspect execute_command.mock_calls to confirm a click command was issued.
867-884: Add one execution assertion for the “visible ignores timeout” path.Line 883 validates the no-wait branch, but adding a click-path assertion would prevent false positives.
Suggested assertion additions
with patch('asyncio.sleep'): await web_element.click(timeout=5) web_element.wait_until.assert_not_called() + web_element.scroll_into_view.assert_called_once() + assert web_element._connection_handler.execute_command.call_count == 3🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_web_element.py` around lines 867 - 884, The test test_click_visible_ignores_timeout currently asserts wait_until was not called but misses verifying the click execution path; after awaiting web_element.click(timeout=5) add an assertion that the click invoked the connection layer (e.g. web_element._connection_handler.execute_command.assert_called() or a more specific assert_called_with / assert_has_calls) so the test ensures the click actually executed via web_element.click and not a no-op.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_web_element.py`:
- Around line 737-751: The test
test_click_using_js_not_visible_with_timeout_waits currently only asserts
web_element.wait_until was called; update it to also assert the JavaScript click
actually ran after the wait by asserting web_element.execute_script (or the
method that performs the JS click used by click_using_js) was called once with
the expected script/arguments after invoking
web_element.click_using_js(timeout=5); reference the test function name
test_click_using_js_not_visible_with_timeout_waits and the mocks
web_element.wait_until and web_element.execute_script to add the post-wait
assertion.
- Around line 835-854: The test test_click_not_visible_with_timeout_waits should
also assert that the click path runs after waiting: after awaiting
web_element.click(timeout=5) verify that web_element.wait_until was called
(already present) and additionally assert that web_element.scroll_into_view was
called and that web_element._connection_handler.execute_command was invoked for
the click action (e.g., verify execute_command.call_count increased and/or that
one of the calls contains the expected click-related payload). Update the test
to check scroll_into_view.assert_called_once() and inspect
execute_command.mock_calls to confirm a click command was issued.
- Around line 867-884: The test test_click_visible_ignores_timeout currently
asserts wait_until was not called but misses verifying the click execution path;
after awaiting web_element.click(timeout=5) add an assertion that the click
invoked the connection layer (e.g.
web_element._connection_handler.execute_command.assert_called() or a more
specific assert_called_with / assert_has_calls) so the test ensures the click
actually executed via web_element.click and not a no-op.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9fbaa140-1072-49da-887f-c61d2aa30ba3
📒 Files selected for processing (1)
tests/test_web_element.py
thalissonvs
requested changes
Jul 16, 2026
Resolve conflict from the tests/ -> tests/unit + tests/integration reorg and apply review feedback (thalissonvs): use wait_until as the single visibility gate in click() and click_using_js() instead of the is_visible()/timeout branching. - wait_until now fails fast when timeout=0 (checks once, then raises WaitElementTimeout) so the default no-wait behavior is preserved. - the not-visible path now raises WaitElementTimeout (was ElementNotVisible). - port the click-timeout unit tests to tests/unit/test_web_element.py and add real-browser coverage (wait-then-click, timeout expiry) in the integration suite with a late-appearing button fixture.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pydoll/elements/web_element.py (1)
487-487: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
floatfor timeout type annotations.Timeouts in Python are commonly fractional (e.g.,
timeout=0.5). The type hintintrestricts this unnecessarily, while the underlying polling logic (loop.time() - start_time >= timeout) fully supports floats. This aligns with standard asyncio interfaces andWebElement.get_shadow_root, which already usestimeout: float.
pydoll/elements/web_element.py#L487-L487: changetimeout: int = 0totimeout: float = 0inwait_until.pydoll/elements/web_element.py#L531-L531: changetimeout: int = 0totimeout: float = 0inclick_using_js.pydoll/elements/web_element.py#L569-L569: changetimeout: int = 0totimeout: float = 0inclick.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pydoll/elements/web_element.py` at line 487, Update the timeout annotations in wait_until, click_using_js, and click to use float instead of int while preserving the default value of 0, enabling fractional timeout values consistently with get_shadow_root.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pydoll/elements/web_element.py`:
- Around line 554-555: In pydoll/elements/web_element.py at lines 554-555,
update click_using_js to call wait_until(is_visible=True, timeout=timeout)
before scroll_into_view. Apply the same ordering change in click at lines
599-600 so both methods wait for visibility before scrolling.
---
Nitpick comments:
In `@pydoll/elements/web_element.py`:
- Line 487: Update the timeout annotations in wait_until, click_using_js, and
click to use float instead of int while preserving the default value of 0,
enabling fractional timeout values consistently with get_shadow_root.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1bedd349-e948-4ec3-a125-6f1722a61e0d
📒 Files selected for processing (4)
pydoll/elements/web_element.pytests/integration/pages/web_element.htmltests/integration/test_web_element_integration.pytests/unit/test_web_element.py
The mouse click() path needs the element on-screen: scroll_into_view runs while the button is still display:none (zero box, no-op), so a button below the fold never received the dispatched mouse event. Move it into the initial viewport.
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.
Description
Add an optional
timeoutparameter toclick()andclick_using_js()onWebElement.When an element is not immediately visible, callers can now pass
timeout=Nto wait up to N secondsfor the element to become visible before clicking. Internally delegates to the existing
wait_until(is_visible=True)method, keeping the implementation DRY.Default
timeout=0preserves current behavior (raisesElementNotVisibleimmediately).Related Issue(s)
Closes #274
Type of Change
How Has This Been Tested?
Testing Checklist
Implementation Details
wait_until(is_visible=True, timeout=N)which already implements the polling loopwith 0.5s intervals and raises
WaitElementTimeouton expiry.for both
click()andclick_using_js().API Changes
click(): new keyword argumenttimeout: int = 0click_using_js(): new keyword argumenttimeout: int = 0Checklist before requesting a review
poetry run task lintand fixed any issuesSummary by CodeRabbit
New Features
timeoutparameter to both standard and JavaScript-based element clicks.Bug Fixes
timeout=0: visibility is checked once and fails immediately if not met.timeoutvalues now raise aValueError.WaitElementTimeout(instead of prior visibility-specific errors).Tests