Skip to content

Commit 4050387

Browse files
committed
Make execute_javascript throw a JavascriptError if the javascript failed.
- Require this interface to use returnByValue = True, because we want to help the user by getting the value out of the result object. - We should either use returnByValue or return the result not the value. I chose the latter because that's what any existing user expects it to do.
1 parent 7553510 commit 4050387

File tree

4 files changed

+72
-7
lines changed

4 files changed

+72
-7
lines changed

browserdebuggertools/chrome/interface.py

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
from typing import Optional
55

66
from browserdebuggertools import models
7-
from browserdebuggertools.exceptions import TargetNotFoundError
7+
from browserdebuggertools.exceptions import TargetNotFoundError, JavascriptError
88
from browserdebuggertools.targets_manager import TargetsManager
99

1010
logging.basicConfig(format='%(levelname)s:%(message)s')
@@ -151,11 +151,22 @@ def stop_page_load(self):
151151
return self.execute("Page", "stopLoading")
152152

153153
def execute_javascript(self, script, **kwargs):
154+
if "returnByValue" in kwargs:
155+
if not kwargs["returnByValue"]:
156+
raise ValueError(
157+
"If want returnByValue as False, "
158+
f"use .execute('runtime', 'evaluate', "
159+
f"{{'expression': '{script}', returnByValue: False}}) directly"
160+
)
154161
params = {
155162
"expression": script,
163+
"returnByValue": True
156164
}
157165
params.update(kwargs)
166+
158167
result = self.execute("Runtime", "evaluate", params)["result"]
168+
if result.get("subtype") == "error":
169+
raise JavascriptError(f"{result['description']}")
159170

160171
return result.get("value")
161172

browserdebuggertools/exceptions.py

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,10 @@ class JavascriptDialogNotFoundError(NotFoundError):
4141
pass
4242

4343

44+
class JavascriptError(DevToolsException):
45+
pass
46+
47+
4448
class MaxRetriesException(DevToolsException):
4549
pass
4650

tests/e2etests/chrome/test_interface.py

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -563,9 +563,15 @@ def test(self: Union[TestCase, ChromeInterfaceTest]):
563563

564564
# Block any new document requests and open url in a new tab
565565
self.devtools_client.block_main_frames()
566-
self.devtools_client.execute_javascript(
567-
f"window.open('http://localhost:{self.testSite.port}/simple_page_2', '_blank')",
568-
returnByValue=False, userGesture=True
566+
# In Chrome 112 for some reason we can't specify useGesture and returnByValue at the same
567+
# time, if we do we get an 'Object reference chain is too long' error.
568+
# So don't use execute_javascript which insists on setting returnByValue.
569+
self.devtools_client.execute(
570+
"Runtime", "evaluate",
571+
{
572+
"expression": f"window.open('http://localhost:{self.testSite.port}/simple_page_2', '_blank')",
573+
"userGesture": True
574+
}
569575
)
570576

571577
# Initially the title of the new tab is equal to the URL, but if we wait a bit it will

tests/unittests/chrome/test_interface.py

Lines changed: 47 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
1+
import re
12
from unittest import TestCase
2-
33
from unittest.mock import patch, MagicMock
44

55
from browserdebuggertools.chrome.interface import ChromeInterface
6-
from browserdebuggertools.exceptions import TargetNotFoundError
6+
from browserdebuggertools.exceptions import TargetNotFoundError, JavascriptError
77

88
MODULE_PATH = "browserdebuggertools.chrome.interface."
99

@@ -25,10 +25,54 @@ def test(self, mockExecute):
2525
result = self.interface.execute_javascript("document.readyState", foo="baa", x=2)
2626

2727
mockExecute.assert_called_once_with(
28-
"Runtime", "evaluate", {"expression": "document.readyState", "foo": "baa", "x": 2}
28+
"Runtime", "evaluate",
29+
{
30+
"expression": "document.readyState",
31+
"foo": "baa", "x": 2, "returnByValue": True
32+
}
2933
)
3034
self.assertEqual(mock_result, result)
3135

36+
def test_value_error(self, mockExecute):
37+
mock_result = MagicMock()
38+
mockExecute.return_value = {"id": 1, "result": {"value": mock_result}}
39+
40+
with self.assertRaisesRegex(
41+
ValueError,
42+
re.escape(
43+
"If want returnByValue as False, "
44+
"use .execute('runtime', 'evaluate', "
45+
"{'expression': 'document.readyState', returnByValue: False}) directly"
46+
)
47+
):
48+
self.interface.execute_javascript(
49+
"document.readyState", returnByValue=False
50+
)
51+
52+
self.assertFalse(mockExecute.called)
53+
54+
def test_javascript_error(self, mockExecute):
55+
mockExecute.return_value = {
56+
"result": {
57+
"type": "object",
58+
"subtype": "error",
59+
"className": "SyntaxError",
60+
"description": "SyntaxError: Unexpected identifier 'and'",
61+
"objectId": "1297274167796877720.2.1"
62+
}
63+
}
64+
65+
with self.assertRaises(JavascriptError):
66+
self.interface.execute_javascript("garbage and stuff", foo="baa", x=2)
67+
68+
mockExecute.assert_called_once_with(
69+
"Runtime", "evaluate",
70+
{
71+
"expression": "garbage and stuff",
72+
"foo": "baa", "x": 2, "returnByValue": True
73+
}
74+
)
75+
3276

3377
class Test_ChromeInterface_switch_target(ChromeInterfaceTest):
3478

0 commit comments

Comments
 (0)