Opened 7 years ago
Closed 6 years ago
#6220 enhancement closed fixed (fixed)
Replace Deprecated Assertion Methods in twisted.conch
Reported by: | Julian Berman | Owned by: | Julian Berman |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | conch | Keywords: | |
Cc: | z3p | Branch: |
branches/remove-deprecated-test-methods-conch-6220-3
branch-diff, diff-cov, branch-cov, buildbot |
Author: | julian, tomprince, rwall |
Description
This (and succeeding tickets) are breaking up #5771
Attachments (4)
Change History (38)
comment:1 Changed 7 years ago by
Cc: | z3p added |
---|
Changed 7 years ago by
Attachment: | conch-deprecate.patch added |
---|
comment:2 Changed 7 years ago by
comment:3 Changed 7 years ago by
Keywords: | review removed |
---|
comment:4 Changed 7 years ago by
Owner: | set to Julian Berman |
---|
comment:5 Changed 7 years ago by
Keywords: | review added |
---|
Attached updated patch should cover all the deprecations.
Also, if it helps reviewers, I'm using the following (vimscript) to do the substitutions, manually confirming each one so that I get a quick view of the surrounding lines to try and make sure on each one that things look OK:
%s/assertAlmostEquals(/assertAlmostEqual(/ec %s/assertEquals(/assertEqual(/ec %s/assertIdentical(/assertIs(/ec %s/assertNot(/assertFalse(/ec %s/assertNotAlmostEquals(/assertNotAlmostEqual(/ec %s/assertNotEquals(/assertNotEqual(/ec %s/assertNotIdentical(/assertIsNot(/ec %s/assertNotSubstring(/assertNotIn(/ec %s/assertSubstring(/assertIn(/ec %s/assert_(/assertTrue(/ec %s/failIf(/assertFalse(/ec %s/failIfEqual(/assertNotEqual(/ec %s/failIfEquals(/assertNotEqual(/ec %s/failIfIdentical(/assertIsNot(/ec %s/failIfIn(/assertNotIn(/ec %s/failUnless(/assertTrue(/ec %s/failUnlessFailure(/assertFailure(/ec %s/failUnlessIdentical(/assertIs(/ec %s/failUnlessIn(/assertIn(/ec %s/failUnlessRaises(/assertRaises(/ec
Changed 7 years ago by
Attachment: | conch-deprecate-v2.patch added |
---|
comment:6 Changed 7 years ago by
Owner: | Julian Berman deleted |
---|
comment:7 Changed 7 years ago by
Owner: | set to Jean-Paul Calderone |
---|---|
Status: | new → assigned |
comment:8 Changed 7 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Jean-Paul Calderone to Julian Berman |
Status: | assigned → new |
Thanks. Even limited to just Conch, I definitely found my eyes glazing over by the end of this review.
- Hm. I'm not sure
assertIdentical
is actually on its way to deprecation. I guess we could decide we don't want to keep it as an alternate spelling ofassertIs
. So, fine, I guess. - Throughout, some changes to which method is being called also changes how nearby lines need to be indented. For code like
foo(bar\nbaz)
,baz
should start in the same column asbar
. - Also throughout, test methods without docstrings are being modified. Ideally, these would gain docstrings as part of this maintenance. Normally this makes sense because to modify a test method, you really have to already understand what it is trying to do. That argument is perhaps somewhat less strong in this case, where the changes don't really require a very deep understanding of the code. However, it would still be nice...
- in
twisted/conch/test/test_checkers.py
, it seems likeassertLoggedIn
would benefit from using the newly introducedsuccessResultOf
. - in
twisted/conch/test/test_ssh.py
, some uses offailIf
andfailUnless
should have beenassertIn
andassertNotIn
and it probably makes sense to change them to use those methods now, rather than just switching them toassertTrue
andassertFalse
. - Similarly, in
twisted/conch/test/test_cftp.py
somefailIf
andfailUnless
should beassertEqual
andassertNotEqual
, or perhaps evenassertSubstring
.
comment:9 Changed 7 years ago by
Hey, thanks.
I'm generally trying to apply the rule "one spelling for every assertion" which I've found mentioned in the other tickets and which makes sense to me.
Note that you also missed that I am considering assertSubstring
and friends to be deprecated in favor of assertIn
as to me that is one assertion (and is the way I write it when writing test cases). I guess that deserved pointing out since ... let's see if it requires discussion :).
Regarding the rest of the feedback, yeah I've noticed a lot of tests that use non-rich assertions. There are assert_(whatever == None) all over the code base. I was consciously trying to avoid changing any of these. I think it's valuable to change them, since they provide better error messages, but Twisted as-is hides some of those nicer messages anyhow, and I think that's beyond the goal of this ticket (and friends), which isn't really to generally improve the assertions around the test suite, but to just allow the deprecations to move forward. I'd love for that to cover the requirement for adding docstrings too :P. Indentation is a harder case to make I guess, though I also noticed those.
So essentially I agree with everything you said but am hoping, with the exception of indentation I guess, not to need to do it along with these sets of changes (though ultimately if this goes smoothly I'd like to do that as well). Do you disagree with any of that?
comment:10 Changed 7 years ago by
I'm generally trying to apply the rule "one spelling for every assertion" which I've found mentioned in the other tickets and which makes sense to me.
Okay, that sounds good.
Note that you also missed that I am considering assertSubstring and friends to be deprecated in favor of assertIn as to me that is one assertion (and is the way I write it when writing test cases). I guess that deserved pointing out since ... let's see if it requires discussion :).
Okay. Please note that somewhere - either in the development documentation, or on the ticket for actually deprecating the functions we're going to deprecate, or both.
Regarding the rest of the feedback, yeah I've noticed a lot of tests that use non-rich assertions. There are assert_(whatever == None) all over the code base. I was consciously trying to avoid changing any of these.
Yes, please do avoid that. I should have been more specific, my comments about switching to more precise assertion methods was only intended to be for the assertions you're already changing in this branch. If you change failIf(x == y)
, it seems like it makes sense to change it to assertNotEqual(x, y)
, not assertFalse(x == y)
. Otherwise next week or whenever someone is just going to have to visit that line again.
I'd love for that to cover the requirement for adding docstrings too :P.
I think you might get away without adding any docstrings, but I also think the patch would be much better with them, despite the extra length it will add.
comment:11 Changed 7 years ago by
Keywords: | review added |
---|---|
Owner: | Julian Berman deleted |
OK.
So here's what I've done: as of now I think I'll opt to try and improve the assertions for lines I change but not add docstrings, that requires too much of a different style of thinking.
The latest patch does that for this series of changes (conch).
I've documented the deprecated tests and the to-be-accepted new incantations in #4771
Changed 7 years ago by
Attachment: | conch-deprecate-v3.patch added |
---|
comment:13 Changed 7 years ago by
Owner: | set to Tom Prince |
---|
comment:14 Changed 7 years ago by
Author: | → tomprince |
---|---|
Branch: | → branches/remove-deprecated-test-methods-conch-6220 |
(In [37283]) Branching to remove-deprecated-test-methods-conch-6220.
comment:16 Changed 7 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Tom Prince to Julian Berman |
Thanks for your work on this.
- Please change
UNIXPasswordDatabaseTests.assertLoggedIn
to usesuccessResultOf
, as suggested by exarkun. - There are a number of places where you rewrapped assertions that hides the parallel nature of the assertion. For example
I think it is easier to see what is being tested in the previous version.
- self.failIfEquals(proto.outgoingPacketSequence, - proto2.outgoingPacketSequence) - self.failIfEquals(proto.incomingPacketSequence, - proto2.incomingPacketSequence) - self.failIfEquals(proto.currentEncryptions, - proto2.currentEncryptions) + self.assertNotEqual( + proto.outgoingPacketSequence, proto2.outgoingPacketSequence) + self.assertNotEqual( + proto.incomingPacketSequence, proto2.incomingPacketSequence) + self.assertNotEqual( + proto.currentEncryptions, proto2.currentEncryptions)
- In
test_insults
,PrintableCharacters.verifyResults
: based on the message, theassertFalse
you changed should probably beassertEqual(..., [])
instead.- I think there are a bunch more occurences of the same check that could be changed. On the other hand, all the tests in this module look to be using mocks. So it probably isn't worth fixing this up.
Please fix 1+2 and file a ticket for 3. Then submit a patch against the branch and put in review.
comment:17 Changed 7 years ago by
Hi Tom thanks for the quick review.
Re: 1: it's not on a line being modified in this ticket (nor is it part of the general objective we're trying to get done here which is just remove assertions and replace them with their new canonical name so that we can deprecate and move towards unittest2 assertions). There's already plenty of work to do there, so I think (and last we spoke I believe exarkun agreed at the end) that we should try to stick to just improving in those places, but when doing so try to use the best fitting assertion, which is what I've tried to do.
Re 2: I saw this while changing. The style guide doesn't seem to say anything about wrapping more than PEP8 does and so far all the test methods I looked at were rather haphazard about which they preferred. I don't care either way but if there is such a preference in cases like this maybe we should add it to the style guide? I don't personally care either way more than to say that I picked this way since it would have made all the diffs here a tiny bit nicer since it wouldn't have reindented all the second lines. I'll make that change though here and then put up for review again.
And OK I'll open a ticket for 3.
comment:18 Changed 7 years ago by
Re 1: I'm fine leaving it for a follow up ticket (but do file it in that case). But, changing it does involve modifying both the assertions in that method (removing one in the process).
Re 2: I'm not sure that there is anything concrete enough to put in the style-gude. The preference is for clear and easy to understand code. I did debate mentioning this, and some of the places with the new style are no more or less clear as what is currently there and can stay as you changed them. It just the case where there is clear parallelism that is lost with new wrapping that I think needs to be changed.
comment:19 Changed 7 years ago by
Note twisted doesn't actually have asssertIs
(see build results).
comment:20 Changed 7 years ago by
Ugh I was trying to specifically avoid that (I specifically avoided assertIsNone for that reason) but apparently some slipped through. I'm thinking then that the best first step might be to add all the unittest2 assertions to TestCase (if they're not present due to being on 2.6) and then come back to these. I'll take a closer look on tuesday.
comment:21 Changed 7 years ago by
I think it would make more sense to do all the existing changes, before adding yet another method to change.
comment:22 Changed 7 years ago by
Well. The plan is to make only a single pass over the test suite, and when doing so it'd be nice if the best possible assertion methods were present and usable while making these changes.
comment:23 Changed 7 years ago by
You can only make the best possible change at a particular point in time. Other parts of Twisted should always be improving, so even if you wait for assertIs
, by then someone will have had an idea for another even cooler improvement which you may then decide to wait for, and once that is done someone will have had another idea for another even cooler improvement...
There's nothing fundamentally interesting about the assertIdentical/assertIs
change, at least not compared to the rest of the renaming being done. If you figure out how to do the rest of the renaming and get it accepted, then it shouldn't be hard to make a future pass for assertIdentical/assertIs
.
So I don't think you should be very worried about making these changes and then later making the assertIdentical/assertIs
change. Of course, you could probably introduce the necessary assertIs
method into trial in about two minutes...
comment:24 Changed 7 years ago by
The list of "cool" improvements is well defined: it's the list of all the assertions present in 2.7+ (unittest2).
There's nothing better about assertIs
over assertIdentical
no, but first adding all the unittest2 assertions and then doing all these would mean only needing to look at everything once.
I think there should only be ~8 or so missing completely from trial's TestCase
right now. If it's easy enough which I expect it will be it's just easier to add them. I.e. I'm thinking of doing:
- Add all unittest2 assertions
- Rename usages of to-be-deprecated assertions in the suite to the ones in unittest2
- Deprecate old assertions
in that order. Maybe even aiming for full unittest2 compatibility before getting these merged is worthwhile but that will probably be a bit more work. This ticket though is nearly done so I'll just open a ticket adding assertIs
, then we can come back to this, then I'll go back to 1.
comment:25 Changed 7 years ago by
After #6350 lands with assertIs, consider this up for review (I'll come back to add the keyword). Here's a patch fixing 2.
1 is in 6351.
Changed 7 years ago by
Attachment: | conch-deprecate-v4.patch added |
---|
comment:26 Changed 7 years ago by
The list of "cool" improvements is well defined ... Maybe even aiming for full unittest2 compatibility before getting these merged is worthwhile
See, while writing this comment you had an idea for a further improvement. :) I could hardly make a more convincing argument than that.
comment:27 Changed 7 years ago by
Keywords: | review added |
---|---|
Owner: | Julian Berman deleted |
comment:28 Changed 7 years ago by
Owner: | set to Richard Wall |
---|---|
Status: | new → assigned |
comment:29 Changed 7 years ago by
Author: | tomprince → tomprince, rwall |
---|---|
Branch: | branches/remove-deprecated-test-methods-conch-6220 → branches/remove-deprecated-test-methods-conch-6220-2 |
(In [38320]) Branching to 'remove-deprecated-test-methods-conch-6220-2'
comment:30 Changed 7 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Richard Wall to Julian Berman |
Status: | assigned → new |
Code Review
Thanks Julian.
Notes:
- The last patch (conch-deprecate-v4.patch) had already been applied to log:branches/remove-deprecated-test-methods-conch-6220 by tomprince.
- Builds still failing in original branch because it didn't contain new assertion methods:
- Merged forward. See log:branches/remove-deprecated-test-methods-conch-6220-2
- Added news file
- New Build Results look good:
- Inspected all the changes by eye and they all look right - although it is a rather large patch.
- I compared the coverage of conch in the branch and in trunk - they
are exactly the same - which is reassuring.
chbranch Twisted remove-deprecated-test-methods-conch-6220-2 cd branches/remove-deprecated-test-methods-conch-6220-2 coverage run --source=twisted.conch ./bin/trial twisted.conch.test coverage report > .coverage.txt # repeat for trunk then diff -u trunk/.coverage.txt branches/remove-deprecated-test-methods-conch-6220-2/.coverage.txt
Please address or answer the following points:
- I found the following unconverted conch test assertions - do these
need replacing?
[richard@zorin remove-deprecated-test-methods-conch-6220-2]$ ack '(failIf|failIfIdentical|assertIdentical|assertNotIdentical|assertNotEquals|assertEquals|assertSubstring|assert_)[^A-Za-z0-9]' twisted/conch/test twisted/conch/test/test_endpoints.py 433: self.assertNotIdentical(None, protocol.transport) 959: self.assertNotIdentical(None, protocol.transport) 997: self.assertNotIdentical(None, protocol.transport) 1133: self.assertIdentical( 1170: self.assertIdentical(result, helper.knownHosts) twisted/conch/test/test_helper.py 560: self.assertEquals(len(result), 1) 561: self.assertEquals(result[0].group(), "zoom") 575: self.assertEquals( 579: self.assertEquals( 583: self.assertEquals( 591: self.assertNotEquals( 595: self.assertNotEquals( 610: self.assertIdentical(warningsShown[0]['category'], DeprecationWarning) twisted/conch/test/test_text.py 21: self.assertEquals( 31: self.assertEquals( 41: self.assertEquals( 51: self.assertEquals( 61: self.assertEquals( 72: self.assertEquals( 83: self.assertEquals( 94: self.assertEquals( 113: self.assertIdentical(warningsShown[0]['category'], DeprecationWarning)
- branches/remove-deprecated-test-methods-conch-6220-2/twisted/conch/test/test_transport.py
- There are still some examples of argument indentation changes. Personally, I prefer the new indentation, but both exarkun and tomprince asked for the arguments to start at the same column, so I guess you should change that in this and other changed files.
Please submit a new patch against the new branch for what will hopefully be a final review.
-RichardW.
comment:31 Changed 6 years ago by
Keywords: | review added |
---|---|
Owner: | Julian Berman deleted |
I think those assertions seem to be from newer commits (post patch). I've fixed them though, along with tidying some more indentation.
comment:32 Changed 6 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Julian Berman |
This looks good, please merge.
comment:33 Changed 6 years ago by
Author: | tomprince, rwall → julian, tomprince, rwall |
---|---|
Branch: | branches/remove-deprecated-test-methods-conch-6220-2 → branches/remove-deprecated-test-methods-conch-6220-3 |
(In [38988]) Branching to remove-deprecated-test-methods-conch-6220-3.
comment:34 Changed 6 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
(In [38990]) Merge remove-deprecated-test-methods-conch-6220-3: Replace to-be-deprecated assertions in Conch
Author: Julian Reviewers: tom.prince, exarkun, rwall Fixes: #6220
Replace older test method assertions like fail* (which are soon to be deprecated) in twisted.conch's test suite with their newer equivalents.
There are a couple of methods to be deprecated that haven't been changed. At least
assertIdentical
,assertNotIdentical
,assertNotEquals
andassertSubstring
.