addCleanup method on TestCase to allow for easier tear down.

[Jonathan Lange]

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

[Jonathan Lange]

Ready for review in addCleanup-2610

[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!

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

[Jonathan Lange]

See r20104

[Jean-Paul Calderone]

Please follow the coding standard.

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

[therve]

• All the methods you've added should include docstrings in the format :

  """
  doc
  """
  

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

Thanks!

[Jonathan Lange]

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.

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

[therve]

Last comment: deferRunCleanups and _cbDeferRunCleanups are missing docstrings.

[Jonathan Lange]

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