Support for the new builtin breakpoint function in Python 3.7 #3331

tonybaloney · 2018-03-22T06:41:30Z

Working on conditions stated in #3180

Support for Python 3.7s builtin breakpoint() method.

These are the behaviours:

When breakpoint() is called and PYTHONBREAKPOINT is set to the default value, PyTest will use the Custom PDB trace UI instead of the system default Pdb.
When tests are complete, the system will default back to the system Pdb trace UI.
If --pdb is called on execution of Pytest, the custom Pdb interface is used on both breakpoint() and failed tests/unhandled exceptions.
If --pdbcls is used, the custom class will be executed when a test fails (as expected within existing behaviour), but also when breakpoint() is called from within a test, the custom class debugger will be instantiated.
When breakpoint() is called and PYTHONBREAKPOINT is NOT set to the default value, PyTest will not get involved.

Thanks for submitting a PR, your contribution is really appreciated!

Here's a quick checklist that should be present in PRs (you can delete this text from the final description, this is
just a guideline):

Create a new changelog file in the changelog folder, with a name like <ISSUE NUMBER>.<TYPE>.rst. See changelog/README.rst for details.
Target the master branch for bug fixes, documentation updates and trivial changes.
Target the features branch for new features and removals/deprecations.
Include documentation when adding new features.
Include new tests or update existing tests when applicable.

Unless your change is trivial or a small documentation fix (e.g., a typo or reword of a small section) please:

Add yourself to AUTHORS in alphabetical order;

…tin breakpoint function

coveralls · 2018-03-22T07:05:40Z

Coverage increased (+0.02%) to 92.821% when pulling 0762666 on tonybaloney:breakpoint_support into 6f95189 on pytest-dev:features.

tonybaloney · 2018-03-22T08:56:20Z

Discovered that the debugging module has no test coverage, so I want to develop tests for it before I start fiddling with it's behaviours.

RonnyPfannschmidt · 2018-03-22T08:58:02Z

@tonybaloney for historic reasons its in testing/test_pdb.py

tonybaloney · 2018-03-22T09:10:56Z

@RonnyPfannschmidt where would you recommend I put these tests? is test_debugging ok? Should I migrate the existing test case (perhaps in a seperate PR)?

RonnyPfannschmidt · 2018-03-22T09:32:11Z

@tonybaloney for now please put them into test-pdb, as a follow-up it seems sensible to rename that file

…esetting back when system default (pdb) is used""

…ystem default is set

…akpoint() not supported

…alled when breakpoint() is used

tonybaloney · 2018-03-23T04:43:09Z

This PR is ready for review, I still need to add some documentation to explain the behaviours, but would like feedback on the work so far (most of it was just tests tbh!)

tonybaloney · 2018-03-23T04:52:02Z

Tested with the example IPython class, which breaks but seems to be caused by #2064

(env) anthonyshaw ~/repo/test-pytest $ pytest test --pdbcls=IPython.terminal.debugger:TerminalPdb
=========================================================================================================== test session starts ============================================================================================================
platform darwin -- Python 3.7.0a4, pytest-3.4.3.dev70+g242fb785, py-1.5.3, pluggy-0.6.0
rootdir: /Users/anthonyshaw/repo/test-pytest, inifile:
collected 1 item

test/test_breakpoint.py
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> PDB set_trace (IO-capturing turned off) >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
F                                                                                                                                                                                                            [100%]

================================================================================================================= FAILURES =================================================================================================================
_____________________________________________________________________________________________________________ test_breakpoint ______________________________________________________________________________________________________________

    def test_breakpoint():
>      breakpoint()

test/test_breakpoint.py:2:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
../pytest/_pytest/debugging.py:78: in set_trace
    cls._pdb_cls().set_trace(frame)
env/lib/python3.7/site-packages/IPython/terminal/debugger.py:26: in __init__
    self.pt_init()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <IPython.terminal.debugger.TerminalPdb object at 0x10629ab70>

    def pt_init(self):
        def get_prompt_tokens(cli):
            return [(Token.Prompt, self.prompt)]

        def patch_stdout(**kwargs):
            return self.pt_cli.patch_stdout_context(**kwargs)

        if self._ptcomp is None:
            compl = IPCompleter(shell=self.shell,
                                        namespace={},
                                        global_namespace={},
                                        parent=self.shell,
                                       )
            self._ptcomp = IPythonPTCompleter(compl, patch_stdout=patch_stdout)

        kbmanager = KeyBindingManager.for_prompt()
        supports_suspend = Condition(lambda cli: hasattr(signal, 'SIGTSTP'))
        kbmanager.registry.add_binding(Keys.ControlZ, filter=supports_suspend
                                      )(suspend_to_bg)

        if self.shell.display_completions == 'readlinelike':
            kbmanager.registry.add_binding(Keys.ControlI,
                                 filter=(HasFocus(DEFAULT_BUFFER)
                                         & ~HasSelection()
                                         & ViInsertMode() | EmacsInsertMode()
                                         & ~cursor_in_leading_ws
                                         ))(display_completions_like_readline)
        multicolumn = (self.shell.display_completions == 'multicolumn')

        self._pt_app = create_prompt_application(
                            editing_mode=getattr(EditingMode, self.shell.editing_mode.upper()),
                            key_bindings_registry=kbmanager.registry,
                            history=self.shell.debugger_history,
                            completer= self._ptcomp,
                            enable_history_search=True,
                            mouse_support=self.shell.mouse_support,
                            get_prompt_tokens=get_prompt_tokens,
                            display_completions_in_columns=multicolumn,
>                           style=self.shell.style
        )
E       AttributeError: 'TerminalInteractiveShell' object has no attribute 'style'

env/lib/python3.7/site-packages/IPython/terminal/debugger.py:66: AttributeError
========================================================================================================= 1 failed in 0.29 seconds =========================================================================================================

tonybaloney · 2018-03-23T04:52:43Z

@nicoddemus @RonnyPfannschmidt done (except docs)

tonybaloney · 2018-03-23T05:17:45Z

Build failed on 1 test

https://travis-ci.org/pytest-dev/pytest/jobs/357232290#L585

I asserted the output was "1 passed" but it is "2 passed" on Travis. Might have something to do with Hypothesis, trying to repro locally, but I can make the assertion a bit more vauge

tonybaloney · 2018-03-23T05:21:54Z

Yep. once I install hypothesis, it changes to 2 passed locally as well (which I guess makes sense as the hypothesis plugin is designed to do that)

…veloping in pytest)

nicoddemus

Awesome work @tonybaloney, many thanks. Please take a look at my comments.

nicoddemus · 2018-03-26T22:36:35Z

_pytest/debugging.py

@@ -27,12 +34,20 @@ def pytest_configure(config):
    if config.getvalue("usepdb"):
        config.pluginmanager.register(PdbInvoke(), 'pdbinvoke')

+    # Use custom Pdb class set_trace instead of default Pdb on breakpoint() call
+    if SUPPORTS_BREAKPOINT_BUILTIN:
+        _environ_pythonbreakpoint = getattr(os.environ, 'PYTHONBREAKPOINT', '')


This seems like it should be _environ_pythonbreakpoint = os.environ.get('PYTHONBREAKPOINT', '') right?

Fixed in f1f4c8c

nicoddemus · 2018-03-26T22:42:22Z

changelog/3180.feature.rst

@@ -0,0 +1,12 @@
+Support for Python 3.7s builtin breakpoint() method.


Thanks for the detailed changelog entry, but unfortunately towncrier can't format multi-paragraph entries correctly in the CHANGELOG.

I suggest adding this to a new section after pdb section in the docs, and reducing the changelog entry to:

Support for Python 3.7s builtin ``breakpoint()`` method, see `the debugging documentation <link here>`_ for details.

The new section could be named something like Support for breakpoint() support and provide a link to the relevant PEP as well which would act as an introduction to the topic as well.

nicoddemus · 2018-03-26T22:46:10Z

testing/test_pdb.py

+            called.append("set_trace")
+
+    _pytest._CustomDebugger = _CustomDebugger
+    return called


It probably doesn't matter, but I think it is better to get rid of the reference to the class after the test:

_pytest._CustomDebugger = _CustomDebugger yield called del _pytest._CustomDebugger

Done in f1f4c8c

nicoddemus · 2018-03-26T22:57:37Z

testing/test_pdb.py

+        """
+        config = testdir.parseconfig()
+
+        pytest_configure(config)


This is clever, but it might be dangerous to call pytest_configure by hand like this. How about:

testdir.makeconftest(""" from pytest import hookimpl from _pytest.debugging import pytestPDB @hookimpl(hookwrapper=True) def pytest_configure(config): yield assert sys.breakpointhook is pytestPDB.set_trace @hookimpl(hookwrapper=True) def pytest_unconfigure(config): yield assert sys.breakpointhook is sys.__breakpoint__ """) testdir.makepyfile(""" def test_nothing(): pass """)

I couldn't locate the right test pattern looking at the others. This makes sense

Fixed in f1f4c8c

nicoddemus · 2018-03-26T23:00:57Z

testing/test_pdb.py

+        assert sys.breakpointhook != pytestPDB.set_trace
+
+    @pytest.mark.skipif(not SUPPORTS_BREAKPOINT_BUILTIN, reason="Requires breakpoint() builtin")
+    def test_sys_breakpointhook_configure_and_unconfigure_with_pdb_flag(self, testdir):


You can probably parametrize this test with the above.

@pytest.mark.skipif(not SUPPORTS_BREAKPOINT_BUILTIN, reason="Requires breakpoint() builtin") @pytest.mark.parametrize('args', [('--pdb',), ()]) def test_sys_breakpointhook_configure_and_unconfigure(self, testdir, args): ... result = testdir.runpytest_inprocess(*args)

(It's not necessary to pass p1 to runpytest_inprocess).

Fixed in f1f4c8c

nicoddemus · 2018-03-26T23:03:30Z

testing/test_pdb.py

+    def test_pdb_not_altered(self, testdir):
+        """
+        Test that calling PDB set_trace() is not affected
+        when PYTHONBREAKPOINT=0


Is this comment correct? I don't see PYTHONBREAKPOINT being set in the test..

Removed in f1f4c8c

nicoddemus · 2018-03-26T23:04:12Z

testing/test_pdb.py

 import pytest


+_ENVIRON_PYTHONBREAKPOINT = getattr(os.environ, 'PYTHONBREAKPOINT', '')


Again this seems like it should be `_ENVIRON_PYTHONBREAKPOINT = os.environ.get('PYTHONBREAKPOINT', '')

Fixed in f1f4c8c

tonybaloney · 2018-03-27T10:27:30Z

@nicoddemus made the code changes. Docs still need improving, but explain the behaviour for now. I can add more examples later when I've got some more time

nicoddemus · 2018-03-27T10:42:05Z

Hi @tonybaloney thanks for the quick response! I made some doc formatting changes (all minor), hope you don't mind.

I worry though that the os.environ mistake was not caught by any tests. Can you design a test that fails with the incorrect code?

Our rst-linter unfortunately doesn't accept `ref` directives in the CHANGELOG files

tonybaloney · 2018-03-27T21:57:21Z

@nicoddemus not sure I really understood this properly. It doesn't seem to execute. If I change the assertions it doesnt fail the tests
f1f4c8c#diff-6244362351d7c228d95cde366267b10aR511

nicoddemus · 2018-03-28T00:06:25Z

@tonybaloney Oh my bad, my example was incomplete:

...
testdir.makepyfile("""
    def test_nothing(): pass
""")
result = testdir.runpytest()
result.stdout.fnmatch_lines([
    '*1 passed in *',
])

IOW, testdir lets you create test files, execute pytest and check the output. I forgot the last bit, sorry.

tonybaloney · 2018-03-28T22:05:33Z

@nicoddemus that makes sense, but all 3 fail now

  File "/Users/anthonyshaw/repo/pytest/env/lib/python3.7/site-packages/pluggy-0.6.0-py3.7.egg/pluggy/__init__.py", line 253, in register
    self._verify_hook(hook, hookimpl)
  File "/Users/anthonyshaw/repo/pytest/env/lib/python3.7/site-packages/pluggy-0.6.0-py3.7.egg/pluggy/__init__.py", line 361, in _verify_hook
    (hookimpl.plugin_name, hook.name))
pluggy.PluginValidationError: Plugin '/private/var/folders/5_/ljj1v1m91hl_2r_9t_00bhmc0000gn/T/pytest-of-anthonyshaw/pytest-67/test_sys_breakpointhook_configure_and_unconfigure0/conftest.py'
hook 'pytest_configure'
historic incompatible to hookwrapper

I looked at the documentation and https://docs.pytest.org/en/latest/reference.html#_pytest.hookspec.pytest_configure that hook doesn't support hookwrapper=True.

I tried taking that out and just get another error

>       self._match_lines(lines2, fnmatch, 'fnmatch')
E       Failed: nomatch: '*1 passed in *'
E           and: 'INTERNALERROR> Traceback (most recent call last):'
E           and: 'INTERNALERROR>   File "/Users/anthonyshaw/repo/pytest/_pytest/main.py", line 96, in wrap_session'
E           and: 'INTERNALERROR>     config._do_configure()'
E           and: 'INTERNALERROR>   File "/Users/anthonyshaw/repo/pytest/_pytest/config.py", line 922, in _do_configure'
E           and: 'INTERNALERROR>     self.hook.pytest_configure.call_historic(kwargs=dict(config=self))'
E           and: 'INTERNALERROR>   File "/Users/anthonyshaw/repo/pytest/env/lib/python3.7/site-packages/pluggy-0.6.0-py3.7.egg/pluggy/__init__.py", line 630, in call_historic'
E           and: 'INTERNALERROR>     proc(x)'
E           and: "INTERNALERROR> TypeError: 'NoneType' object is not callable"
E           and: ''
E       remains unmatched: '*1 passed in *'

/Users/anthonyshaw/repo/pytest/_pytest/pytester.py:1175: Failed

* Execute pytest in a subprocess in cases of tests which change global state * Parametrize with --pdb and without the flag

nicoddemus · 2018-03-28T23:23:54Z

testing/test_pdb.py

        """
-        assert sys.breakpointhook != pytestPDB.set_trace
+        assert sys.breakpointhook == pytestPDB.set_trace


This test seemed incorrect to me and it was failing on my system: wasn't sys.breakpoint supposed to be set in the normal circumstance? I changed the logic and docstring to reflect that and it passes. @tonybaloney please double check here.

nicoddemus · 2018-03-28T23:24:52Z

@tonybaloney sorry my bad, I forgot pytest_configure cannot be hook-wrappers. I pushed some changes to fix the tests:

Execute some of them in a separate process, so changes to global state doesn't affect the rest of the tests.
I had to use the internal config._cleanup mechanism to ensure the hook is restored at the end of the session.
Fixed the parametrization in two of the tests.
I had to change the logic of one of the tests, see 804392e#r177918076.

…self

tonybaloney · 2018-04-03T00:42:55Z

@nicoddemus I figured out why that last test was failing, because the test is being run inside a pytest session, the new behaviour would mean that sys.breakpointhook is going to be unpredictable depending on the version of pytest being used and it doesn't test anything other than runtime assumptions.
So I removed it.

I'm all done now :)

nicoddemus

Thanks a lot @tonybaloney!

I would like a second set of eyes to take a look before merging though, do you have some time to review this @RonnyPfannschmidt?

asottile

looks great 👍

asottile · 2018-04-10T01:50:49Z

testing/test_pdb.py

+        Test that supports breakpoint global marks on Python 3.7+ and not on
+        CPython 3.5, 2.7
+        """
+        if sys.version_info.major == 3 and sys.version_info.minor >= 7:


could use sys.version_info >= (3, 7) as a slightly more readable alternative.

asottile · 2018-04-10T01:51:06Z

AUTHORS

@@ -17,6 +17,7 @@ Andreas Zeidler
 Andrzej Ostrowski
 Andy Freeland
 Anthon van der Neut
+Anthony Shaw
 Anthony Sottile


well hello fellow anthony :D

RonnyPfannschmidt · 2018-04-10T05:15:32Z

well done, im happy to merge 👍

tonybaloney added 3 commits March 22, 2018 17:25

start acceptance testing

9edcb7e

add a module global for whether the current runtime supports the buil…

3bca983

…tin breakpoint function

assert that custom PDB class is used as breakpoint hook where supported

91d99af

tonybaloney changed the base branch from master to features March 22, 2018 08:48

tonybaloney added 7 commits March 22, 2018 20:40

move tests to test_pdb

5a53b9a

"Added acceptance tests for configuration of sys.breakpointhook and r…

a1ff758

…esetting back when system default (pdb) is used""

add check for support of breakpoint() and use Custom Pdb class when s…

21ada0f

…ystem default is set

conditional for resetting of sys.breakpointhook for cleanup where bre…

0e83e4f

…akpoint() not supported

add tests to validate that --pdbcls custom debugger classes will be c…

db581ea

…alled when breakpoint() is used

add changelog entry

dcbba38

add myself to authors

1ec9913

tonybaloney changed the title ~~[WIP] Breakpoint support~~ Support for the new builtin breakpoint method Mar 23, 2018

tonybaloney changed the title ~~Support for the new builtin breakpoint method~~ Support for the new builtin breakpoint function in Python 3.7 Mar 23, 2018

linting and removed double test

242fb78

fix assertion when hypothesis is installed (which is should be for de…

e97bd87

…veloping in pytest)

nicoddemus requested changes Mar 26, 2018

View reviewed changes

tonybaloney added 3 commits March 27, 2018 17:38

updates for code review recommendations

f1f4c8c

update documentation for new feature

671ab5a

fix syntax

b45006e

Improve docs formatting

0a4200b

Use full link in CHANGELOG

060f047

Our rst-linter unfortunately doesn't accept `ref` directives in the CHANGELOG files

add test for custom environment variable

3998b70

add broken test

09e5a22

Fix tests that check that breakpoint function is configured/restored

804392e

* Execute pytest in a subprocess in cases of tests which change global state * Parametrize with --pdb and without the flag

nicoddemus reviewed Mar 28, 2018

View reviewed changes

remove a test that would fail because pytest is being used to test it…

4d84759

…self

Remove unused pytestPDB import

0762666

nicoddemus approved these changes Apr 3, 2018

View reviewed changes

asottile approved these changes Apr 10, 2018

View reviewed changes

RonnyPfannschmidt approved these changes Apr 10, 2018

View reviewed changes

RonnyPfannschmidt merged commit 2241c98 into pytest-dev:features Apr 10, 2018

asottile mentioned this pull request Sep 24, 2018

breakpoint() support #4027

Closed

		@@ -0,0 +1,12 @@
		Support for Python 3.7s builtin breakpoint() method.

		import pytest


		_ENVIRON_PYTHONBREAKPOINT = getattr(os.environ, 'PYTHONBREAKPOINT', '')

Support for the new builtin breakpoint function in Python 3.7 #3331

Support for the new builtin breakpoint function in Python 3.7 #3331

Conversation

tonybaloney commented Mar 22, 2018 • edited Loading

coveralls commented Mar 22, 2018 • edited Loading

tonybaloney commented Mar 22, 2018

RonnyPfannschmidt commented Mar 22, 2018

tonybaloney commented Mar 22, 2018

RonnyPfannschmidt commented Mar 22, 2018

tonybaloney commented Mar 23, 2018

tonybaloney commented Mar 23, 2018

tonybaloney commented Mar 23, 2018

tonybaloney commented Mar 23, 2018

tonybaloney commented Mar 23, 2018

nicoddemus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonybaloney commented Mar 27, 2018

nicoddemus commented Mar 27, 2018

tonybaloney commented Mar 27, 2018

nicoddemus commented Mar 28, 2018 • edited Loading

tonybaloney commented Mar 28, 2018

Choose a reason for hiding this comment

nicoddemus commented Mar 28, 2018

tonybaloney commented Apr 3, 2018

nicoddemus left a comment

Choose a reason for hiding this comment

asottile left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RonnyPfannschmidt commented Apr 10, 2018

tonybaloney commented Mar 22, 2018 •

edited

Loading

coveralls commented Mar 22, 2018 •

edited

Loading

nicoddemus commented Mar 28, 2018 •

edited

Loading