Opened 4 years ago

Closed 4 years ago

#4300 task closed fixed (fixed)

Write some unit tests for Deferreds

Reported by: jml Owned by:
Priority: normal Milestone:
Component: core Keywords:
Cc: jesstess, therve Branch: branches/deferred-unit-tests-4300-2
(diff, github, buildbot, log)
Author: exarkun, jml Launchpad Bug:

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 4 years ago by jml

  • Author set to jml
  • Branch set to branches/deferred-unit-tests-4300

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

comment:2 Changed 4 years ago by jml

  • 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 4 years ago by jml

  • Owner jml deleted

comment:4 Changed 4 years ago by jesstess

  • Owner set to jesstess

comment:5 Changed 4 years ago by jesstess

  • Cc jesstess added
  • Keywords review removed
  • Owner changed from jesstess to jml

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 4 years ago by jml

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

comment:7 Changed 4 years ago by jml

  • Resolution set to fixed
  • Status changed from new to closed

(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 4 years ago by exarkun

  • Resolution fixed deleted
  • Status changed from closed to reopened

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 4 years ago by exarkun

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

refs #4300

comment:10 Changed 4 years ago by exarkun

  • Keywords review added
  • Owner jml deleted
  • Status changed from reopened to new

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 4 years ago by therve

  • Cc therve added
  • Keywords review removed
  • Owner set to exarkun
  • 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 4 years ago by exarkun

(In [29805]) Expand the test_doubleAsynchronousImplicitChaining docstring

refs #4300

comment:13 Changed 4 years ago by exarkun

(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 4 years ago by exarkun

(In [29807]) Document _NO_RESULT like anything else

refs #4300

comment:15 Changed 4 years ago by exarkun

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 4 years ago by exarkun

  • Author changed from jml to exarkun, jml
  • Branch changed from branches/deferred-unit-tests-4300 to branches/deferred-unit-tests-4300-2

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

comment:17 Changed 4 years ago by exarkun

(In [29811]) Document _chainedTo

refs #4300

comment:18 Changed 4 years ago by exarkun

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

comment:19 Changed 4 years ago by exarkun

  • Resolution set to fixed
  • Status changed from new to closed

(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 3 years ago by <automation>

  • Owner exarkun deleted
Note: See TracTickets for help on using tickets.