-
Notifications
You must be signed in to change notification settings - Fork 66
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
Disable timing-out when a debugging session is in progress #62
Conversation
Thanks for the PR! |
Sure.
As far as I can tell, this would only effect things being debugged. Or if
Initially, I thought it was the case that
Maybe, but I'm not quite sure how we'd test this. Maybe just calling a test with ridiculously small timeout that sleeps for 1s or whatever and assert that it does in fact take 1s. |
Sorry about all the formatting commits, couldn't get |
I still find your suggested heuristic pretty scary to use, it feels super ad-hoc. Would this not work if you check But I'd also like to get back to the other point that perhaps the runner should be calling the |
You're right, and there's a performance hit too — around 50ms for my machine. I read over PEP553 and the PyDevD source code. to see if there was a better way to do this. I initially just threw this together thinking it would be the best way to do it based on . . . another source 😉 . My new approach should target all debuggers across 2.7-3.8 as long as they use a similar approach to PyDevD (specifically, utilizing My strategy is to check
and see if the module the function belongs to is in the std lib — if not, I assume that it has been hooked and thus a debugging session is active. This would almost makes sense to check at startup, however that wouldn't cover the case of attaching to a process. As for performance — the cost is around 2us/call on my machine. WRT to using |
There is however a difference between a breakpoint hook being installed and actually being in an active debugging session isn't there? I think it's possible for someone having installed a breakpoint hook but not be in a debugging session. Or only having a breakpoint in the second test and a timeout occurring in the first test. |
another reasonable way would be to sniff sys.gettrace() and sys.breakpointhook() to find out which debugger might be used. then try an api from the detected debugger to ask it whether it's currently in an active debugger session. This would again need more coordination with the debugger however. But I somehow suspect that any reliable solution is going to involve pytest(-timeout) and the debugger(s) to agree on some api they can use from each other. If we could do something like |
Technically yes, but practically I can't think of that scenario occurring.
This is a side-effect that I'm willing to live with and had planned for. I think it's reasonable for a few reasons. One reason is that when you are debugging, chances are you are only looking a very small subset of tests, usually only one (based on my experience only). Another is that the alternative — allowing the one test to timeout — won't always continue to the second test (as noted in the readme):
Finally, that last reason is my implementation would match the Uggh, only read your first comment before writing all that. WRT to using
I tested measuring coverage as well as profiling a test test and found that no trace was set by either, but even so I've pushed a whitelist based approach of inspecting |
# Conflicts: # pytest_timeout.py
Thanks for your patience. I'm starting to like this implementation more, so that's good :) I think for testing strategy it seems pydevd has a That's just a first brainstorm at how to go about this. I'm looking to test two things here: 1. The detection works and the timeout does get cancelled. 2. The whitelist keeps being correct. |
Wouldn't |
Mostly yes, but I added that because I think it's important to test against the latest released version (probably in tox) |
Is ipdb.set_trace() also handled ? Since this also doesn't work in v1.3.4. |
@sar772004 Can you let me know if that works? |
|
Alright, so here's the state of testing. It's easy enough to test ipdbRequires the testing test session disabled input capturing, but this isn't currently possible. I've submitted this PR to try and allow for this: pytest-dev/pytest#7207 pydevdAfter jumping around the source code of I might take a look into ptvsd which might be a wrapper around PyDev.Debugger that may be able to tame it's power. |
test_pytest_timeout.py
Outdated
try: | ||
import ipbd | ||
except ImportError: | ||
ipbd = None |
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.
You've typoed ipdb here. To be fair you changed the test to no longer skip if it's not there so may as well import directly, we do this for pexpect as well and it's not that a huge dependency.
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.
Yep, was focused on that one test. Will remove this.
test_pytest_timeout.py
Outdated
@@ -13,6 +18,7 @@ | |||
have_spawn = pytest.mark.skipif( | |||
not hasattr(pexpect, "spawn"), reason="pexpect does not have spawn" | |||
) | |||
have_ipdb = pytest.mark.skipif(ipbd is None, reason="ipdb is not installed") |
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.
No longer used, which as I say above I think is fine.
btw, after fixing the ipdb imports the ipdb test works just fine for me. It's only pydevd that'll need some more work. |
I must not have pushed my latest commits. If I actually check to make sure the test didn't error out ipdb will fail. |
I removed importing |
Hi, apologies for the break. Life can be hectic. So this works, but has no working tests for ipdb or pydevd. ipdb would be fixed once we have a new pytest. Could we at least mark these tests as xfail? Currently the test suite doesn't pass at all. I'm a bit concerned that with an xfailing test no one will ever bother to work on fixing it though.. |
Fair enough, I could definitely see that becoming the case. |
Could you rebase and fix the conflicts as well as xfail ipdb for now. There's no need to tie this to a pytest release itself for now and this is pretty much ready to be merged otherwise I think? Anything you'd still like to see addressed first? |
Nope, will rebase and xfail ipdb when I get the chance. |
(I'm showing my lack of git knowledge but) @flub I tried to rebase but there was quite a few merge conflicts, is there any reason I couldn't merge instead? |
Just had a look at doing this. Seems like there's already a merge I also don't mind doing the whole thing, but I'm not entirely sure on |
I don't care about the commit history that much. I'll remerge master. |
# Conflicts: # README.rst
We still need to skip this check on windows as our windows CI doesn't work with this.
Should really add this to pre-commit, but it's far from clean at the moment.
Fixes #32
This improved functionality is implemented by checking ifSee discussion for evolution of detection.pydevd.py
appears in any of the stack frames. If it does, we assume that a debugging session is active.Also updated:
py.test
->pytest
,py.test
is apparently deprecatedREADME.rst
to reflect new functionality