Test timeout policy should be decided by runner

[jml]

At the moment, any test made from t.trial.unittest.TestCase will have a two minute default timeout. This is incorrect. The default timeout for tests should be specified by the runner, not by the Trial API. By default, tests should not timeout.

[glyph]

I'm not sure I agree with:

"By default, tests should not timeout."

but then I'm not sure what you're referring to. I think I agree with the general sentiment here of who should decide and why, but the default configuration of the default runner should specify a reasonable timeout.

[exarkun]

Like glyph, most of this ticket makes sense to me, but the last sentence of the description also confuses me. This test suite:

from twisted.trial.unittest import TestCase
from twisted.internet.defer import Deferred

class HangingTests(TestCase):
    def test_hangs(self):
        return Deferred()

should not result in trial test_hangs requiring a ^C (or other signal) to cause it to complete.

[jml]

Right. The default configuration of the default runner should timeout tests.

What I meant was that, HangingTests('test_hangs').run(TestResult()) should hang unless an explicit timeout-ing API is called.

[exarkun]

It would also be very useful if the timeout could be specified as a command line argument to the trial program.

I kind of want this for a number of reasons, so I'm bumping the priority.

[djfroofy]

N.B. There is a test in this patch which I'm marking skip for now (twisted.trial.test.test_deferred.TestNeverFire), so I don't think this is ready to merge as is; I don't know if this test should be revised or if it's no longer applicable given this change.

[jerub]

I don't think changing trial's default behaviour to by-default making unit test runs have a chance at hanging indefinitely is positive. Please update the patch to default to the current timeout, instead of defaulting to a zero timeout.

Please update twisted/trial/test/test_deferred.py's skip message (if required after the default timeout is changed) to be a logical complete sentence that makes sense in context. It only makes sense if you have read and understood the comments on this ticket and the difficulty posed by getting this ticket merged, and why that test has to be skipped. I currently do not fully understand why that test is skipped.

[djfroofy]

I will work on revised patch to do something smarter in twisted/trial/test/test_deferred.py - maybe you missed reading my question on this above? I definitely don't think we should skip the test either unless it is deemed irrelevant after this change - in which case it should just be removed.

Concerning the default behavior of trial (if we are talking about the script) with regards to timeouts - it is the same. The default timeout is still 120 seconds and has to be set to 0 when running through trial:

        ['timeout', 't', util.DEFAULT_TIMEOUT_DURATION, 
 	 'The default timeout in seconds. You can use the value 0 for no ' 
 	 'timeout but this may cause your tests to hang.', float]] 

The above DEFAULT_TIMEOUT_DURATION value is the same as before.

[djfroofy]

The revised patch does not skip the TestNeverFire test but instead sets deafultTimeout through api - as trial script currently does. See notes above also on default timeout behavior when running trial.

[spiv]

Thanks for this, I think this will make a nice improvement when it is ready. I'm happy with the basic idea to keep the default behaviour of the command-line the same while changing the default behaviour of the API to not timeout.

I have a few style nitpicks, but also some deeper concerns about the implementation.

1. Variable names test_defaultTimeout should be camelCase not lower_with_underscores
2. test_defaultTimeout needs a docstring. If it had a docstring clearly stating the intent of this test this review might have had less questions about this test. It certainly would have sped my review. My guess is something like “The value passed to C{--timeout} is set as the C{timeout} attribute of all the loaded tests.”
3. When stated like that the behaviour you're testing for sounds a bit weird to be honest: why do all the TestCase objects loaded by Trial need to be adjusted? I would have expected a policy more like:
   1. The runner's timeout is set by the --timeout option.
   2. As the runner runs a test, the timeout is the test's own timeout (if set), otherwise the runner's timeout (if set), otherwise no timeout.
   3. In the API, the runner's timeout defaults to None (no timeout).
   4. In the command-line options, the timeout defaults to 120, and a value of 0 (or perhaps -1 or "never"?) is translated to None (never timeout) in the API.
4. If I'm right that you don't need to be setting timeouts on all tests, then I think you don't need the suggestTimeout API. Which is good, because I think it's a bit smelly that it needs to be added to the TestSuite API, and I think as it's done at the moment breaks mixing Trial-native cases and suites with other pyunit compatible cases and suites. And removing suggestTimeout will save you from needing to add unit tests for it ;)
5. Rather than iterating over suite._tests[0]._tests, why not use _iterateTests(suite)? i.e. write that part of the test as:

   tests = [test for test in _iterateTests(suite)
            if isinstance(test, unittest.TestCase)]
   

6. You have excess whitespace in the list comprehension. [ t for t in … ] ought to be [t for t in …].
7. I'm not sure what the purpose of filtering by isinstance is here. No other tests in that file do that, so why is it needed here? Why not just take all the tests returned by _iterateTests?
8. I don't think there's any practical value in asserting that nonsensical values for the timeout attribute like “orange” can be set and retrieved.
9. In getTimeout, use return None not a bare return, because you explicitly intend to return that value, not just leave the function.

[djfroofy]

Thanks for the thorough review.

Replying to spiv:

Thanks for this, I think this will make a nice improvement when it is ready. I'm happy with the basic idea to keep the default behaviour of the command-line the same while changing the default behaviour of the API to not timeout. I have a few style nitpicks, but also some deeper concerns about the implementation. 1. Variable names test_defaultTimeout should be camelCase not lower_with_underscores

Sure no problem - I will include changes in a revised patch.

2. test_defaultTimeout needs a docstring. If it had a docstring clearly stating the intent of this test this review might have had less questions about this test. It certainly would have sped my review. My guess is something like “The value passed to C{--timeout} is set as the C{timeout} attribute of all the loaded tests.”

Your guess is wrong :) - see below.

3. When stated like that the behaviour you're testing for sounds a bit weird to be honest: why do all the TestCase objects loaded by Trial need to be adjusted? I would have expected a policy more like: 1. The runner's timeout is set by the --timeout option. 2. As the runner runs a test, the timeout is the test's own timeout (if set), otherwise the runner's timeout (if set), otherwise no timeout. 3. In the API, the runner's timeout defaults to None (no timeout). 4. In the command-line options, the timeout defaults to 120, and a value of 0 (or perhaps -1 or "never"?) is translated to None (never timeout) in the API.

I have implemented the policy you are describing above.

Concerning whether 0 or -1 should mean "never", I don't have many strong opinions except that 0 is less to type and I have the question: when would using an actual zero second timeout be useful?

5. If I'm right that you don't need to be setting timeouts on all tests, then I think you don't need the suggestTimeout API. Which is good, because I think it's a bit smelly that it needs to be added to the TestSuite API, and I think as it's done at the moment breaks mixing Trial-native cases and suites with other pyunit compatible cases and suites. And removing suggestTimeout will save you from needing to add unit tests for it ;)

Yes, I do not think this needs be an API directly on TestCase or TestSuite either. But somehow, somewhere in the code something like suggestTimeout needs to happen - i.e. if before a trial TestCaseis invoked we need to introspect it and see if it has defined a timeout explicitly and if not use the defaultTimeout (if not None). How would you recommend implementing this? Maybe a private function used in the suite and some adapters to deal with non-trial test cases?

5. Rather than iterating over suite._tests[0]._tests, why not use _iterateTests(suite)? i.e. write that part of the test as: {{{ tests = [test for test in _iterateTests(suite) if isinstance(test, unittest.TestCase)] }}}

Sure, will do.

6. You have excess whitespace in the list comprehension. [ t for t in … ] ought to be [t for t in …].

Okay, will correct.

7. I'm not sure what the purpose of filtering by isinstance is here. No other tests in that file do that, so why is it needed here? Why not just take all the tests returned by _iterateTests?

There are some stdlib unittest.TestCase classes in there so the point is to only muck with the timeouts on test cases that are trial TestCase instances. My understanding (which could be wrong) is that timeout isn't applicable to stdlib unittest TestCase instances.

8. I don't think there's any practical value in asserting that nonsensical values for the timeout attribute like “orange” can be set and retrieved.

Well, my intention was just exercising some backwards compat. In the code before my patch there is a comment on this:

    def getTimeout(self):
        # ... omitting docstring for brevity
        timeout =  util.acquireAttribute(self._parents, 'timeout',
                                         util.DEFAULT_TIMEOUT_DURATION)
        try:
            return float(timeout)
        except (ValueError, TypeError):
            # XXX -- this is here because sometimes people will have methods
            # called 'timeout', or set timeout to 'orange', or something
            # Particularly, test_news.NewsTestCase and ReactorCoreTestCase
            # both do this.
            warnings.warn("'timeout' attribute needs to be a number.",
                          category=DeprecationWarning)
            return util.DEFAULT_TIMEOUT_DURATION

9. In getTimeout, use return None not a bare return, because you explicitly intend to return that value, not just leave the function.

Sounds good.

[djfroofy]

This patch addresses concerns raised in last review by spiv.

[exarkun]

Thanks for pursuing this!

1. in twisted/scripts/trial.py, _getLoader is now setting an attribute on the TestSuite class. This is no good - it should set the attribute on an instance instead, somehow.
2. Regarding the 0 vs -1 discussion, for disabling the timeout, it's probably not a big deal, but -1 or some other sentinel is a better option. Using a 0 timeout may not be the most common use case, but 0 is a valid timeout, there's no reason to exclude it. When it doubt, avoid special cases.
3. It wouldn't be bad to have a test which verifies that with no --timeout option, the default timeout (120) is actually used.
4. What's the self.flushWarnings() for in test_defaultTimeout?
5. The trial man page should be updated (sorrrrrrryyyyyy)

Thanks again.