twisted.trial.test.test_test_visitor emits deprecation warnings

[Jean-Paul Calderone]

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:~$ 

[kelly]

Patch to suppress the warnings.

[kelly]

Patch to suppress the warnings.

[Jonathan Lange]

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.

[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.

[Jean-Paul Calderone]

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

[kelly]

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

[Jean-Paul Calderone]

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

[Thijs Triemstra]

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)

[Vladimir Perić]

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.

[Vladimir Perić]

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.

[therve]

[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!

[Vladimir Perić]

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

[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.

[Vladimir Perić]

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.

[therve]

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

[Tom Prince]

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

[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

[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.

[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

[therve]

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.

2. 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.

3. DryRunMixin has a suppressed warning, I think it's not needed anymore.

Please merge once fixed!

[Tom Prince]

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

[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.

[Tom Prince]

(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.