Home Original page

[py][bidi] Allow resetting viewport by cgoldberg · Pull Request #16623 · SeleniumHQ/selenium

PR Code Suggestions ✨

Latest suggestions up to ce185c6

CategorySuggestion                                                                                                                                    Impact
General
Make viewport size asserts tolerant

In test_set_viewport, modify the viewport dimension assertions to allow a small
tolerance (e.g., 2 pixels) to prevent flaky tests caused by platform-specific
rounding or UI rendering differences.

py/test/selenium/webdriver/common/bidi_browsing_context_tests.py [346-359]

 def test_set_viewport(driver, pages):
     """Test setting the viewport."""
     context_id = driver.current_window_handle
     driver.get(pages.url("formPage.html"))
 
     try:
         driver.browsing_context.set_viewport(context=context_id, viewport={"width": 251, "height": 301})
 
         viewport_size = driver.execute_script("return [window.innerWidth, window.innerHeight];")
 
-        assert viewport_size[0] == 251
-        assert viewport_size[1] == 301
+        assert abs(viewport_size[0] - 251) <= 2
+        assert abs(viewport_size[1] - 301) <= 2
     finally:
         driver.browsing_context.set_viewport(context=context_id, viewport=None, device_pixel_ratio=None)

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a potential source of test flakiness due to platform-specific rendering differences and proposes a reasonable solution to improve test robustness by using a tolerance for assertions.

Medium
✅ Align type hints with behavior
Suggestion Impact:The commit changed the type hint to include None as suggested and also fixed assigning the correct value to params["devicePixelRatio"].

code diff:

-        device_pixel_ratio: float | UNDEFINED = UNDEFINED,
+        device_pixel_ratio: float | None | UNDEFINED = UNDEFINED,
         user_contexts: list[str] | None = None,
     ) -> None:
         """Modifies specific viewport characteristics on the given top-level traversable.
@@ -1013,7 +1013,7 @@
         elif device_pixel_ratio is None:
             params["devicePixelRatio"] = None
         else:
-            params["devicePixelRatio"] = viewport
+            params["devicePixelRatio"] = device_pixel_ratio

Update the type hint for the device_pixel_ratio parameter in set_viewport to
float | None | UNDEFINED to match the implementation that accepts None for
resetting the value.

py/selenium/webdriver/common/bidi/browsing_context.py [984-990]

 def set_viewport(
     self,
     context: str | None = None,
     viewport: dict | None | UNDEFINED = UNDEFINED,
-    device_pixel_ratio: float | UNDEFINED = UNDEFINED,
+    device_pixel_ratio: float | None | UNDEFINED = UNDEFINED,
     user_contexts: list[str] | None = None,
 ) -> None:

[Suggestion processed]

Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies that the type hint for device_pixel_ratio is inconsistent with the implementation, which explicitly handles None as a valid input to reset the value.

Low
Learned
best practice
Clarify tri-state parameter semantics

Update the docstring to clearly document the tri-state behavior using the
UNDEFINED sentinel versus None, so callers understand omission vs reset
semantics.

py/selenium/webdriver/common/bidi/browsing_context.py [984-1001]

 def set_viewport(
     self,
     context: str | None = None,
     viewport: dict | None | UNDEFINED = UNDEFINED,
     device_pixel_ratio: float | UNDEFINED = UNDEFINED,
     user_contexts: list[str] | None = None,
 ) -> None:
     """Modifies specific viewport characteristics on the given top-level traversable.
 
     Args:
         context: The browsing context ID.
-        viewport: The viewport parameters (`None` resets to default).
-        device_pixel_ratio: The device pixel ratio (`None` resets default).
+        viewport: Viewport parameters. Use UNDEFINED to leave unchanged, None to reset to default, or a dict with dimensions to set.
+        device_pixel_ratio: Device pixel ratio. Use UNDEFINED to leave unchanged, None to reset to default, or a float to set.
         user_contexts: The user context IDs.
 
     Raises:
         Exception: If the browsing context is not a top-level traversable.
     """
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Enforce accurate and consistent documentation and naming to match actual behavior and APIs.

Low
  • Update

Previous suggestions

✅ Suggestions up to commit b91113a
CategorySuggestion                                                                                                                                    Impact
Possible issue
✅ Fix incorrect variable assignment bug
Suggestion Impact:The commit changes params["devicePixelRatio"] from viewport to device_pixel_ratio, exactly addressing the bug.

code diff:

-            params["devicePixelRatio"] = viewport
+            params["devicePixelRatio"] = device_pixel_ratio

In set_viewport, fix the incorrect assignment to params["devicePixelRatio"] by
using the device_pixel_ratio variable instead of viewport.

py/selenium/webdriver/common/bidi/browsing_context.py [1011-1016]

 if device_pixel_ratio is UNDEFINED:
     pass
 elif device_pixel_ratio is None:
     params["devicePixelRatio"] = None
 else:
-    params["devicePixelRatio"] = viewport
+    params["devicePixelRatio"] = device_pixel_ratio

[Suggestion processed]

Suggestion importance[1-10]: 10

__

Why: The suggestion correctly identifies a critical copy-paste bug that assigns the wrong variable, breaking the set_viewport functionality for the device_pixel_ratio parameter.

High
✅ Correct a test assertion typo
Suggestion Impact:The commit updated the assertion to use default_viewport_size[1] for the height comparison, correcting the typo as suggested.

code diff:

         assert viewport_size[0] == default_viewport_size[0]
-        assert viewport_size[1] == default_viewport_size[0]
+        assert viewport_size[1] == default_viewport_size[1]
         assert device_pixel_ratio == default_device_pixel_ratio

In test_set_viewport_back_to_default, correct the assertion for viewport height
to compare against the default height (default_viewport_size[1]) instead of the
default width.

py/test/selenium/webdriver/common/bidi_browsing_context_tests.py [402-404]

 assert viewport_size[0] == default_viewport_size[0]
-assert viewport_size[1] == default_viewport_size[0]
+assert viewport_size[1] == default_viewport_size[1]
 assert device_pixel_ratio == default_device_pixel_ratio

[Suggestion processed]

Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a copy-paste error in a test assertion, which would cause the test to validate the wrong behavior and potentially pass incorrectly.

High