Repository navigation
PEP 553 built-in debug() function (bpo-31353) #3355
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 35 commits
9dc577a
bc4634d
f7e3c40
6696c23
4600bcf
0f46af7
a141f63
79d7ac8
1666925
8437033
6534542
2d75d7a
ed649fa
0bea882
eee151d
05b7cae
6b9a59c
5e0b93a
c57d0dc
eb17d0f
244bff5
c4bed66
6eb3470
af81484
f3200c7
1c60e11
ed41006
b84a018
f6db9e3
cf9e7ac
e46c59d
9bc36b8
b5c2393
2846d3b
50c3e40
6b23d5b
0d2fae5
1cf3e06
6217e90
28bd8e4
8808367
bd68b76
fea5d3d
599108d
a74f23e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,9 +17,11 @@ | |
| import types | ||
| import unittest | ||
| import warnings | ||
| from contextlib import ExitStack | ||
| from operator import neg | ||
| from test.support import TESTFN, unlink, check_warnings | ||
| from test.support import EnvironmentVarGuard, TESTFN, unlink, check_warnings | ||
| from test.support.script_helper import assert_python_ok | ||
| from unittest.mock import patch | ||
| try: | ||
| import pty, signal | ||
| except ImportError: | ||
|
|
@@ -1511,6 +1513,126 @@ def test_construct_singletons(self): | |
| self.assertRaises(TypeError, tp, 1, 2) | ||
| self.assertRaises(TypeError, tp, a=1, b=2) | ||
|
|
||
|
|
||
| class TestBreakpoint(unittest.TestCase): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe you should add a setUp() method using self.addCleanup() to save/restore sys.breakpointhook value, instead of doing that in each test.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this is a great suggestion, coupled with the observation below that the test suite could be run with I'm going to use an |
||
| def test_breakpoint(self): | ||
| with patch('pdb.set_trace') as mock: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hum, would it be possible to modify the unit test so it doesn't rely on breakpoint() calling pdb.set_trace()? The test may be run with PYTHONBREAKPOINT set to something else.. to debug the test (or another test while running the test suite). Or skip the test if sys.breakpointhook is no sys.breakpointhook?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See above about the clean slate, but in this case we're testing the default behavior, which is exactly that the clean slate calls |
||
| breakpoint() | ||
| mock.assert_called_once() | ||
|
|
||
| def test_breakpoint_with_breakpointhook_set(self): | ||
| call_status = 'Not called' | ||
| def my_breakpointhook(): | ||
| nonlocal call_status | ||
| call_status = 'Called' | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You might use a mock to get arguments: assert_called_once_with(...).
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not quite sure what you're suggesting here. There are other tests to ensure that the right positional and keyword arguments are passed through. This test just ensures that a custom
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think haypo is suggesting code like: myhook = mock.MagicMock()
sys.breakpointhook = myhook
breakpoint('hello')
myhook.assert_called_once_with('hello')instead of manual tracking of the call status.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @merwok ah, thanks! I like that. |
||
| try: | ||
| sys.breakpointhook = my_breakpointhook | ||
| breakpoint() | ||
| finally: | ||
| sys.breakpointhook = sys.__breakpointhook__ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suggest to use support.swap_attr(sys, "breakpointhook", ...): it's shorter and restore the variable to its previous value. Again, the test runner might have replaced sys.breakpointhook for good reasons. Don't reset it to sys.breakpointhook.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Excellent suggestion. I'm going to use this as mentioned above. |
||
| self.assertEqual(call_status, 'Called') | ||
|
|
||
| def test_breakpoint_with_breakpointhook_reset(self): | ||
| call_status = 'Not called' | ||
| def my_breakpointhook(): | ||
| nonlocal call_status | ||
| call_status = 'Called' | ||
| try: | ||
| sys.breakpointhook = my_breakpointhook | ||
| breakpoint() | ||
| finally: | ||
| sys.breakpointhook = sys.__breakpointhook__ | ||
| self.assertEqual(call_status, 'Called') | ||
| with patch('pdb.set_trace') as mock: | ||
| breakpoint() | ||
| mock.assert_called_once() | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be possible to restore sys.breakpointhook to its original value at the end of the test?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, see above. |
||
|
|
||
| def test_breakpoint_with_args_and_keywords(self): | ||
| call_args = None | ||
| def my_breakpointhook(*args, **kws): | ||
| nonlocal call_args | ||
| call_args = args, kws | ||
| try: | ||
| sys.breakpointhook = my_breakpointhook | ||
| breakpoint(1, 2, 3, four=4, five=5) | ||
| finally: | ||
| sys.breakpointhook = sys.__breakpointhook__ | ||
| self.assertEqual(call_args, ((1, 2, 3), dict(four=4, five=5))) | ||
|
|
||
| def test_breakpoint_with_passthru_error(self): | ||
| def my_breakpointhook(): | ||
| pass | ||
| try: | ||
| sys.breakpointhook = my_breakpointhook | ||
| self.assertRaises(TypeError, breakpoint, 1, 2, 3, four=4, five=5) | ||
| finally: | ||
| sys.breakpointhook = sys.__breakpointhook__ | ||
|
|
||
| def test_envar_good_path_builtin(self): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. env var tests expect that sys.breakpointhook is the default hook. Maybe put these tests in a new test case and set sys.breakpointhook to sys.breakpointhook in setUp() (and use addCleanup to restore the old value)? Or maybe rewrite the tests to call directly sys.breakpointhook rather than testing indirectly that breakpoint() calls sys.breakpointhook and expect sys.breakpointhook to be sys.breakpointhook?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a good suggestion. I think the clean-slate setup will address this. |
||
| with ExitStack() as resources: | ||
| env = resources.enter_context(EnvironmentVarGuard()) | ||
| env['PYTHONBREAKPOINT'] = 'int' | ||
| mock = resources.enter_context(patch('builtins.int')) | ||
| breakpoint('7') | ||
| mock.assert_called_once_with('7') | ||
|
|
||
| def test_envar_good_path_other(self): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be possible to add a second V, I'm confused by "envar" for "env var" :-)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, ha! "envvar" always looks weird to me. I've always read/written the shortcut for "environment variable" as "envar" so that's just a habit I always have to break when using Both are reasonable shortcuts, e.g. environment variable vs. environment variable. If you don't mind, I'll keep it this way.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't mind.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What madman shortens environment to en? :)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| with ExitStack() as resources: | ||
| env = resources.enter_context(EnvironmentVarGuard()) | ||
| env['PYTHONBREAKPOINT'] = 'sys.exit' | ||
| mock = resources.enter_context(patch('sys.exit')) | ||
| breakpoint() | ||
| mock.assert_called_once_with() | ||
|
|
||
| def test_envar_good_path_noop_0(self): | ||
| with ExitStack() as resources: | ||
| env = resources.enter_context(EnvironmentVarGuard()) | ||
| env['PYTHONBREAKPOINT'] = '0' | ||
| mock = resources.enter_context(patch('pdb.set_trace')) | ||
| breakpoint() | ||
| mock.assert_not_called() | ||
|
|
||
| def test_envar_good_path_empty_string(self): | ||
| # PYTHONBREAKPOINT='' is the same as it not being set. | ||
| with ExitStack() as resources: | ||
| env = resources.enter_context(EnvironmentVarGuard()) | ||
| env['PYTHONBREAKPOINT'] = '' | ||
| mock = resources.enter_context(patch('pdb.set_trace')) | ||
| breakpoint() | ||
| mock.assert_called_once_with() | ||
|
|
||
| def test_envar_unimportable(self): | ||
| for envar in ( | ||
| '.', '..', '.foo', 'foo.', '.int', 'int.' | ||
| 'nosuchbuiltin', | ||
| 'nosuchmodule.nosuchcallable', | ||
| ): | ||
| with self.subTest(envar=envar): | ||
| with ExitStack() as resources: | ||
| env = resources.enter_context(EnvironmentVarGuard()) | ||
| env['PYTHONBREAKPOINT'] = envar | ||
| mock = resources.enter_context(patch('pdb.set_trace')) | ||
| w = resources.enter_context(check_warnings(quiet=True)) | ||
| breakpoint() | ||
| self.assertEqual( | ||
| str(w.message), | ||
| f'Ignoring unimportable $PYTHONBREAKPOINT: "{envar}"') | ||
| self.assertEqual(w.category, RuntimeWarning) | ||
| mock.assert_not_called() | ||
|
|
||
| def test_envar_ignored_when_hook_is_set(self): | ||
| with ExitStack() as resources: | ||
| env = resources.enter_context(EnvironmentVarGuard()) | ||
| env['PYTHONBREAKPOINT'] = 'sys.exit' | ||
| mock = resources.enter_context(patch('sys.exit')) | ||
| try: | ||
| sys.breakpointhook = int | ||
| breakpoint() | ||
| finally: | ||
| sys.breakpointhook = sys.__breakpointhook__ | ||
| mock.assert_not_called() | ||
|
|
||
|
|
||
| @unittest.skipUnless(pty, "the pty and signal modules must be available") | ||
| class PtyTests(unittest.TestCase): | ||
| """Tests that use a pseudo terminal to guarantee stdin and stdout are | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| PEP 553 - Add a new built-in called ``breakpoint()`` which calls | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. PEP 553 can be written: to get a direct link to the PEP ;-)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn’t docutils auto-linkify text that looks like |
||
| ``sys.breakpointhook()``. By default this imports ``pdb`` and calls | ||
| ``pdb.set_trace()``, but users may override ``sys.breakpointhook()`` to call | ||
| whatever debugger they want. The original value of the hook is saved in | ||
| ``sys.__breakpointhook__``. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -422,6 +422,29 @@ builtin_callable(PyObject *module, PyObject *obj) | |
| return PyBool_FromLong((long)PyCallable_Check(obj)); | ||
| } | ||
|
|
||
| static PyObject * | ||
| builtin_breakpoint(PyObject *self, PyObject *args, PyObject *keywords) | ||
| { | ||
| PyObject *hook = PySys_GetObject("breakpointhook"); | ||
|
|
||
| if (hook == NULL) { | ||
| PyErr_SetString(PyExc_RuntimeError, "lost sys.breakpointhook"); | ||
| return NULL; | ||
| } | ||
| Py_INCREF(hook); | ||
| PyObject *retval = PyObject_Call(hook, args, keywords); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You might try to use the new METH_FASTCALL calling convention. It prevents the creation a temporary tuple for 'args' (and dict for 'keywords') and so make your function faster :-D
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Heh. Since this isn't even documented I didn't know about them! I'll think about it (I hate to perpetuate the use of undocumented APIs). |
||
| Py_DECREF(hook); | ||
| return retval; | ||
| } | ||
|
|
||
| PyDoc_STRVAR(breakpoint_doc, | ||
| "breakpoint(*args, **kws)\n\ | ||
| \n\ | ||
| Call the sys.breakpointhook() function, passing *args and **kws\n\ | ||
| through unchanged. sys.breakpointhook() must accept the arguments\n\ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not simply "Call sys.breakpointhook(*args, **kwds)."? It's just a suggestion. I don't know what is the best. |
||
| passed.\n\ | ||
| \n\ | ||
| By default, this drops you into the pdb debugger."); | ||
|
|
||
| typedef struct { | ||
| PyObject_HEAD | ||
|
|
@@ -2627,6 +2650,7 @@ static PyMethodDef builtin_methods[] = { | |
| BUILTIN_ANY_METHODDEF | ||
| BUILTIN_ASCII_METHODDEF | ||
| BUILTIN_BIN_METHODDEF | ||
| {"breakpoint", (PyCFunction)builtin_breakpoint, METH_VARARGS | METH_KEYWORDS, breakpoint_doc}, | ||
| BUILTIN_CALLABLE_METHODDEF | ||
| BUILTIN_CHR_METHODDEF | ||
| BUILTIN_COMPILE_METHODDEF | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PYTHONBREAKPOINT should be written:
(as you did in the What's New in Python 3.7) and you should document the variable in Doc/using/cmdline.rst.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed!