pytest-dev / pytest-timeout

MIT License
216 stars 64 forks source link

Add option to set timeout over whole test session #110

Closed jrgparkinson closed 1 year ago

jrgparkinson commented 3 years ago

New command line/ini option --timeout-session which will apply the timeout given by --timeout over the whole test session rather than individual tests.

flub commented 3 years ago

Thanks for contributing!

My first thought seeing this is why is the session timeout mutually exclusive with the per-test timeout? From a UX point of view I'd have expected to be able to do something like --timeout=5 --session-timeout=30.

Which kind of leads to my second thought, which features of pytest-timeout do you need on the session level? There are already many ways to avoid running a process, and thus a pytest process, indefinitely. And most CI systems do this already by default. So in what situations does having this in pytest-timeout help?

jrgparkinson commented 3 years ago

Hi @flub, yes I agree the interface is clumsy and it should be possible to set both per-test function and per-session timeouts as you describe. I imagine this will require using a different signal (e.g. signal.SIGUSR1) for the session timeout. Not sure about timeouts controlled by threading, but I imagine that's possible too.

Before I look into this further, I'll explain why I think this is a useful feature and see if you agree :)

Whilst it is always desirable to have quick tests, and often you may wish to enforce such a constraint during CI, sometimes it is not possible for every test function to execute within the same threshold (e.g. some tests necessarily require a wait stage). Instead, one alternative is to require that every test*.py file executes within some period of time. However I'm not aware of a pytest hook that allows this sort of timeout to be configured, so this is the next best thing and can achieve the same end result if each test.*py file is run separately.

The advantage of this strategy over other methods of ending long running processes is that the test session will exist gracefully, including teardown operations, so future tests in a pipeline shouldn't fail due to the timeout. This can make it easier to isolate and investigate failures in complex pipelines that involve running many tests.

This might seem like quite a niche situation, but I've seen it regularly enough to imagine this might be useful. If you agree, I'll investigate how to make it work in a more user-friendly manner.

flub commented 3 years ago

Right, I'm not really for this but also not really against it. If it's implemented well I don't mind, but bear in mind i won't be too responsive with reviews... so might need some patience.

To which point... I don't think your current implementation works IIUC after the timeout triggers the next test will still run.

I'm also not sure you need a second signal. Rather you should only set the signal when in a test as before, but the timer now needs to be set to min(global-deadline, test-deadline). Tests are interrupted as before and that's that. I don't think you ever want the signal to trigger during pytest's own code, that seems like asking for trouble, we should probably assume that code won't be the cause of insufferable delays. Instead you probably want to wrap the runtest loop so that in between each test you check if the session timeout has expired and whether to continue with the next test or abort. And if aborting you need to do it in such a way that all the session scoped fixture etc get torn down correctly, otherwise you defeat your own goal.

This last point reaches a new issue though, if you still end up code-under test (e.g. a fixture teardown) after the timeout triggered you no longer really reach the goal. I'm not sure how the teardowns and test execution works, so I'm afraid you may have to explore this a bit more. However this does make me wonder if there are any more edge cases like this. If we find too many that can not all be solved my opinion will probably sway back to preferring to do this timeout outside of pytest (only downside is you may need to clean up global state somewhere in a different way in your pipeline). But I don't really know the edge cases until i've seen more code...

Hopefully that braindump was not too incomprehensible...

jrgparkinson commented 2 years ago

Apologies for the very late response, and thank you for your thoughtful comments.

The current implementation with pytest_runtestloop does actually ensure that a) after the timeout is triggered, no further tests are run b) after the timeout is triggered, all module/session scoped teardowns are still run

Though I still don't fully understand how pytest achieves this.

I think your last point, about it being preferable in most cases to do this timeout outside of pytest then reset the environment another way, raises a key issue. In reality, I suspect the need for this specific way of doing things (session timeout within pytest) is probably very limited. Combining this with the difficulty of combining it neatly with the existing pytest-timeout package, and high possibility of breaking something or overcomplicating the interface to pytest-timeout, I think the better option right now would be for me to create a new standalone plugin for my purposes.

Which leads me to a final question. Given the MIT licence for this package, am I right in thinking that I would be free to create a new package which is heavily based on this one? I don't want to infringe any copyright or annoy anyone.

flub commented 2 years ago

Apologies for the very late response, and thank you for your thoughtful comments.

No worries, I myself am also this slow at times :)

The current implementation with pytest_runtestloop does actually ensure that a) after the timeout is triggered, no further tests are run b) after the timeout is triggered, all module/session scoped teardowns are still run

Good to know, I guess this part is not a problem then.

Though I still don't fully understand how pytest achieves this.

I guess the hook is still inside of whatever catches the exceptions.

I think your last point, about it being preferable in most cases to do this timeout outside of pytest then reset the environment another way, raises a key issue. In reality, I suspect the need for this specific way of doing things (session timeout within pytest) is probably very limited. Combining this with the difficulty of combining it neatly with the existing pytest-timeout package, and high possibility of breaking something or overcomplicating the interface to pytest-timeout, I think the better option right now would be for me to create a new standalone plugin for my purposes.

It doesn't sound overly complicated at the moment anymore, the session timeout would be some kind of soft-timeout in signal mode and hard timeout in thread mode, which matches the existing behaviour in pytest-timeout so is not so bad, at the moment I'm probably mildly optimistic to merge a good implementation.

Though of course, if you rather make a separate plugin, that's entirely fine and up to you (though if not and you care enough about a maintained timeout plugin i might try and rope you into more maintenance ;) I've been looking for a while for someone with more interest for maintaining this plugin)

Which leads me to a final question. Given the MIT licence for this package, am I right in thinking that I would be free to create a new package which is heavily based on this one? I don't want to infringe any copyright or annoy anyone.

Of course you are welcome to fork and re-use whatever you like! If you think this is the better approach then please do so.

nifx commented 2 years ago

Hi everyone, I also like this approach with a session timeout. Is there still anything planned or is there a separate plugin for this? Thanks in advance.

flub commented 1 year ago

Closing this for now as no one seems to be working on it anymore. But as the last comments indicate it could be re-opened.