[py] Add detailed error message if KeyError thrown in get_window_size and get_window_position (#15503)

[aikfiend]

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Motivation and Context

It returns a more detailed error than KeyError 'width' for a better understanding on which side is the error, the webdriver itself or the remote web server. Unfortunately, the build system is too complicated to do a simple fix and I do not agree and do not want to run all the existing tests as the changes made do not affect the other tests. Here I belong to your CI

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• I have read the contributing document.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

---

PR Type

Bug fix, Tests

---

Description

• Added detailed error messages for KeyError in get_window_size and get_window_position.

• Enhanced error handling to clarify missing keys in window size and position.

• Introduced new tests to validate error handling for missing keys.

• Used mocking and parameterized tests to ensure robustness.

---

Changes walkthrough 📝

	Relevant files
Bug fix	webdriver.py Add detailed error handling for `KeyError` in window methods py/selenium/webdriver/remote/webdriver.py Added try-except blocks to handle KeyError in get_window_size and get_window_position. Raised detailed error messages specifying missing keys. +8/-2
Tests	window_tests.py Add tests for missing keys in window methods                          py/test/selenium/webdriver/common/window_tests.py Added parameterized tests for missing keys in get_window_size and get_window_position. Used mocking to simulate missing keys in get_window_rect. Validated error messages for KeyError exceptions. +24/-0

---

Need help?

Type /help how to ... in the comments thread for any questions about Qodo Merge usage.

Check out the documentation for more information.

[CLAassistant]

All committers have signed the CLA.

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Error Message Format The error message format includes the raw dictionary content which might expose internal implementation details. Consider if a more user-friendly message would be better. raise KeyError(f"No size with key: {e.args[0]} existed in {size}") Test Robustness The tests use random integers which could potentially lead to flaky tests. Consider using fixed values for more predictable test behavior. driver.get_window_rect = Mock(return_value={k: randint(1, 15360) for k in ("width", "height") if k != missing_key})

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Learned best practice	Enhance exception messages with more context to improve debugging experience The error message could be more helpful by providing additional context about what operation was being performed and what the expected keys are. This would make debugging easier for users of the library. py/selenium/webdriver/remote/webdriver.py [1047-1050] try: return {k: size[k] for k in ("width", "height")} except KeyError as e: - raise KeyError(f"No size with key: {e.args[0]} existed in {size}") + raise KeyError(f"Failed to get window size: missing required key '{e.args[0]}' in response {size}. Expected keys 'width' and 'height'.") Apply this suggestion Suggestion importance[1-10]: 6	Low
Update

[aikfiend]

The tests use random integers which could potentially lead to flaky tests. Consider using fixed values for more predictable test behavior.

Unfortunately no. Because in the current context, it is not relevant at all. We do not need the exact values, we are not testing mathematics or calculations here.

From the testing point of view:

a = 100
assert a == 100

is a bicycle of trying to test the Python itself.

The real testing starts when you try to use random data, while you already know what are your min and max values or data types.

[cgoldberg]

The real testing starts when you try to use random data

I don't see the point of using random data here. You are mocking the function and setting a return value. What's the advantage of having it change every time you run the test?

[aikfiend]

I don't see the point of using random data here. You are mocking the function and setting a return value. What's the advantage of having it change every time you run the test?

I do not see any advantage in using the hardcoded value. In real-world window size and position may vary up to the max screen resolution known today - 16k.

[cgoldberg]

Please remove the randomization, as it serves no purpose. I will not merge code that adds complexity for no benefit.

[aikfiend]

Please remove the randomization, as it serves no purpose. I will not merge code that adds complexity for no benefit.

Removed only as it became a requirement for merging, but not agree with reviewer

[cgoldberg]

@aikfiend
I'm no longer interested in reviewing your code and will not be merging it. Maybe somebody else will.