pdb: add option to skip `pdb.set_trace()` by blueyed · Pull Request #4854 · pytest-dev/pytest

blueyed · 2019-02-28T13:19:43Z

TODO:

doc ?
better name(s) ?

blueyed · 2019-02-28T13:20:40Z

It is meant ease running tests, where the code is (full of) __import__("pdb").set_trace().

codecov · 2019-02-28T17:21:03Z

Codecov Report

Merging #4854 into features will increase coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##           features    #4854      +/-   ##
============================================
+ Coverage     96.05%   96.06%   +<.01%     
============================================
  Files           114      114              
  Lines         25737    25747      +10     
  Branches       2547     2549       +2     
============================================
+ Hits          24722    24734      +12     
+ Misses          704      703       -1     
+ Partials        311      310       -1

Impacted Files	Coverage Δ
testing/test_pdb.py	`99.18% <100%> (+0.01%)`	⬆️
src/_pytest/debugging.py	`89.07% <100%> (+0.24%)`	⬆️
testing/acceptance_test.py	`98.03% <0%> (+0.39%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b5cddc...adebfd0. Read the comment docs.

jeffreyrack

Would it make sense to wrap pdb.set_trace() when the option isn't used to help users know about the option?

Something like:

example_test.py
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> PDB set_trace (IO-capturing turned off) >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
To disable break points set via pdb, use --pdb-skip.
> /home/vagrant/pytest-source/example_test.py(7)test_a()
-> a = 'b'
(Pdb)

blueyed · 2019-03-01T22:36:35Z

What about the option name(s)?

I've used/added a longer run for clarify, but the typical use would rather be to append -skip to an existing --pdb (the short version).
Since there is --pdbcls it should maybe be --pdbskip instead?!

nicoddemus · 2019-03-01T22:52:05Z

Since there is --pdbcls it should maybe be --pdbskip instead?!

Personally I prefer --pdb-skip, but I'm fine either way. 👍

blueyed · 2019-03-02T00:25:53Z

@jeffreyrack

To disable break points set via pdb, use --pdb-skip.

Not a bad idea - but in general it just adds noise - typically you would a set_trace to trigger.

I consider this new feature rather as a gem for advanced users, which are also messy by needing it in the first place.. ;)

Similar to how e.g. pdbpp also has an edit command to edit the source in-place, but does not add a note about it when entering interactive mode - but maybe it could/should? :p

blueyed · 2019-03-02T15:43:28Z

@nicoddemus
I've added a method to pass stdin to MultiCapture, which can be used to interact with pdb in tests.
What do you think?

nicoddemus · 2019-03-03T12:54:48Z

I've added a method to pass stdin to MultiCapture, which can be used to interact with pdb in tests.

The code looks good, but I'm not sure I understand the user case, can you please elaborate?

blueyed · 2019-03-03T13:34:34Z

The code looks good, but I'm not sure I understand the user case, can you please elaborate?

It allows to use inprocess pytest instead of pexpect.
Extended the example a bit.

One downside currently is that you do not get the "stdin gets echoed to stdout" that terminals normally do.

blueyed · 2019-03-04T18:30:53Z

Waiting for coverage improvements through master to rebase this against.

asottile · 2019-03-05T22:29:55Z

Would it be better to not implement this and wait for PYTHONBREAKPOINT=0 to become ubiquitous

blueyed · 2019-03-05T22:34:01Z

Would it be better to not implement this and wait for PYTHONBREAKPOINT=0 to become ubiquitous

Oh, wasn't aware of it.

Should we just respect / look at it here instead?

asottile · 2019-03-05T22:38:13Z

Would it be better to not implement this and wait for PYTHONBREAKPOINT=0 to become ubiquitous

Oh, wasn't aware of it.

Should we just respect / look at it here instead?

👍 sg2m

blueyed · 2019-03-05T22:56:34Z

Well, almost finished it - but then thought that having to set PYTHONBREAKPOINT=0 is much more involving than just appending --pdb-skip (which also can be completed through your shell).
I therefore think we should have both.

asottile · 2019-03-05T22:59:24Z

My leaning is that pytest shouldn't reimplement stuff that's standard python (then you have to learn two sets of tools), curious to hear what the others have to say though

blueyed · 2019-03-05T23:09:49Z

Well, PYTHONBREAKPOINT is only supported with breakpoint() - given that we should probably not look at it at all with pdb.set_trace.

asottile · 2019-03-05T23:13:35Z

That's a good point, which makes this feature seem even less of a good idea since it won't handle other debuggers like pudb / ipdb / the dozen others

blueyed · 2019-03-05T23:20:08Z

Yes, it is specific to pdb, but that's implied through the name already.
pytest only monkeypatches pdb.set_trace, not more. Since I am using pdbpp this is fine, since it monkeypatches pdb itself, but AFAIK there are plugins for ipdb etc to make pytest work with them. That sounds like a situation to be improved in general though (not related to this PR currently).

asottile · 2019-03-05T23:24:55Z

My hope for the debugging plugins was that as we eventually drop <3.7 we can just use breakpoint and not need all these options and plugins

blueyed · 2019-03-17T16:38:14Z

Will move the stdin improvements (e2cd884) to #4937 / a new PR.

blueyed · 2019-04-02T12:12:23Z

Just found that --pdbcls unittest.mock:Mock can be used already for skipping set_trace, except for that it would still print "PDB set_trace (IO-capturing turned off)", and obviously also breaks --pdb.
But maybe proving a better no-op-for-set_trace class is a good idea?

Otherwise I think this is good for now, but could only need some addition to the docs maybe?

asottile · 2019-04-03T20:30:29Z

:S

blueyed · 2019-04-03T20:44:25Z

?

asottile · 2019-04-03T20:46:03Z

My impression was we weren't going to merge this as is given these comments:

blueyed · 2019-04-03T21:27:30Z

@asottile
I see - but this is another case for a review that has been approved (hit my search for that as a single result), and then getting not feedback via my last comments.
I've came back to this due to having a need for it mysel, but hoped for "hopefully we're not having to deprecate anything later" when merging it.

blueyed · 2019-04-03T21:31:03Z

As for looking at PYTHONBREAKPOINT: I think that this is not really that much relevant here, since this is about skipping pdb.set_trace() alone, but certainly something good to consider in a future PR - I'd be happy to review something in this regard from you, after all I've noticed you getting the improved "Quitting debugger" outcome during the screencast that I've watched.. :)

asottile · 2019-04-03T22:37:58Z

@asottile
I see - but this is another case for a review that has been approved (hit my search for that as a single result), and then getting not feedback via my last comments.
I've came back to this due to having a need for it mysel, but hoped for "hopefully we're not having to deprecate anything later" when merging it.

heh well I didn't think I had to block it because it was [WIP] when I looked at it -- usually when moving something from WIP -> not WIP you get another review

blueyed · 2019-04-03T22:39:16Z

usually when moving something from WIP -> not WIP you get another review

Not here.. WIP is just a silly blocking status.

asottile · 2019-04-03T22:42:38Z

after all I've noticed you getting the improved "Quitting debugger" outcome during the screencast that I've watched.. :)

have greatly appreciated this btw 👍 I used to ^\ pytest but not any more!

blueyed · 2019-04-04T11:35:54Z

@asottile

I used to ^\ pytest but not any more!

Interesting - did not know that exists..

This reverts commit e88aa95, reversing changes made to 1410d3d. I do not think it warrants an option anymore, and there is a way to achieve this via `--pdbcls` if needed.

blueyed · 2019-04-27T00:27:49Z

Reverting it in #5173.

Revert "Merge pull request #4854 from blueyed/pdb-skip"

blueyed added type: enhancement new feature or API change, should be merged into features branch plugin: debugging related to the debugging builtin plugin labels Feb 28, 2019

blueyed force-pushed the pdb-skip branch from 80865da to 9e47186 Compare February 28, 2019 17:19

blueyed requested a review from jeffreyrack March 1, 2019 08:26

blueyed changed the title ~~pdb: add option to skip pdb.set_trace()~~ [RFC] pdb: add option to skip pdb.set_trace() Mar 1, 2019

jeffreyrack reviewed Mar 1, 2019

View reviewed changes

Comment thread src/_pytest/debugging.py

blueyed force-pushed the pdb-skip branch from 9e47186 to 8f6beaf Compare March 2, 2019 15:42

nicoddemus approved these changes Mar 3, 2019

View reviewed changes

blueyed changed the title ~~[RFC] pdb: add option to skip pdb.set_trace()~~ [WIP] pdb: add option to skip pdb.set_trace() Mar 3, 2019

blueyed force-pushed the pdb-skip branch from fd1e7ac to 7b4a927 Compare March 5, 2019 22:06

blueyed force-pushed the pdb-skip branch from 7b4a927 to a600ce8 Compare March 5, 2019 23:00

pdb: add option to skip pdb.set_trace()

adebfd0

blueyed force-pushed the pdb-skip branch from a600ce8 to adebfd0 Compare March 29, 2019 10:13

blueyed changed the title ~~[WIP] pdb: add option to skip pdb.set_trace()~~ pdb: add option to skip pdb.set_trace() Apr 2, 2019

blueyed merged commit e88aa95 into pytest-dev:features Apr 3, 2019

blueyed deleted the pdb-skip branch April 3, 2019 20:25

asottile added a commit that referenced this pull request Apr 27, 2019

Merge pull request #5173 from blueyed/revert

ac7ee40

Revert "Merge pull request #4854 from blueyed/pdb-skip"

Uh oh!

Conversation

blueyed commented Feb 28, 2019

Uh oh!

blueyed commented Feb 28, 2019

Uh oh!

codecov Bot commented Feb 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jeffreyrack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

blueyed commented Mar 1, 2019

Uh oh!

nicoddemus commented Mar 1, 2019

Uh oh!

blueyed commented Mar 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blueyed commented Mar 2, 2019

Uh oh!

nicoddemus commented Mar 3, 2019

Uh oh!

blueyed commented Mar 3, 2019

Uh oh!

blueyed commented Mar 4, 2019

Uh oh!

asottile commented Mar 5, 2019

Uh oh!

blueyed commented Mar 5, 2019

Uh oh!

asottile commented Mar 5, 2019

Uh oh!

blueyed commented Mar 5, 2019

Uh oh!

asottile commented Mar 5, 2019

Uh oh!

blueyed commented Mar 5, 2019

Uh oh!

asottile commented Mar 5, 2019

Uh oh!

blueyed commented Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asottile commented Mar 5, 2019

Uh oh!

blueyed commented Mar 17, 2019

Uh oh!

blueyed commented Apr 2, 2019

Uh oh!

asottile commented Apr 3, 2019

Uh oh!

blueyed commented Apr 3, 2019

Uh oh!

asottile commented Apr 3, 2019

Uh oh!

blueyed commented Apr 3, 2019

Uh oh!

blueyed commented Apr 3, 2019

Uh oh!

asottile commented Apr 3, 2019

Uh oh!

blueyed commented Apr 3, 2019

Uh oh!

asottile commented Apr 3, 2019

Uh oh!

blueyed commented Apr 4, 2019

Uh oh!

blueyed commented Apr 27, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Feb 28, 2019 •

edited

Loading

blueyed commented Mar 2, 2019 •

edited

Loading

blueyed commented Mar 5, 2019 •

edited

Loading