Add timeout for Testdir.runpytest_subprocess() and Testdir.run() #4078
Conversation
Codecov Report
@@ Coverage Diff @@
## features #4078 +/- ##
============================================
+ Coverage 94.5% 94.54% +0.04%
============================================
Files 109 109
Lines 23920 23963 +43
Branches 2370 2375 +5
============================================
+ Hits 22606 22657 +51
+ Misses 1002 997 -5
+ Partials 312 309 -3
Continue to review full report at Codecov.
|
nicoddemus
left a comment
nicoddemus
left a comment
There was a problem hiding this comment.
Great initial work @altendky!
Please also add a CHANGELOG entry.
As I commented in the PR, I would rather avoid including the monotonic dependency if possible, but if we decide to keep it, we should add a trivial CHANGELOG entry mentioning the new dependency.
src/_pytest/pytester.py
Outdated
| except subprocess.TimeoutExpired: | ||
| raise self.TimeoutExpired() | ||
| else: | ||
| end = monotonic.monotonic() + timeout |
There was a problem hiding this comment.
I don't think we need to introduce a new dependency to get an accurate timeout here, TBH. I think using time.time() on Linux and time.clock() on Windows will give us good enough accuracy? To be clear I like having the two implementations separate like this, makes it easier to drop the Python 2-only code later, I'm questioning the external dependency to achieve that.
There was a problem hiding this comment.
Assuming nobody goes changing the system clock during a test... sure. I expected resistance to the dependency but defaulted to using the 'proper' thing. (here anyways) Sure, I'll remove it.
src/_pytest/pytester.py
Outdated
|
|
||
| remaining = end - monotonic.monotonic() | ||
| if remaining <= 0: | ||
| raise self.TimeoutExpired() |
There was a problem hiding this comment.
I suggest that we add a message here with the command and how many seconds was the timeout.
src/_pytest/pytester.py
Outdated
| args = self._getpytestargs() + args | ||
| return self.run(*args) | ||
|
|
||
| if "timeout" in kwargs: |
There was a problem hiding this comment.
This is a bit simpler:
return self.run(*args, timeout=kwargs.get("timeout"))There was a problem hiding this comment.
Given the verbosity of what I did it's probably worthwhile, but not quite the same. My way is independent of how the defaulting is handled in the called function. Yours depends on the default being triggered by passing None. But sure, will do.
| assert testdir.runpytest_subprocess(testfile).ret == EXIT_OK | ||
|
|
||
|
|
||
| def test_testdir_run_timeout_expires(testdir): |
asottile
left a comment
asottile
left a comment
There was a problem hiding this comment.
oops forgot to press submit, some of this is duped from what @nicoddemus said so feel free to ignore those :)
setup.py
Outdated
| "attrs>=17.4.0", | ||
| "more-itertools>=4.0.0", | ||
| "atomicwrites>=1.0", | ||
| "monotonic", |
There was a problem hiding this comment.
this is only used in python2.x and so it should be a conditional dependency
There was a problem hiding this comment.
I left it this way because then you can access monotonic consistently via monotonic.monotonic(), though sure, at this point we only use it for one thing and only in py2 (and it seems is going away anyways).
| return popen | ||
|
|
||
| def run(self, *cmdargs): | ||
| def run(self, *cmdargs, **kwargs): |
There was a problem hiding this comment.
should this be timeout=None instead of allowing arbitrary kwargs? I notice they aren't checked so it could potentially allow .run(foo='bar') even when foo isn't used / would be a TypeError otherwise
There was a problem hiding this comment.
I'd love to do that, and did do that, but py2. This re-surprises me each time I find it.
https://repl.it/@altendky/pytest-devpytest4078-001
def f(*args, something=None):
passPython 2.7.10 (default, Jul 14 2015, 19:46:27)
[GCC 4.8.2] on linux
Traceback (most recent call last):
File "python", line 1
def f(*args, something=None):
^
SyntaxError: invalid syntax
But yes, if we are concerned about extras getting lost I can add some validation. I was maybe being a bit loose with it given elsewhere in the file there's a random **kwargs that is (was?) entirely unused.
There was a problem hiding this comment.
oh, here's keyword-only arguments in python2+3:
def f(**kwargs):
arg = kwargs.pop('arg', None)
if kwargs:
raise TypeError('Unexpected arguments: {}'.format(', '.join(sorted(kwargs))))
|
@nicoddemus and @asottile, thanks for the interest and feedback. The main thing I dislike about this is rolling my own timeout. |
|
Thanks @altendky for the quick response!
I'm OK with the implementation you posted. 👍 |
|
@nicoddemus, also, there are several other functions that could expose and pass through a timeout parameter. Should they all? That would be my tendency. Tests for each or just the one that actually implements the timeout? |
|
I would only add the timeout parameter to |
Too bad it's horrible. It waits 90% of the timeout even if the process finishes immediately. First option is just to have a (parameter-settable?) resolution for waiting. Maybe default to 0.1 seconds. Second option is maybe something with |
|
So |
|
yeah probably going to need a loop anyway, there's no signals on windows :) |
|
would it be the worst to only support this behaviour in python3+? |
|
You think the code sitting there is going to cause enough trouble we don't want to deal with maintaining it? But in my case it's for pytest-twisted which is supporting 2.7 so I do want it, yes. :] |
|
ah ok I meant more because this is going to be hard to get right / working on python2 (especially python2 windows) 😆 |
|
Do we really want to be throwing away all the (now other than |
|
I'm almost satisfied. Just the two remaining questions left in the original message for tracking.
|
|
@altendky I think so, the patch looks good as is to me. 👍 |
asottile
left a comment
asottile
left a comment
There was a problem hiding this comment.
otherwise looks great -- thanks for the patch!
src/_pytest/pytester.py
Outdated
| break | ||
|
|
||
| remaining = end - time.time() | ||
| if remaining <= 0: |
There was a problem hiding this comment.
I would write if time.time() > end: myself, but this is equivalent logic
There was a problem hiding this comment.
That's a leftover from when I actually used remaining elsewhere. Thanks for catching it.
It cancels previous builds for new commits, assuming that you have pushed fixups. It is a Travis setting, enabled for pytest. |
|
@blueyed thanks but I think it isn't that. Both builds are for the same commit and start at the same time as far as I can tell. The GitHub build links start out pointing at the canceled build. https://travis-ci.org/pytest-dev/pytest/builds/437650065 |
|
Yeah, looks weird (same commit). Not sure. Likely related to this feature though I guess. |
|
@blueyed there are a lot of duplicates including for other people as well, though some of them actually fully ran both builds instead of canceling. https://travis-ci.org/pytest-dev/pytest/pull_requests anyways... |
altendky
changed the title
altendky
changed the title
|
I really don't like that we are already getting silly timeout errors. https://travis-ci.org/pytest-dev/pytest/jobs/437814901#L959 An empty test is taking more than 10 seconds? Should I just massively extend these times so they don't fail for a minute or five or such? |
|
@altendky |
|
@blueyed, good call, I forgot about that feature. Thanks. I'll review that when I get a chance. |
altendky
changed the title
|
@blueyed, I assume it was random slowness since only one job failed. Certainly we want the tests to pass quickly, but we can probably afford to occasionally wait a long time to avoid having flaky tests (so long as test integrity is maintained). |
|
I forgot I had permissions to rebuild, and now it passes. So yeah, intermittent... :[ I hate time. |
altendky
changed the title
|
I'm not used to being able to merge things myself... Am I supposed to do that now? Wait for somewhere else to merge it? It's no matter to me, I can wait. |
|
Oh you are free to merge after at least one reviewer has approved and there are no more comments to address. 😁 I would merge it now, but please do the honors. 🎉 |
|
Awesome, thanks @altendky! |
|
|
||
| start = time.time() | ||
| result = testdir.runpytest_subprocess(testfile, timeout=10) | ||
| result = testdir.runpytest_subprocess(testfile, timeout=120) |
There was a problem hiding this comment.
@altendky
Wouldn't that still fail with assert duration < 5 below?
There was a problem hiding this comment.
Unfortunately the build failure is gone, likely because it was restarted?! (would be good to copy'n'paste those in the future). (it was for py27-pluggymaster)
There was a problem hiding this comment.
:[ Yes, I should have extended that as well. Or, really, made one number calculated from the other to avoid this error in the future.
How should I proceed? Push a new commit to the branch? Start a new branch?
And :[ again. I did double check that my link to the broken build was for a specific build. I didn't realize that Travis was going to reuse that number and effectively break the link. Lesson learned. sigh Don't restart builds, trigger a new build (empty commit, close/reopen PR, something).
As this has already been merged, I believe we need to start a new branch. |
|
I think it is fine for now - just noticed this. |
|
I'd rather fix it now than leave a wrong and confusing thing sitting around. Unless either of you want to discourage correcting it. |
If you have the time, I personally prefer this to be fixed now. 😁 |
5 participants
#4073
WIP for:
signal.alarm()for py2 timeoutkwargs(other thantimeout) torunpytest_subprocess()?timeoutkwarg which was previously ignored?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):
changelogfolder, with a name like<ISSUE NUMBER>.<TYPE>.rst. See changelog/README.rst for details.Target themasterbranch for bug fixes, documentation updates and trivial changes.featuresbranch for new features and removals/deprecations.Unless your change is trivial or a small documentation fix (e.g., a typo or reword of a small section) please:
AUTHORSin alphabetical order;