add --session-timeout #165

okken · 2024-01-23T23:30:31Z

Addresses request #163

okken · 2024-01-24T15:06:33Z

Adding WIP because docs need to be updated.

pytest_timeout.py

test_pytest_timeout.py

okken · 2024-01-25T23:18:31Z

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 · 2024-01-26T21:50:50Z

@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

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

flub · 2024-01-29T20:57:41Z

pytest_timeout.py

@@ -18,6 +19,7 @@


 __all__ = ("is_debugging", "Settings")
+suite_timeout_key = pytest.StashKey[float]()


TIL about pytest's Stash. As this is a module-level constant though could it be upper case?

Uppercase is fine with me. I was going by convention of uses of StashKey in pytest-dev: https://github.com/search?q=org%3Apytest-dev%20pytest.StashKey&type=code
However, I like it uppercase. good call.

pytest_timeout.py

flub · 2024-01-29T21:05:24Z

pytest_timeout.py

@@ -507,3 +534,12 @@ def dump_stacks(terminal):
            thread_name = "<unknown>"
        terminal.sep("~", title="Stack of %s (%s)" % (thread_name, thread_ident))
        terminal.write("".join(traceback.format_stack(frame)))
+
+
+def pytest_runtest_makereport(item, call):


this hook is apparently called many times for a single test, see CallInfo. Furthermore this hook is documented to "stop at first non-None result". are you sure this hook will always correctly interact with the hook that writes to the terminal or whatever output?

I'll add a test for if call.when == "teardown" so it's only called once per test.
I can also add a hookimpl(tryfirst=True) to try to ensure that other hooks that might return non-None run after this one.
But really, I don't think there are guarantees

flub · 2024-01-29T21:24:13Z

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 to pytest_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 · 2024-01-31T19:03:29Z

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 · 2024-01-31T19:14:04Z

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 to pytest_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 · 2024-02-06T19:01:48Z

pytest_timeout.py

+
+    suite_timeout = config.getoption("--suite-timeout")
+    if suite_timeout:
+        timeout_header.append("suite timeout: %ss" % suite_timeout)


can you add some tests for this? I suspect you missed some newlines here.

flub · 2024-02-06T19:12:52Z

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 · 2024-02-06T22:47:58Z

@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 · 2024-02-06T22:56:37Z

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 · 2024-02-07T20:08:11Z

@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.

…out more clear.

okken · 2024-02-07T21:21:17Z

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.

flub

Looking good! Maybe time to start editing the README :)

flub · 2024-02-08T18:38:11Z

test_pytest_timeout.py

+    # 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


i don't think this is asserted? not immediately sure how you would assert it, maybe with more verbose output and more output lines to match. or maybe there's something easier.

i don't think this is addressed yet?

flub · 2024-02-08T18:42:00Z

test_pytest_timeout.py

+    # 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


0.25 from an edge is too brittle. experience tells me you can't really work with sub-seconds times in this test suite, it gets flaky otherwise.

Make the total test duration 2s and the session timeout 1s?

All the numbers in this description are now wrong. maybe just describe that this is designed to timeout during the first test and ensure the first test still runs to completion but the 2nd test is not started?

okken · 2024-02-09T00:54:44Z

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

README.rst

pytest_timeout.py

flub · 2024-02-11T14:57:10Z

test_pytest_timeout.py

+    # 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


All the numbers in this description are now wrong. maybe just describe that this is designed to timeout during the first test and ensure the first test still runs to completion but the 2nd test is not started?

flub · 2024-02-11T14:57:22Z

test_pytest_timeout.py

+    # 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


i don't think this is addressed yet?

test_pytest_timeout.py

flub · 2024-02-25T15:26:21Z

@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 · 2024-02-25T16:43:45Z

@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 · 2024-02-25T16:59:42Z

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

okken · 2024-02-25T17:00:22Z

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

okken · 2024-02-25T17:03:16Z

I'm pretty sure I addressed any concerns.

flub · 2024-03-07T20:50:18Z

Thanks for the work and bearing with me!

For pytest-dev/pytest-timeout#165. [no changelog]

add --suite-timeout

7ef1122

flub reviewed Jan 25, 2024

View reviewed changes

pytest_timeout.py Outdated Show resolved Hide resolved

pytest_timeout.py Outdated Show resolved Hide resolved

pytest_timeout.py Outdated Show resolved Hide resolved

test_pytest_timeout.py Outdated Show resolved Hide resolved

okken added 2 commits January 26, 2024 13:43

switch to seconds

901762f

Merge branch 'master' into okken_suite_timeout

e4e357d

flub reviewed Jan 29, 2024

View reviewed changes

okken added 3 commits January 31, 2024 10:36

recommended flub changes

f6cd2e0

gate logic to once per test

4d7e9e7

try to run hook first

4e6063e

add suite timeout to header, linting fix.

77ad48d

flub reviewed Feb 6, 2024

View reviewed changes

move suite timeout to pytest_runtest_protocol

3351d99

suite-timeout -> session-timeout

44dc497

okken changed the title ~~add --suite-timeout~~ add --session-timeout Feb 6, 2024

extend test_header to include session timeout. Make test_session_time…

fcdc5eb

…out more clear.

flub reviewed Feb 8, 2024

View reviewed changes

flub mentioned this pull request Feb 8, 2024

[QUESTION] When can we expect a release? #167

Closed

flub changed the base branch from master to main February 8, 2024 19:07

okken added 2 commits February 8, 2024 16:43

docs + config setting support + test mod

a611873

docs update

d01a14f

readme

ea9fa95

flub reviewed Feb 11, 2024

View reviewed changes

describe session timeout better

ea6281e

Tweak docs a little

ef5f18b

flub approved these changes Mar 7, 2024

View reviewed changes

flub merged commit 38c5f24 into pytest-dev:main Mar 7, 2024
14 checks passed

flub mentioned this pull request Mar 7, 2024

feature request: add suite timeout #163

Closed

romanz added a commit to trezor/trezor-firmware that referenced this pull request Feb 11, 2025

chore(python): bump pytest-timeout to 2.3.1

739d477

For pytest-dev/pytest-timeout#165. [no changelog]

romanz added a commit to trezor/trezor-firmware that referenced this pull request Feb 11, 2025

chore(python): bump pytest-timeout to 2.3.1

b55a943

For pytest-dev/pytest-timeout#165. [no changelog]

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add --session-timeout #165

add --session-timeout #165

okken commented Jan 23, 2024

okken commented Jan 24, 2024

okken commented Jan 25, 2024

okken commented Jan 26, 2024 •

edited

Loading

flub left a comment

flub Jan 29, 2024

okken Jan 31, 2024

flub Jan 29, 2024

okken Jan 31, 2024

flub commented Jan 29, 2024

okken commented Jan 31, 2024

okken commented Jan 31, 2024

flub Feb 6, 2024

flub commented Feb 6, 2024

okken commented Feb 6, 2024

okken commented Feb 6, 2024

flub commented Feb 7, 2024

okken commented Feb 7, 2024

flub left a comment

flub Feb 8, 2024

flub Feb 11, 2024

flub Feb 8, 2024

flub Feb 11, 2024

okken commented Feb 9, 2024

flub Feb 11, 2024

flub Feb 11, 2024

flub commented Feb 25, 2024

okken commented Feb 25, 2024

okken commented Feb 25, 2024

okken commented Feb 25, 2024

okken commented Feb 25, 2024

flub commented Mar 7, 2024

		@@ -18,6 +19,7 @@


		__all__ = ("is_debugging", "Settings")
		suite_timeout_key = pytest.StashKey[float]()

add --session-timeout #165

add --session-timeout #165

Conversation

okken commented Jan 23, 2024

okken commented Jan 24, 2024

okken commented Jan 25, 2024

okken commented Jan 26, 2024 • edited Loading

flub left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flub commented Jan 29, 2024

okken commented Jan 31, 2024

okken commented Jan 31, 2024

Choose a reason for hiding this comment

flub commented Feb 6, 2024

okken commented Feb 6, 2024

okken commented Feb 6, 2024

flub commented Feb 7, 2024

okken commented Feb 7, 2024

flub left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okken commented Feb 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flub commented Feb 25, 2024

okken commented Feb 25, 2024

okken commented Feb 25, 2024

okken commented Feb 25, 2024

okken commented Feb 25, 2024

flub commented Mar 7, 2024

okken commented Jan 26, 2024 •

edited

Loading