Skip to content
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

3610 add trace option #3647

Merged
merged 18 commits into from Jul 7, 2018
Merged

Conversation

@jeffreyrack
Copy link
Member

jeffreyrack commented Jul 2, 2018

Closes #3610 by adding the --trace option to pytest.

This option can be used to enter the debugger at the start of every test being ran.

Example usage:

C:\Users\User\Desktop\pytest>python -m pytest test.py --trace
c:\Users\User\AppData\Local\Programs\Python\Python36\lib\site-packages\pytest-3.
6.3.dev89+g57198d47.d20180701-py3.6.egg\_pytest\assertion\rewrite.py:250: UserWa
rning: Module _pytest was already imported from c:\Users\User\AppData\Local\Prog
rams\Python\Python36\lib\site-packages\pytest-3.6.3.dev89+g57198d47.d20180701-py
3.6.egg\_pytest\__init__.py, but c:\users\user\desktop\pytest is being added to
sys.path
  import pkg_resources
c:\Users\User\AppData\Local\Programs\Python\Python36\lib\site-packages\pytest-3.
6.3.dev89+g57198d47.d20180701-py3.6.egg\_pytest\assertion\rewrite.py:250: UserWa
rning: Module pytest was already imported from c:\Users\User\AppData\Local\Progr
ams\Python\Python36\lib\site-packages\pytest-3.6.3.dev89+g57198d47.d20180701-py3
.6.egg\pytest.py, but c:\users\user\desktop\pytest is being added to sys.path
  import pkg_resources
============================= test session starts =============================
platform win32 -- Python 3.6.5, pytest-3.6.3.dev89+g57198d47.d20180701, py-1.5.3
, pluggy-0.6.0
rootdir: C:\Users\User\Desktop\pytest, inifile: tox.ini
plugins: hypothesis-3.65.0
collected 1 item

test.py
>>>>>>>>>>>>>>>>>>> PDB set_trace (IO-capturing turned off) >>>>>>>>>>>>>>>>>>>
> c:\users\user\desktop\pytest\test.py(4)test_foo()
-> assert True
(Pdb) c
.                                                                [100%]

========================== 1 passed in 0.58 seconds ===========================

C:\Users\User\Desktop\pytest>

While implementing this PR, there were a couple of things I wasn't sure how to best handle within the pytest codebase:

How to access a config value from within the pytest_pyfunc_call hook (Right now using a global variable, likely not the ideal way to implement).

Does it make sense to add the set_break option to PytestPdb.set_trace()? This doesn't seem like the right spot for it, and I'm instead thinking of moving the other logic out into another function which PytestPdb.set_trace calls before actually breaking.

@jeffreyrack jeffreyrack requested a review from nicoddemus Jul 2, 2018
jeffreyrack added 2 commits Jul 2, 2018
@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 2, 2018

Coverage Status

Coverage decreased (-0.1%) to 92.504% when pulling 067de25 on jeffreyrack:3610-add-trace-option into 8680dfc on pytest-dev:features.

@@ -38,6 +49,8 @@ def pytest_configure(config):
else:
pdb_cls = pdb.Pdb

global immediately_break
immediately_break = config.getvalue("trace")

This comment has been minimized.

Copy link
@RonnyPfannschmidt

RonnyPfannschmidt Jul 2, 2018

Member

this should be a option to the pdbinvoke plugin, which then could handle the pyfunc_call

if pyfuncitem._isyieldedfunction():
pyfuncitem.args = [testfunction, pyfuncitem._args]
else:
if "func" in pyfuncitem._fixtureinfo.argnames:

This comment has been minimized.

Copy link
@RonnyPfannschmidt

RonnyPfannschmidt Jul 2, 2018

Member

this is already availiable in request - the fixture should be named pytest_test_function and be availiable in any case

jeffreyrack added 7 commits Jul 3, 2018
@RonnyPfannschmidt

This comment has been minimized.

Copy link
Member

RonnyPfannschmidt commented Jul 3, 2018

a followup could be some docs

@nicoddemus

This comment has been minimized.

Copy link
Member

nicoddemus commented Jul 4, 2018

Hi @jeffreyrack, thanks a lot for the PR.

The option works well, each test starts tracing into PDB as advertised. But how do I quit the debugger?

  • c(continue) would continue to the next test, and stop there again. The same with quit and exit.
  • CTRL+C raises KeyboardInterrupt, but that is caught by our Pdb code (I think?) and ignored.

I managed to quit by killing the terminal, but that doesn't seem like a good UI. Or perhaps I am missing something?

@jeffreyrack

This comment has been minimized.

Copy link
Member Author

jeffreyrack commented Jul 4, 2018

@nicoddemus Quit/exit will quit the debugger, which will then get started again at the start of the next test.

I believe that this is consistent with the --pdb option, where quitting the debugger will continue running the tests until the next time error that causes the debugger to be entered again.

Do you have a recommendation on what might be a better a better UI that allows the user to exit entirely?

@nicoddemus

This comment has been minimized.

Copy link
Member

nicoddemus commented Jul 4, 2018

@jeffreyrack,

I believe that this is consistent with the --pdb option, where quitting the debugger will continue running the tests until the next time error that causes the debugger to be entered again.

Oh, I agree with you that it is consistent. And the feature does seem useful, my only concern is that might get some users by surprise because there's no obvious way to "stop" debugging (it certainly does not feel friendly when you run that in a file with a lot of tests, I tested with pytest --trace testing/test_mark.py).

Do you have a recommendation on what might be a better a better UI that allows the user to exit entirely?

Some ideas, just throwing them around:

  • Perhaps we can catch KeyboardInterrupt and in that case disable the --trace option? (related to #3436).

  • If we don't find a solution to stop running the tests, can we display a message on how to quit the debugging session? Something like ">>> --trace enabled, all tests will stop at the debugger. To quit, kill the terminal with CTRL + break." in bright red. Just so users don't feel trapped, like the vim joke goes.

All in all this seems like a really nice feature, just trying to make it a little more user friendly because that was my first impression when trying it out (or again I'm missing something more obvious than killing the terminal).

@nicoddemus

This comment has been minimized.

Copy link
Member

nicoddemus commented Jul 4, 2018

(cc @brianmaissy which also contributed to --pdb recently)

@asottile

This comment has been minimized.

Copy link
Member

asottile commented Jul 5, 2018

re: hard to kill tests -- I've noticed the same thing when running with --pdb which I myself find pretty frustrating :) (usually end up resorting to ^\)

The other thing is pdb catches KeyboardInterrupt and SystemExit so it's not trivial to get out or even communicate a way out (maybe by modifying how pdb works to not do this?)

@nicoddemus

This comment has been minimized.

Copy link
Member

nicoddemus commented Jul 7, 2018

FWIW uf we can't find a nice solution for now, I'm fine with merging this as is; the feature feels solid, and we can always improve it later.

@asottile

This comment has been minimized.

Copy link
Member

asottile commented Jul 7, 2018

same here, all for merging this even as-is 👍

@nicoddemus nicoddemus merged commit 303133f into pytest-dev:features Jul 7, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nicoddemus

This comment has been minimized.

Copy link
Member

nicoddemus commented Jul 7, 2018

Merged then! Thanks a lot @jeffreyrack! 😁

@jeffreyrack jeffreyrack deleted the jeffreyrack:3610-add-trace-option branch Jul 8, 2018
@blueyed

This comment has been minimized.

Copy link
Contributor

blueyed commented Oct 30, 2019

Was it considered to use functools.partial here?
This allows for passing in the func as an arg, not kwarg. See #6099 (comment).

@RonnyPfannschmidt

This comment has been minimized.

Copy link
Member

RonnyPfannschmidt commented Oct 30, 2019

that would require a extension of the calling mechanism, which is likely to generate way more pain (imho)

@blueyed

This comment has been minimized.

Copy link
Contributor

blueyed commented Oct 30, 2019

@RonnyPfannschmidt ?
This was creating a reference to #6099, where I've used functools, and you approved it (the code at least). So what do you mean?

@RonnyPfannschmidt

This comment has been minimized.

Copy link
Member

RonnyPfannschmidt commented Oct 30, 2019

@blueyed i did read this one before the actual solution and misunderstood the question

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.