Ticket #2610 (closed enhancement: fixed)

Opened 3 years ago

Last modified 3 years ago

addCleanup method on TestCase to allow for easier tear down.

Reported by: jml Owned by: jml
Priority: highest Milestone:
Component: trial Keywords:
Cc: therve Branch:
Author: Launchpad Bug:

Description

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

Change History

Changed 3 years ago by jml

  • owner jml deleted
  • priority changed from normal to highest
  • keywords review added

Ready for review in addCleanup-2610

Changed 3 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!

Changed 3 years ago by jml

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.

Changed 3 years ago by jml

See r20104

Changed 3 years ago by exarkun

  • owner set to jml
  • keywords review removed

Please follow the coding standard.

Changed 3 years ago by jml

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.

Changed 3 years ago by 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!

Changed 3 years ago by therve

  • cc therve added

Changed 3 years ago by jml

  • keywords review added
  • owner changed from jml 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.

Changed 3 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.

Changed 3 years ago by therve

  • keywords review removed
  • owner changed from therve to jml

Last comment: deferRunCleanups and _cbDeferRunCleanups are missing docstrings.

Changed 3 years ago by jml

  • status changed from new to closed
  • resolution set to fixed

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

Note: See TracTickets for help on using tickets.