add --session-timeout

[okken]

Addresses request #163

[okken]

Adding WIP because docs need to be updated.

[okken]

This is awesome feedback. thanks so much for taking the time to add detailed notes. I’m grateful for your perspective. I will play with this a bit and try some changes.

[okken]

@flub I've made, I think, all suggested changes except the timeout location. I went with pytest_runtest_makereport because that was the last point in pytest_runtest_protocol list of hooks that I could get hold of item, and thus session. If I did the work right in pytest_runtest_protocol, I would end up with an extra test being run after the timeout.

Again, thanks so much for your feedback. I've learned even more about pytest with this effort, and I appreciate it.

[flub]

If I did the work right in pytest_runtest_protocol, I would end up with an extra test being run after the timeout.

I'm not sure I follow this yet. The docs I can find say:

If at any point session.shouldfail or session.shouldstop are set, the loop is terminated after the runtest protocol for the current item is finished.

Looking at pytest_runtestloop() in main.py seems to support what those docs are saying. It calls raise session.Failed(session.shouldfail) after the call topytest_runtest_protocol()` thus cleanly terminating in between two items, without starting the next one or starting to call any reporting hooks about the next item.

What am I missing? (I'm just reading code, not running so am probably missing something...)

[okken]

This should probably be added to the pytest_report_header as well. Or maybe only if it is set to keep the noise down

good idea. I usually run with --no-header so it didn't occur to me. :)

[okken]

If I did the work right in pytest_runtest_protocol, I would end up with an extra test being run after the timeout.

I'm not sure I follow this yet. The docs I can find say:

If at any point session.shouldfail or session.shouldstop are set, the loop is terminated after the runtest protocol for the current item is finished.

Looking at pytest_runtestloop() in main.py seems to support what those docs are saying. It calls raise session.Failed(session.shouldfail) after the call topytest_runtest_protocol()` thus cleanly terminating in between two items, without starting the next one or starting to call any reporting hooks about the next item.

What am I missing? (I'm just reading code, not running so am probably missing something...)

What I'd really like is a hook right before pytest_runtest_protocol to be able to NOT run the next test if a timeout occurs. But we don't have that.

Example: 10 tests that each take say 10 sec setup, 10 sec test, 10 sec teardown. So total of 30 sec per full test. If we set suite timeout to 15 sec. we expect one test to complete, and not run a second test.

However, if we check for timeout in pytest_runtest_protocol we will see a timeout right when we start the second test, and we mark it to stop testing, then we will complete 2 tests, and run for 60 seconds.

If we check timeout in pytest_runtest_teardown, we still only check the timeout at the beginning of the teardown. It's not terrible there. But if someone has long teardowns, and the timeout is during the teardown, then we don't see the timeout until the next test is done.

So I picked the hook that can be checked after the teardown, and that really is only pytest_runtest_makereport

[flub]

What I'd really like is a hook right before pytest_runtest_protocol to be able to NOT run the next test if a timeout occurs. But we don't have that.

Example: 10 tests that each take say 10 sec setup, 10 sec test, 10 sec teardown. So total of 30 sec per full test. If we set suite timeout to 15 sec. we expect one test to complete, and not run a second test.

Right, let's make a test for exactly this scenario so we can play with this and make sure it behaves right. A test with two tests, and the suite-timeout is expected to trigger during the first test.

Because while I entirely agree with all your logic and desire to when to interrupt, I still don't understand what the thing is that stops the check from being added at the end of pytest_runtest_protocol. That way it'll always run the first test in a suite, but given the design you proposed that is what I would expect.

However, if we check for timeout in pytest_runtest_protocol we will see a timeout right when we start the second test, and we mark it to stop testing, then we will complete 2 tests, and run for 60 seconds.

If we check timeout in pytest_runtest_teardown, we still only check the timeout at the beginning of the teardown. It's not terrible there. But if someone has long teardowns, and the timeout is during the teardown, then we don't see the timeout until the next test is done.

I think when using the pytest_runtest_teardown hook you can also choose to run whatever code before or after the teardown? It is the hook that invokes the teardown, so it can do whatever is right? (not that I think we need to do something in this hook, but I'm trying to understand why you think this is an issue)

[okken]

@flub Thanks for your patience with me and this PR. I honestly didn't understand how @pytest.hookimpl(hookwrapper=True) and yield worked together to allow code to run at the end.

I've moved the suite timeout check to pytest_runtest_protocol. It was already being used by the plugin, so I tacked on the suite timeout check at the end of the existing implementation.

Question: Does it make sense to call this flag --session-timeout instead of --suite-timeout?

It just occurred to me that the rest of pytest generally uses the term "session" instead of "suite". Both are used in the documentation, but I think the code only uses "session".

[okken]

I'm going to assume you agree that --session-timeout makes more sense, so changing the PR. Let me know if you like --suite-timeout better.

[flub]

@flub Thanks for your patience with me and this PR. I honestly didn't understand how @pytest.hookimpl(hookwrapper=True) and yield worked together to allow code to run at the end.

Thanks for keeping at it! And bearing with my slow responses (I've been ill in between as well :( )

I've moved the suite timeout check to pytest_runtest_protocol. It was already being used by the plugin, so I tacked on the suite timeout check at the end of the existing implementation.

Glad to see this is working out! I think the changes are much simpler now.

Question: Does it make sense to call this flag --session-timeout instead of --suite-timeout?

It just occurred to me that the rest of pytest generally uses the term "session" instead of "suite". Both are used in the documentation, but I think the code only uses "session".

Yep, that's probably better.

---

I think there are a few more tests that would be really helpful to make sure this behaves right:

• The test for the header output. We're concatenating strings now and we may want to really match the output to what we think it is.
• I think the test described earlier that ensures a timeout occurs exactly in between two given tests would be good. This is what the feature promises so we should test it.

[okken]

First, I added a test that would do the 2 test thing, with a slow fixture also, with this comment:

# 2 tests, each with 0.5 sec timeouts
# each using a fixture with 0.5 sec setup and 0.5 sec teardown
# So about 1.5 seconds per test, ~3 sec total,
# Therefore:
# A timeout of 1.25 sec should happen during the teardown of the first test
# and the second test should NOT be run

Then I realized that's also sufficient to test the timeout, so I just changed the one existing test to do that.

Second, I extended test_header to include session timeout, rather than add another header test just for session.

[okken]

I'm really not sure where the right place to put the Session Timeout info in the docs, so I just:

• added a section at bottom, right before the changelog
• added a line to the 'unreleased' section of the changelog

When modifying the changelog I noticed the ini file setting for timeout, and figured session_timeout would also be good. So I added that to the code and added an inifile test for session_timeout

[flub]

@okken ping, would be nice to see this merged. i was kind of waiting for this to publish a release. unless you think it'll take longer and i should release without waiting?

[okken]

@flub thanks for the ping. I have so much going on and this slipped through the cracks. I'll take a look right now.

[okken]

I just pushed up some changes. All doc and/or comment changes

[okken]

Feel free to modify any wording as you see fit to expedite the release

[okken]

I'm pretty sure I addressed any concerns.

[flub]

Thanks for the work and bearing with me!