Make gen_test's timeout configurable #723

merged 5 commits into from Apr 18, 2013


None yet

3 participants

ajdavis commented Apr 11, 2013

Lessons learned from testing Motor with my equivalent, async_test_engine: it's nice to be able to increase the timeout on specific tests if they legitimately take over 5 seconds. It's also nice to set the timeout with an environment variable when launching an interactive debugging session - in this case I generally set the timeout to something huge so I'm not interrupted while debugging.


This generally seems like a good idea, but I have a few comments:

For decorators with optional arguments, I prefer to have "func=None" as the first argument and require that the other args be passed by keyword, both to enforce use of the keyword at the call site for legibility and to avoid type-based dispatch.

I'm -1 on using an environment variable to set the timeout process-wide (especially with as generic a name as "TIMEOUT"). If we're going to have a global config I'd rather figure out how best to use tornado.options for this kind of internal use, but setting a global floor in absolute terms doesn't feel like the right approach.

If your use case is a debugger I think what you want is not a floor on all timeouts but a way to suspend time while the program is paused. Fortunately I think that's not too hard as long as your code uses IOLoop.time() instead of time.time() (BTW, the toro 0.5 docs talk about time.time() but the code uses IOLoop.time()).

class DebuggableIOLoop(SelectIOLoop):
  def __init__(self):
    self.clock = 0

  def poll(self, timeout):
    start = time.time()
    result = super().poll(timeout)
    self.clock += time.time() - start
    return result

  def time(self):
    return self.clock
ajdavis commented Apr 12, 2013

I'll fix the Toro docs, thanks. And I agree about the decorator's arguments.

I think I understand your DebuggableIOLoop idea, interesting.

I assert that interactively debugging a gen_test-decorated method is a very common use-case and that overriding the timeout should be an easily accessible feature. When a test fails I want to start debugging it immediately. IDEs like PyCharm make it easy to set an environment variable before debugging. Furthermore, PyCharm knows how to run the test my cursor is in right now, a feature I use all the time—but it runs it with Nose, not tornado.test.runtests. Hence my preference for the env var.

Which do you think is best?:

  1. Keep the env var, but give it a better name (GEN_TEST_TIMEOUT?)
  2. Make AsyncTestCase use DebuggableIOLoop by default
  3. Ship DebuggableIOLoop so it's available with runtests --ioloop=DebuggableIOLoop (makes it harder to use with IDEs)
  4. Something else?

Interesting. Many of the python users I know left java to get away from the IDE-centric culture there, so I haven't heard much about what python IDEs do. (dictating a particular test runner seems kind of obnoxious)

Whatever we do for gen_test's timeout should also apply to AsyncTestCase.wait, so if it's an environment variable maybe call it ASYNC_TEST_TIMEOUT?

DebuggableIOLoop is a little too magical to make the default (plus it's not really usable by projects that haven't dropped Tornado 2.x compatibility, since it requires the use of IOLoop.time).

I think the viable options are:

  1. Environment variable. It's a small change and most compatible with the IDE, but it introduces a different kind of configuration than we use anywhere else.
  2. Overridable method on AsyncTestCase. This allows application developers to override timeouts however they want (environment variable? detect debugger with sys.gettrace?) and gets it out of my hair, but projects have to opt in if any of their developers want to use this feature. It's consistent with methods like get_new_ioloop, but less convenient.
  3. tornado.options. Overridable constants like this are what the options module is good at but we don't currently impose the options module on projects, and it's unclear how you'd set the option in this case.

I'll think some more about the future of the options module and whether it makes sense to use it here. Otherwise I'm leaning towards the environment variable.

ajdavis commented Apr 13, 2013

Thanks, Ben. I agree that PyCharm's test config is inflexible, but even so it's awfully convenient! I'll show you some time. A quick Googling suggests that Komodo's and WingWare's testing features are similarly useful and restrictive.

The latest patches rename the variable to ASYNC_TEST_TIMEOUT and apply it to AsyncTestCase.wait as well as gen_test.

dkador commented Apr 13, 2013

Been following along. I'm excited about this change because I also use PyCharm and have run into some of the same frustration as Jesse. I wanted to note that you have your choice of what test runner to use with the IDE. I too prefer nosetest, but I believe other test runners are usable as well.


OK, I've decided to merge this, and then to address my concerns about environment vs tornado.options I'm going to make the timeout an option that takes its default variable from an environment variable. The next question is whether all options should look for a similarly-named environment variable. Have you seen a need for other settings to be configured via the environment?

I think it might be useful to have e.g. a --ioloop option with an environment variable default; this would help in cases like #615 where a user needs to select a non-default IOLoop for all tornado processes they run (other hypothetical options that would be useful in both a command-line and environment form are --httpclient_allow_ipv6 and the proxy-related options).

One drawback to reading from environment variables is that options that rely on hooks (e.g. logging) would still require parse_command_line or parse_config_file to be run at some point to trigger the hooks so an env-only configuration wouldn't work consistently.

@bdarnell bdarnell merged commit 0d44009 into tornadoweb:master Apr 18, 2013

1 check passed

Details default The Travis build passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment