Opened 8 years ago

Closed 7 years ago

#4300 task closed fixed (fixed)

Write some unit tests for Deferreds

Reported by: Jonathan Lange Owned by:
Priority: normal Milestone:
Component: core Keywords:
Cc: jesstess, therve Branch: branches/deferred-unit-tests-4300-2
branch-diff, diff-cov, branch-cov, buildbot
Author: exarkun, jml

Description

Deferreds need more unit tests, the current test suite has far too little coverage, making it hard for us to make improvements like #411 or C implementations of Deferreds.

Change History (20)

comment:1 Changed 8 years ago by Jonathan Lange

Author: jml
Branch: branches/deferred-unit-tests-4300

(In [28457]) Branching to 'deferred-unit-tests-4300'

comment:2 Changed 8 years ago by Jonathan Lange

Keywords: review added

The branch adds a bunch of unit tests for Deferred, particularly for some thorny cases around Deferreds that have callbacks that return Deferreds that themselves are buried somewhere deep in the callback chain.

The branch also records chaining explicitly, thus allowing us to test and improve the Deferred repr(), which makes debugging slightly easier.

comment:3 Changed 8 years ago by Jonathan Lange

Owner: Jonathan Lange deleted

comment:4 Changed 8 years ago by jesstess

Owner: set to jesstess

comment:5 Changed 8 years ago by jesstess

Cc: jesstess added
Keywords: review removed
Owner: changed from jesstess to Jonathan Lange

Thanks for the well-documented test cases - they are so clear we could be pointing people looking for more examples of using Deferreds to these. A few comments:

  • copyright bump on test_defer.py
  • I'm having trouble parsing the test_asynchronousImplicitErrorChain docstring. I think an article and some commas are missing?
  • in test_nestedAsynchronousChainedDeferredsWithExtraCallbacks: "Create a synchronous Deferreds" ==> "Create a synchronous Deferred"
  • the test_reprWithChaining docstring has the docstring of the test above it but is about waiting and not results.
  • L{Deferred} markup disappears after test_nestedAsynchronousChainedDeferredsWithExtraCallbacks, if you want that consistency.
  • in defer.py: the Deferred class docstring references doc/howto/defer.html, which I don't believe exists anymore.

Other than that, musing on defer.py for a while I couldn't come up with basic cases that were missing coverage - looks good to merge.

comment:6 Changed 8 years ago by Jonathan Lange

(In [28478]) Respond to jesstess's comments (refs #4300)

comment:7 Changed 8 years ago by Jonathan Lange

Resolution: fixed
Status: newclosed

(In [28480]) Merge deferred-unit-tests-4300

  • Author: jml, exarkun
  • Reviewer: jesstess
  • Fixes #4300

Add a bunch of tests and a couple of debugging affordances to Deferred.

comment:8 Changed 8 years ago by Jean-Paul Calderone

Resolution: fixed
Status: closedreopened

Reverted in r28814:

Revert r28480 (merge of deferred-unit-tests-4300)

The change to Deferred.__str__ made the CPU and memory requirements for this method (at least) quadratic on the length of the Deferred chain. This makes it impossible to compute the string representation for many Deferreds, and unreasonably expensive for many others.

comment:9 Changed 7 years ago by Jean-Paul Calderone

(In [29554]) Adjust the new repr to only followed a Deferred chain one link

refs #4300

comment:10 Changed 7 years ago by Jean-Paul Calderone

Keywords: review added
Owner: Jonathan Lange deleted
Status: reopenednew

I changed the new repr code to only go one step in the chain. It's probably possible to actually render the full chain in only O(N) time, but I didn't bother. We can try that out at some future point. I mainly want to get this merged before #411 because of the extra test coverage.

I made a few other minor changes, but nothing too surprising. I think the branch looks good overall now (as it did before, with just the minor issue of the repr performance). I'd like another set of eyes though, since I just made some changes and I also paired with jml a bit when the branch was first being worked on.

comment:11 Changed 7 years ago by therve

Cc: therve added
Keywords: review removed
Owner: set to Jean-Paul Calderone
  • Please make a full sentence in the NEWS fragment.
  • flake: twisted/test/test_defer.py:11: 'util' imported but unused
  • It seems test_asynchronousImplicitErrorChain should return a Deferred, considering what's said in the docstring.
  • test_doubleAsynchronousImplicitChaining ought to have a better docstring.
  • the comments of _NO_RESULT and _chainedTo can probably be moved in the module and class docstring, respectively.

Nothing major, +1!

comment:12 Changed 7 years ago by Jean-Paul Calderone

(In [29805]) Expand the test_doubleAsynchronousImplicitChaining docstring

refs #4300

comment:13 Changed 7 years ago by Jean-Paul Calderone

(In [29806]) Change the docstring for test_asynchronousImplicitErrorChain to make it clear that gc interaction is not being tested directly here

refs #4300

comment:14 Changed 7 years ago by Jean-Paul Calderone

(In [29807]) Document _NO_RESULT like anything else

refs #4300

comment:15 Changed 7 years ago by Jean-Paul Calderone

Please make a full sentence in the NEWS fragment.

Since it was a misc file, it should have been empty. So I emptied it.

flake: twisted/test/test_defer.py:11: 'util' imported but unused

Oops. It looks like this was introduced when Deferred.setTimeout was removed. I think I'll leave it alone for now because it's still used in the version of defer.py in the branch, so removing it would complicate testing.

It seems test_asynchronousImplicitErrorChain should return a Deferred, considering what's said in the docstring.

I adjusted the docstring. The gc interaction is tested elsewhere, so this test was claiming to be a little broader than it needed to be. It verifies that the result of the deferred is None, not a failure, which seems good enough to me.

test_doubleAsynchronousImplicitChaining ought to have a better docstring.

But it's so complete, concise, and expressive! It's the best docstring ever! Yarhh, fine, I expanded it a bit.

the comments of _NO_RESULT and _chainedTo can probably be moved in the module and class docstring, respectively.

Hm. <IRC conversation>. Okay. The conclusion is that docstrings are better than comments because tools (like pydoctor) can deal with them. And we document everything else in docstrings. So, fixed.

comment:16 Changed 7 years ago by Jean-Paul Calderone

Author: jmlexarkun, jml
Branch: branches/deferred-unit-tests-4300branches/deferred-unit-tests-4300-2

(In [29808]) Branching to 'deferred-unit-tests-4300-2'

comment:17 Changed 7 years ago by Jean-Paul Calderone

(In [29811]) Document _chainedTo

refs #4300

comment:18 Changed 7 years ago by Jean-Paul Calderone

Woops, I needed to merge forward anyway to get a clean test run, so I fixed that pyflakes warning after all.

comment:19 Changed 7 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [29812]) Merge deferred-unit-tests-4300-2

Author: jml, exarkun Reviewer: jesstess, therve Fixes: #4300

Add a bunch of tests and a couple of debugging affordances to Deferred. This re-merge fixes a quadratic complexity issue with the new __repr__ for Deferred. Only a single step of chaining is now represented in the repr string for a Deferred.

comment:20 Changed 7 years ago by <automation>

Owner: Jean-Paul Calderone deleted
Note: See TracTickets for help on using tickets.