Opened 7 years ago
Closed 7 years ago
#5955 enhancement closed fixed (fixed)
Move inlineCallbacks test from twisted.test.test_failure to somewhere better (test_defer?)
Reported by: | Itamar Turner-Trauring | Owned by: | Thijs Triemstra |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | core | Keywords: | |
Cc: | Thijs Triemstra | Branch: |
branches/move-inlinecbtest-5955
branch-diff, diff-cov, branch-cov, buildbot |
Author: | thijs |
Description
Importing twisted.internet
in a twisted.python
test module is not ideal. The tests involving inlineCallbacks
in test_failure
should probably be moved somewhere else, e.g. the relevant test_defer
.
Change History (7)
comment:1 Changed 7 years ago by
Author: | → thijs |
---|---|
Branch: | → branches/move-inlinecbtest-5955 |
comment:3 Changed 7 years ago by
Keywords: | review added |
---|
Forced a build.
- moved test to test_defer
- the test uses
getDivisionFailure
fromtest.test_failure
. it's being imported now but it seems better to expose this elsewhere (or maybe duplicate)? - moved the py3 todo statement and added notes about #6008 being a unittest todo support ticket, found it confusing with the #5949 right above it..
Up for review.
comment:4 follow-up: 5 Changed 7 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Thijs Triemstra |
- Rather than importing
getDivisionFailure
, you can probably just duplicate it as a local function. - The test should use
failureResultOf
to extract the failure from the deferred. - The assertion could be made much stronger (and match the docstring) if the entire extracted traceback was compared to the original traceback.
Please resubmit for review after addressing the above points.
comment:5 Changed 7 years ago by
Cc: | Thijs Triemstra added |
---|---|
Keywords: | review added |
Owner: | Thijs Triemstra deleted |
Replying to tom.prince:
- Rather than importing
getDivisionFailure
, you can probably just duplicate it as a local function.
Done.
- The test should use
failureResultOf
to extract the failure from the deferred.
Fixed.
- The assertion could be made much stronger (and match the docstring) if the entire extracted traceback was compared to the original traceback.
The assertion has been expanded, and while it's an improvement, i couldn't figure out how to get the original traceback to compare to, but it also feels this is more out of scope for this ticket.
Forced a new build.
comment:6 Changed 7 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Thijs Triemstra |
This looks good. Please merge.
- The assertion could be made much stronger (and match the docstring) if the entire extracted traceback was compared to the original traceback.
The assertion has been expanded, and while it's an improvement, i couldn't figure out how to get the original traceback to compare to, but it also feels this is more out of scope for this ticket.
I like the new assertions. It would be nice if there was some helpers for doing this, so that the reader doesn't need know how tracebacks look.
comment:7 Changed 7 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
(In [37306]) Branching to 'move-inlinecbtest-5955'