Handle case where Browser.WindowID is not found and improve error handling. #63
Conversation
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Hi @cleitonleonel , first of all, thanks for your contribution! Sorry for not have a contrib file. You have to run the lint to check the code format: |
pydoll/browser/base.py
Outdated
| pages = await self.get_targets() | ||
| valid_page = next( | ||
| (page for page in pages if page.get('type') == 'page' and page.get('attached')), | ||
| None | ||
| ) | ||
|
|
||
| if not valid_page: | ||
| raise RuntimeError("No valid attached browser page found.") | ||
|
|
||
| target_id = valid_page.get('targetId') | ||
| if not target_id: | ||
| raise RuntimeError("Valid page found but missing 'targetId'.") | ||
|
|
||
| command = BrowserCommands.get_window_id_by_target(target_id) | ||
| response = await self._execute_command(command) |
There was a problem hiding this comment.
I think you can refactor this method to simplify the tests. For example, you can create a method to get the valid page and validate before returns. Also, you can pass BrowserCommands.get_window_id_by_target(target_id) direct to the _execute_command method, since there's no other use for the variable command
There was a problem hiding this comment.
That way we can have a more clean implementation and easy to follow
There was a problem hiding this comment.
Here's my suggestion:
if response.get('error'):
target_id = self._get_valid_target_id() # this will raise the exceptions and do the validations
response = await self._execute_command(
BrowserCommands.get_window_id_by_target(target_id)
)This way you can write tests more easily
pydoll/browser/base.py
Outdated
| try: | ||
| return response['result']['windowId'] | ||
| except KeyError: | ||
| error_message = response.get('error', {}).get('message', "Unknown error") | ||
| raise RuntimeError(f"Failed to get window ID: {error_message}") |
There was a problem hiding this comment.
We want to avoid try/except block always as possible, i would suggest something like this:
if (window_id := response.get('result', {}).get('windowId')) is not None:
return window_id
error_data = response.get('error', {})
raise RuntimeError('error message')what do you think?
Description
Improved error handling when retrieving Browser.WindowID. Now, if Browser.WindowID is not found, the code properly handles cases where pages are not attached or targetId is missing, preventing unexpected failures..
Changes
Handle missing Browser.WindowID gracefully.
Added a new method that gets the window ID based on the target ID.
Added extra validation for missing targetId and improved error messages.
Testing
Tests passing, test_browser and test_chrome.
This PR addresses issue #57 by fix bug Browser.WindowID wasn't found.