Opened 10 years ago

#5271 defect new

trial test method timeout support is fragile

Reported by: Jean-Paul Calderone Owned by:
Priority: normal Milestone:
Component: trial Keywords:
Cc: Jonathan Lange Branch:
Author:

Description

Trial supports timeouts for Deferred-returning test methods (including setUp and tearDown but excluding cleanups added with addCleanup). The timeout is enforced separately for each method.

However, the way the timeout is implemented has some problems.

The timeout is enforced on how long the Deferred returned by the method takes to fire. Trial itself goes on to further callbacks after this measured period, some of which may invoke user code - for example, if setUp takes too long, cleanups are still run, and no timeout is imposed on them.

Additionally, the way the timeout is imposed is by firing the application Deferred with a failure. This causes problems if the application code manages to get far enough to also try to fire the Deferred. This may lead to essentially spurious errors being logged against the test. It's also problematic if the application Deferred has errbacks which handle or suppress the failure trial fires the Deferred with.

Two changes would probably improve the situation:

  1. When a timeout expires, stop waiting on the application Deferred to fire before proceeding. Instead, report the timeout error, perform the necessary cleanup (call tearDown if appropriate, call cleanups, clean up the reactor, etc), and just move on.
  2. Impose the timeout on the entire process. Mostly, I think, this means applying it to runCleanups.

One other change might make sense as well. Instead of firing the application Deferred with a failure, cancel it. Then wait a shorter amount of time for it to fire, and then proceed with the cleanup steps. This would let tests still clean up resources they have acquired. However, it involves an additional timeout period, and it's not completely obvious that a cancellation error is going to be friendlier to test code than any other type of error. It does have the advantage of avoiding spurious AlreadyCalledErrors though, since Deferred.cancel explicitly deals with that issue.

Change History (1)

comment:1 Changed 10 years ago by DefaultCC Plugin

Cc: Jonathan Lange added
Note: See TracTickets for help on using tickets.