Opened 11 years ago

Closed 11 years ago

#2610 enhancement closed fixed (fixed)

addCleanup method on TestCase to allow for easier tear down.

Reported by: Jonathan Lange Owned by:
Priority: highest Milestone:
Component: trial Keywords:
Cc: therve Branch:


bzrlib has a neat feature on their TestCase subclass called addCleanup. We should emulate this feature.

Change History (13)

comment:1 Changed 11 years ago by Jonathan Lange

Keywords: review added
Owner: Jonathan Lange deleted
Priority: normalhighest

Ready for review in addCleanup-2610

comment:2 Changed 11 years ago by therve

Could you add some examples in the twisted test suite where it's actually useful? I think I understand what it does but I don't see what it brings against tearDown. Thanks!

comment:3 Changed 11 years ago by Jonathan Lange

Off the top of my head:

  1. addCleanup keeps the teardown code physically close to the setup code.
  2. addCleanup makes it easy to have factory methods in your test classes for things which require substantial dismantling.

comment:4 Changed 11 years ago by Jonathan Lange

See r20104

comment:5 Changed 11 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Jonathan Lange

Please follow the coding standard.

comment:6 Changed 11 years ago by Jonathan Lange

Normally, I find your code reviews extremely valuable and helpful. This one is an exception.

It would be more helpful if you cited specific coding standard violations, or at least which kinds of coding standard violations are prevalent throughout this branch.

comment:7 Changed 11 years ago by therve

  • All the methods you've added should include docstrings in the format :
  • Test methods names should only have the first underscore. For example you may change test_addCleanupCalled_in_reverse_order to test_addCleanupReverseOrder
  • Unused import in test_tests
  • Please add copyright statement in test_tests, and update the one in unittest.
  • The code in _runCleanups is hard to read, and its intend is not very clear. I don't think fireOnOneErrback and consumeErrors are compatible options (the first overriding the second). We probably want to run all the cleanup functions, regardless if another fail, so consumeError seems appropriate. But I feel that it would be a good place for a deferGenerator.


comment:8 Changed 11 years ago by therve

Cc: therve added

comment:9 Changed 11 years ago by Jonathan Lange

Keywords: review added
Owner: changed from Jonathan Lange to therve

Fixed up the copyright and coding standard issues.

I've moved the 'sequentially run functions which may or may not return deferreds' logic to a function in t.internet.defer called runSequentially and modified it to use deferredGenerator. It is clearer.

comment:10 Changed 11 years ago by therve

Humm... I really like runSequentially, and the code in _runCleanups is indeed much clearer. But I feel that this change might be big enought to deserve its own branch. What do you think?

As I understand that you may not want to postpone this forever, you can remove runSequentially from here, add a TODO in _runCleanups, then merge.

comment:11 Changed 11 years ago by therve

Keywords: review removed
Owner: changed from therve to Jonathan Lange

Last comment: deferRunCleanups and _cbDeferRunCleanups are missing docstrings.

comment:12 Changed 11 years ago by Jonathan Lange

Resolution: fixed
Status: newclosed

(In [20290]) Add TestCase.addCleanup and defer.runSequentially.

  • Author: jml
  • Reviewer: therve
  • Fixes #2610

addCleanup is an easy way to add cleanup routines to unit tests. It's particularly useful for factory methods which return objects that require some sort of cleanup (such as an explicit disconnect). Because this is Twisted, callables passed to addCleanup can return Deferreds.

runSequentially takes a sequence of nullary callables and runs them sequentially. If a callable returns a Deferred, runSequentially will not run the next callable until the callback chain of the Deferred has been completed.

comment:13 Changed 7 years ago by <automation>

Owner: Jonathan Lange deleted
Note: See TracTickets for help on using tickets.