Opened 15 years ago
Closed 15 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: | |
Author: |
Description
bzrlib has a neat feature on their TestCase subclass called addCleanup
. We should emulate this feature.
Change History (13)
comment:1 Changed 15 years ago by
Keywords: | review added |
---|---|
Owner: | Jonathan Lange deleted |
Priority: | normal → highest |
comment:2 Changed 15 years ago by
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 15 years ago by
Off the top of my head:
addCleanup
keeps the teardown code physically close to the setup code.addCleanup
makes it easy to have factory methods in your test classes for things which require substantial dismantling.
comment:5 Changed 15 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Jonathan Lange |
Please follow the coding standard.
comment:6 Changed 15 years ago by
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 15 years ago by
- 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!
comment:8 Changed 15 years ago by
Cc: | therve added |
---|
comment:9 Changed 15 years ago by
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 15 years ago by
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 15 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from therve to Jonathan Lange |
Last comment: deferRunCleanups and _cbDeferRunCleanups are missing docstrings.
comment:12 Changed 15 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
(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 11 years ago by
Owner: | Jonathan Lange deleted |
---|
Ready for review in
addCleanup-2610