Opened 2 years ago

Closed 19 months ago

#5955 enhancement closed fixed (fixed)

Move inlineCallbacks test from twisted.test.test_failure to somewhere better (test_defer?)

Reported by: itamar Owned by: thijs
Priority: normal Milestone:
Component: core Keywords:
Cc: thijs Branch: branches/move-inlinecbtest-5955
(diff, github, buildbot, log)
Author: thijs Launchpad Bug:

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 20 months ago by thijs

  • Author set to thijs
  • Branch set to branches/move-inlinecbtest-5955

(In [37306]) Branching to 'move-inlinecbtest-5955'

comment:2 Changed 20 months ago by thijs

(In [37307]) move test, add news file. refs #5955

comment:3 Changed 20 months ago by thijs

  • Keywords review added

Forced a build.

  1. moved test to test_defer
  2. the test uses getDivisionFailure from test.test_failure. it's being imported now but it seems better to expose this elsewhere (or maybe duplicate)?
  3. 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: Changed 20 months ago by tom.prince

  • Keywords review removed
  • Owner set to thijs
  1. Rather than importing getDivisionFailure, you can probably just duplicate it as a local function.
  2. The test should use failureResultOf to extract the failure from the deferred.
  3. 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 in reply to: ↑ 4 Changed 20 months ago by thijs

  • Cc thijs added
  • Keywords review added
  • Owner thijs deleted

Replying to tom.prince:

  1. Rather than importing getDivisionFailure, you can probably just duplicate it as a local function.

Done.

  1. The test should use failureResultOf to extract the failure from the deferred.

Fixed.

  1. 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 19 months ago by tom.prince

  • Keywords review removed
  • Owner set to thijs

This looks good. Please merge.

  1. 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 19 months ago by thijs

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

(In [37859]) Merge move-inlinecbtest-5955: Move test_inlineCallbacksTracebacks from test_failure to test_defer.

Author: thijs
Reviewer: tom.prince
Fixes: #5955

Note: See TracTickets for help on using tickets.