Opened 8 years ago

Closed 14 months ago

#2053 defect closed fixed (fixed)

twisted.test.test_import is redundant

Reported by: exarkun Owned by: lvh
Priority: normal Milestone:
Component: core Keywords:
Cc: therve, thijs Branch: branches/remove-test-import-2053
(diff, github, buildbot, log)
Author: lvh Launchpad Bug:

Description

We used to have pretty crudtastic test coverage. Having some tests that at least imported some modules that might not have been imported anyplace else was useful.

Is this still the case? If so, we seriously need to write some tests. If not, we should get rid of test_import, because it serves no purpose.

Attachments (1)

2503.patch (2.5 KB) - added by ewong 14 months ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 7 years ago by therve

  • Cc therve added

I had a quick look at this, and unfortunately there is at least one module inside test_import without unittests: twisted.python.otp. Maybe we can still remove these tests and create a ticket for adding tests to otp?

comment:2 Changed 4 years ago by <automation>

  • Owner exarkun deleted

comment:3 Changed 3 years ago by thijs

  • Cc thijs added

What's the decision on this ticket?

comment:4 follow-up: Changed 3 years ago by exarkun

twisted.python.otp is deprecated (since 9.0, it seems, though it misreports as 8.3). So we can get rid of that module and then presumably we can get rid of test_import, supposing that we have coverage for everything else it is importing.

comment:5 in reply to: ↑ 4 Changed 3 years ago by thijs

Replying to exarkun:

twisted.python.otp is deprecated (since 9.0, it seems, though it misreports as 8.3). So we can get rid of that module

I've opened #5493 for this.

comment:6 follow-up: Changed 14 months ago by ewong

I've gone through test_import.py, and found that twisted.spread.flavors doesn't have coverage.
(Assuming that I've done it right.) Do I remove the other tests and submit a patch, or do I just leave this as-is?

comment:7 in reply to: ↑ 6 Changed 14 months ago by ewong

Replying to ewong:

I've gone through test_import.py, and found that twisted.spread.flavors doesn't have coverage.
(Assuming that I've done it right.) Do I remove the other tests and submit a patch, or do I just leave this as-is?

Sorry. twisted.spread.flavors is covered.

comment:8 Changed 14 months ago by ewong

  • Owner set to ewong
  • Status changed from new to assigned

Changed 14 months ago by ewong

comment:9 Changed 14 months ago by ewong

  • Owner ewong deleted
  • Status changed from assigned to new

comment:10 Changed 14 months ago by ewong

  • Keywords review added

comment:11 Changed 14 months ago by lvh

  • Owner set to lvh

comment:12 Changed 14 months ago by lvh

  • Author set to lvh
  • Branch set to branches/remove-test-import-2053

(In [40587]) Branching to 'remove-test-import-2053'

comment:13 Changed 14 months ago by lvh

Oops. Sorry this took so long: the ticket is #2053, but the patch is called 2503, which confused me, svn, trac etc terribly :)

comment:14 Changed 14 months ago by lvh

  • Keywords review removed

Hi ewong,

  1. Your patch has the wrong number. That's not so bad.
  2. Your patch's topfile has the wrong number. I missed this, so I accidentally closed the wrong ticket, which made SVN/trac integration very unhappy when I tried to fix my mistake :)

Otherwise, this looks great. Buildbots are happy (modulo spurious windows failures that have nothing to do with this ticket) and I'm happy except for the part where I had to fight SVN ;)

Thanks again for your contribution

lvh

comment:15 Changed 14 months ago by lvh

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

(In [40593]) Merge remove-test-import-2053: remove test_import module

Author: ewong
Reviewer: lvh
Fixes: #2053

twisted.test.test_import was a vestigial module from back when Twisted had really bad test coverage. All it did was try to import (some of the) modules, and see if that raised any exceptions. It didn't really catch anything besides syntax errors or circular imports. Since all of the modules it imported have way better tests now, we can just go ahead and remove this.

Note: See TracTickets for help on using tickets.