Opened 14 years ago
Closed 9 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)
Change History (29)
comment:1 Changed 13 years ago by
Changed 13 years ago by
Attachment: | suppress-test_test_visitor-warnings.patch added |
---|
Patch to suppress the warnings.
comment:2 Changed 13 years ago by
Keywords: | review added |
---|
comment:3 follow-up: 4 Changed 13 years ago by
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 Changed 13 years ago by
Replying to jml:
Thanks for looking into this. Rather than using
suppress
, could you please useTestCase.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 follow-up: 6 Changed 13 years ago by
I think that --dry-run is still implemented in terms of this feature, which makes it somewhat more challenging to remove.
comment:6 Changed 13 years ago by
I've opened a ticket #4063 that replaces --dry-run's use of the visitor code.
comment:8 follow-up: 9 Changed 11 years ago by
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 Changed 10 years ago by
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 10 years ago by
Attachment: | remove-visitors.patch added |
---|
comment:10 Changed 10 years ago by
Author: | → vperic |
---|
comment:11 Changed 10 years ago by
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 10 years ago by
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 10 years ago by
Branch: | → branches/test-visitor-removal-3231 |
---|
(In [34578]) Create branch test-visitor-removal-3231
comment:14 Changed 10 years ago by
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 10 years ago by
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 10 years ago by
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 9 years ago by
Author: | vperic → vperic, tomprince |
---|---|
Branch: | branches/test-visitor-removal-3231 → branches/test-visitor-removal-3231-2 |
(In [36848]) Branching to test-visitor-removal-3231-2
comment:18 Changed 9 years ago by
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
.
comment:19 Changed 9 years ago by
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 9 years ago by
Owner: | changed from Vladimir Perić to Tom Prince |
---|
comment:21 Changed 9 years ago by
Owner: | changed from Tom Prince to Vladimir Perić |
---|
comment:22 Changed 9 years ago by
comment:23 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | Vladimir Perić deleted |
comment:24 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Tom Prince |
- 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.
- 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.
- DryRunMixin has a suppressed warning, I think it's not needed anymore.
Please merge once fixed!
comment:25 Changed 9 years ago by
Branch: | branches/test-visitor-removal-3231-2 → branches/test-visitor-removal-3231-3 |
---|
(In [37465]) Branching to test-visitor-removal-3231-3.
comment:26 Changed 9 years ago by
comment:27 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
Patch to suppress the warnings.