Opened 9 years ago

Closed 4 years ago

#3231 defect closed fixed (fixed)

twisted.trial.test.test_test_visitor emits deprecation warnings

Reported by: Jean-Paul Calderone Owned by: Tom Prince
Priority: normal Milestone:
Component: trial Keywords:
Cc: Thijs Triemstra Branch: branches/test-visitor-removal-3231-3
branch-diff, diff-cov, branch-cov, buildbot
Author: vperic, tomprince

Description

exarkun@boson:~$ trial twisted.trial.test.test_test_visitor
twisted.trial.test.test_test_visitor
  TestTestVisitor
    test_visitCase ... 
/home/exarkun/Projects/Twisted/branches/system-event-ordering-3198-2/twisted/trial/unittest.py:1047: exceptions.DeprecationWarning: Test visitors deprecated in Twisted 8.0
                                                    [OK]
    test_visitEmptySuite ... 
/home/exarkun/Projects/Twisted/branches/system-event-ordering-3198-2/twisted/trial/unittest.py:1210: exceptions.DeprecationWarning: Test visitors deprecated in Twisted 8.0
                                              [OK]
    test_visitNestedSuite ...                                              [OK]
    test_visitPyunitCase ... 
/home/exarkun/Projects/Twisted/branches/system-event-ordering-3198-2/twisted/trial/unittest.py:1337: exceptions.DeprecationWarning: Test visitors deprecated in Twisted 8.0
                                              [OK]
    test_visitPyunitSuite ...                                              [OK]
    test_visitSuite ...                                                    [OK]

-------------------------------------------------------------------------------
Ran 6 tests in 0.014s

PASSED (successes=6)
exarkun@boson:~$ 

Attachments (2)

suppress-test_test_visitor-warnings.patch (784 bytes) - added by kelly 8 years ago.
Patch to suppress the warnings.
remove-visitors.patch (12.0 KB) - added by Vladimir Perić 5 years ago.

Download all attachments as: .zip

Change History (29)

comment:1 in reply to:  description Changed 8 years ago by kelly

Patch to suppress the warnings.

Changed 8 years ago by kelly

Patch to suppress the warnings.

comment:2 Changed 8 years ago by kelly

Keywords: review added

comment:3 Changed 8 years ago by Jonathan Lange

Keywords: review removed
Owner: changed from Jonathan Lange to kelly

Thanks for looking into this. Rather than using suppress, could you please use TestCase.callDeprecated to call the visit code.

Also, it might actually be that the deprecation period on the visitor code has expired. I don't know how to figure this out, I'm afraid.

comment:4 in reply to:  3 Changed 8 years ago by kelly

Replying to jml:

Thanks for looking into this. Rather than using suppress, could you please use TestCase.callDeprecated to call the visit code.

Given that the purpose of the existing tests is to exercise the visitor code, adding assertions about warnings is expanding their purpose somewhat. Wouldn't it be better to add some separate tests to confirm the deprecation warnings and then suppress those warnings for the other tests?

Also, it might actually be that the deprecation period on the visitor code has expired. I don't know how to figure this out, I'm afraid.

Looks like it was deprecated in r23027 as part of the 8.0.0 release which appears to have been released on 25-Mar-2008. That would make this deprecation 1 year 7 months old and almost 1 major version (next release is 9.0.0?).

As far as I can tell from the deprecation discussion, the current policy is:

Every deprecation must last for a minimum of 2 releases and a minimum of one year. Of course, anyone who wants to keep maintaining deprecated APIs for longer than that may do so.

I assume 8.1 and 8.2 count as releases under this scheme? In which case it would be eligible for removal.

comment:5 Changed 8 years ago by Jean-Paul Calderone

I think that --dry-run is still implemented in terms of this feature, which makes it somewhat more challenging to remove.

comment:6 in reply to:  5 Changed 8 years ago by kelly

I've opened a ticket #4063 that replaces --dry-run's use of the visitor code.

comment:7 Changed 7 years ago by Jean-Paul Calderone

Cool. That's taken care of, so we can proceed with this.

comment:8 Changed 6 years ago by Thijs Triemstra

Cc: Thijs Triemstra added

I can't reproduce this with 11.0+?

bin/trial twisted.trial.test.test_test_visitor
twisted.trial.test.test_test_visitor
  TestTestVisitor
    test_visitCase ...                                                     [OK]
    test_visitEmptySuite ...                                               [OK]
    test_visitNestedSuite ...                                              [OK]
    test_visitPyunitCase ...                                               [OK]
    test_visitPyunitSuite ...                                              [OK]
    test_visitSuite ...                                                    [OK]

-------------------------------------------------------------------------------
Ran 6 tests in 0.011s

PASSED (successes=6)

comment:9 in reply to:  8 Changed 5 years ago by Vladimir Perić

Keywords: review added

Replying to thijs:

I can't reproduce this with 11.0+?

This is probably because you are running Python 2.7, which silences DeprecationWarnings by default. They are still there, however, so I'm attaching a patch to remove the methods (they show when running "python -3" which is mildly annoying).

This is my first time deleting some deprecated methods, hope I did everything ok.

Changed 5 years ago by Vladimir Perić

Attachment: remove-visitors.patch added

comment:10 Changed 5 years ago by Vladimir Perić

Author: vperic

comment:11 Changed 5 years ago by Vladimir Perić

Owner: kelly deleted

It occurs to me now I forgot to clear the "owner" field when I submitted this for review. I'll make a branch out of it yet, but the .patch file should be enough for an initial review I hope.

comment:12 Changed 5 years ago by therve

Keywords: review removed
Owner: set to Vladimir Perić

[1] Some unused imports:

twisted/trial/test/test_pyunitcompat.py:12: 'reflect' imported but unused
twisted/trial/test/test_pyunitcompat.py:14: 'util' imported but unused

Please add a NEWS fragment and merge!

comment:13 Changed 5 years ago by Vladimir Perić

Branch: branches/test-visitor-removal-3231

(In [34578]) Create branch test-visitor-removal-3231

comment:14 Changed 5 years ago by Vladimir Perić

Ok, removed the two unused imports and moved everything to a branch. Buildbot is running, build results here. Will merge if everything is ok.

comment:15 Changed 5 years ago by Vladimir Perić

Keywords: review added
Owner: Vladimir Perić deleted

Hm, while fixing some docs errors, I noticed two more classes which have also been deprecated since 8.0, but haven't been tested (so didn't trigger any warnings). I've also removed them, but I guess this should go back on review. Note that one of these wasn't deprecated in the official way, but just in the docstring. Still, if no one noticed in 4 years, I guess it's not that important. :) Both of these classes were just concerned with maintaining compatibility between Python 2.3-2.5, which are no longer even supported.

comment:16 Changed 5 years ago by therve

Keywords: review removed
Owner: set to Vladimir Perić

It still looks good. You should create a fragment for #5554 and close it at the same point you're merging this.

comment:17 Changed 4 years ago by Tom Prince

Author: vpericvperic, tomprince
Branch: branches/test-visitor-removal-3231branches/test-visitor-removal-3231-2

(In [36848]) Branching to test-visitor-removal-3231-2

comment:18 Changed 4 years ago by Tom Prince

I've merged this forward, but haven't looked closely to make sure everything is still sane, due to the split of twisted.trial.unittest.

build results

comment:19 Changed 4 years ago by Julian Berman

Hi.

This looks good, but as per exarkun's comment, I think twisted.trial.runner.DryRunVisitor also should be removed here since #4063 is done.

comment:20 Changed 4 years ago by Tom Prince

Owner: changed from Vladimir Perić to Tom Prince

comment:21 Changed 4 years ago by Tom Prince

Owner: changed from Tom Prince to Vladimir Perić

comment:22 Changed 4 years ago by Tom Prince

Note: This is removing the old adaptors, which stopped being used in #2898. There are still new adaptors that are getting used.

DryRunVisitor can't be removed, since it hasn't been deprecated. (I filled #6333)

build results

comment:23 Changed 4 years ago by Tom Prince

Keywords: review added
Owner: Vladimir Perić deleted

comment:24 Changed 4 years ago by therve

Keywords: review removed
Owner: set to Tom Prince
  1. I have 2 tests failures in trunk (twisted.trial.test.test_loader.LoaderTest.test_loadByNamesPreservesOrder and

twisted.trial.test.test_loader.ZipLoadingTest.test_loadByNamesPreservesOrder) but I think it's easy fix, if you merge your branch forward.

  1. It seems DryRunVisitor is not used anymore. I wonder if we could remove it? It's public, but I don't see what people could do with it. Otherwise open a ticket to deprecate it.
  1. DryRunMixin has a suppressed warning, I think it's not needed anymore.

Please merge once fixed!

comment:25 Changed 4 years ago by Tom Prince

Branch: branches/test-visitor-removal-3231-2branches/test-visitor-removal-3231-3

(In [37465]) Branching to test-visitor-removal-3231-3.

comment:26 Changed 4 years ago by Tom Prince

  1. fixed
  2. #6333
  3. That is fallout from #4063.

As per glyphs comment on #6333, I've also submitted #6363 for review, so I can point to a replacement in the topfile here.

comment:27 Changed 4 years ago by Tom Prince

Resolution: fixed
Status: newclosed

(In [37594]) Merge test-visitor-removal-3231-3: Remove visitor support for trial.

Author: kelly, vperic, tom.prince Reviewers: jml, therve Fixes: #3231

twisted.trial.runner.suiteVisit and PyUnitTestCase as well as visitor methods, all deprecated since Twisted 8.0, have been removed.

Note: See TracTickets for help on using tickets.