Opened 6 years ago

Closed 6 years ago

#6856 enhancement closed fixed (fixed)

Remove unused test case in twisted.test.test_reflect

Reported by: Jonathan Ballet Owned by: Hynek Schlawack
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/remove-dead-code-from-test_reflect-6856
branch-diff, diff-cov, branch-cov, buildbot
Author: hynek

Description

twisted.test.test_reflect.ImportHooksLookupTests is a test case which isn't used at the moment in Twisted code case and has a skip marker since r35441. It depends on the ihooks module which has been removed in Python 3.

This is something which should have been removed as part of #6689 but we somehow missed it. It actually raises a DeprecationWarning in #5929 if tests are executed with Python 2.x and the -3 flag.

I think it's pretty safe to remove this class and move forward on #5929 then.

Attachments (3)

6856-remove-unused-testcase-1.patch (1.7 KB) - added by Jonathan Ballet 6 years ago.
6856-remove-unused-testcase-2.patch (1.6 KB) - added by Jonathan Ballet 6 years ago.
Empty file and git :(
6856-remove-unused-testcase-3.patch (1.7 KB) - added by Jonathan Ballet 6 years ago.
Actually, I can track empty files with Git o/

Download all attachments as: .zip

Change History (10)

Changed 6 years ago by Jonathan Ballet

Changed 6 years ago by Jonathan Ballet

Empty file and git :(

comment:1 Changed 6 years ago by Jonathan Ballet

Keywords: review added

Changed 6 years ago by Jonathan Ballet

Actually, I can track empty files with Git o/

comment:2 Changed 6 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Jonathan Ballet

Hmm. I'm not sure I follow the logic in the ticket description. Perhaps you can expand on it somewhat?

I tried removing the skip from ImportHooksLookupTests and noticed that the tests all pass on Python 2.7. It seems to only be broken on Python 2.6. Perhaps some of the earlier porting work was overly zealous in introducing more skips - or perhaps someone has since fixed the ihooks bug in Python 2.7 (but skipped fixing it in 2.6?).

Regardless, considering that ihooks still exists on Python 2.6 and Python 2.7, what is the justification for deleting this test case? ihooks being removed in Python 3 doesn't seem to be sufficient. If I'm overlooking something, sorry - please correct me!

comment:3 in reply to:  2 Changed 6 years ago by Jonathan Ballet

[spinning my brain up to remember what was this thing about...]

Replying to exarkun:

Hmm. I'm not sure I follow the logic in the ticket description. Perhaps you can expand on it somewhat?

I tried removing the skip from ImportHooksLookupTests and noticed that the tests all pass on Python 2.7. It seems to only be broken on Python 2.6. Perhaps some of the earlier porting work was overly zealous in introducing more skips - or perhaps someone has since fixed the ihooks bug in Python 2.7 (but skipped fixing it in 2.6?).

Regardless, considering that ihooks still exists on Python 2.6 and Python 2.7, what is the justification for deleting this test case? ihooks being removed in Python 3 doesn't seem to be sufficient. If I'm overlooking something, sorry - please correct me!

So, I clearly underlooked this test case, since it actually has tests attached to it by subclassing LookupsTestCase from twisted.python.test.test_reflectpy3, which for some reasons I missed.

I also tried to remove the skip() attributes from the class, and tests pass both on Python 2.6 and Python 2.7...

So. What should I do with this?

  • I can let the skip attribute, and everything "is" fine on all Python versions, although we don't know if it actually works. We'll still have the warning that ihooks has been deprecated in Python 3.0 if the tests are run from Python 2.7 with -3 (which was the reason for creating this ticket in the first place, see https://twistedmatrix.com/trac/ticket/5929#comment:32 ) and this is basically just dead code (what's the point of keeping it if it's always skipped?)
  • I can remove the skip attribute on Python 2.x, since it seems to work, and add it only on Python 3.x if ihooks isn't available. We'll still have the aforementioned warning (which is supposed to be on the review checklist for Python 3 patches), but at least this test will have some meaning where there are executed.

If I go the first way, I'll just close this ticket and put a comment back into #5929 which refers to here. Otherwise, if I go with the second way, I guess I'll make one more patch on #5929 to add this compatibility check (unless I'm being suggested otherwise).

Thoughts?

comment:4 Changed 6 years ago by Jonathan Ballet

Keywords: review added

comment:5 Changed 6 years ago by Hynek Schlawack

Keywords: review removed
Owner: changed from Jonathan Ballet to Hynek Schlawack

So let’s wrap this up a bit:

  1. ihooks are completely undocumented, the only docs on it I could found is the list of undocumented modules (I didn’t know of their existence until I read through this ticket). I don’t see any value in testing explicitly compatibility with an obscure and ostensibly buggy stdlib module.
  2. It apparently is flaky enough to fail on 2.6 for one of you, but to pass for the other. Because of 1., I have no reason to assume that it won’t exercise any flakiness on 2.7 too. Flaky tests are the worst; the headache from Win32 buildbots is bad enough.
  3. This test is dead code ATM, functioning at best as a reminder to not use ihooks. I don’t think that’s the job of a test case. I would suggest to add documentation but ihooks is just too obscure to litter our docs with explicit docs about it.

Long story short, I don’t think this blob of dead code adds enough value to be useful to be had in Twisted; we have too much baggage code to care for already.

If itamar says ihooks is broken since 2.6 I would go and kill it off.

As for the patch, there is an empty line missing between the imports and the first class; no need to fix that, I’m mentioning it just to remind you of the Twisted Coding Standard[tm]. :)

You don’t need to do anything else unless more comments are added here or I got something wrong.

comment:6 Changed 6 years ago by Hynek Schlawack

Author: hynek
Branch: branches/remove-dead-code-from-test_reflect-6856

(In [41425]) Branching to remove-dead-code-from-test_reflect-6856.

comment:7 Changed 6 years ago by Hynek Schlawack

Resolution: fixed
Status: newclosed

(In [41429]) Merge remove-dead-code-from-test_reflect-6856

Author: multani Reviewer: hynek Fixes: #6856

twisted.test.test_reflect.ImportHooksLookupTests was skipped unconditionally because the rather obscure stdlib module ihooks was broken since 2.6. It's either dead code or a flaky test, so we removed it.

Note: See TracTickets for help on using tickets.