Replace Deprecated Assertion Methods in twisted.conch

[Julian Berman]

This (and succeeding tickets) are breaking up #5771

[Tom Prince]

There are a couple of methods to be deprecated that haven't been changed. At least assertIdentical, assertNotIdentical, assertNotEquals and assertSubstring.

[Julian Berman]

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

[Jean-Paul Calderone]

Thanks. Even limited to just Conch, I definitely found my eyes glazing over by the end of this review.

1. 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 of assertIs. So, fine, I guess.
2. 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 as bar.
3. 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...
4. in twisted/conch/test/test_checkers.py, it seems like assertLoggedIn would benefit from using the newly introduced successResultOf.
5. in twisted/conch/test/test_ssh.py, some uses of failIf and failUnless should have been assertIn and assertNotIn and it probably makes sense to change them to use those methods now, rather than just switching them to assertTrue and assertFalse.
6. Similarly, in twisted/conch/test/test_cftp.py some failIf and failUnless should be assertEqual and assertNotEqual, or perhaps even assertSubstring.

[Julian Berman]

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?

[Jean-Paul Calderone]

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.

[Julian Berman]

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

[Julian Berman]

Sorry. #4991.

[Tom Prince]

(In [37283]) Branching to remove-deprecated-test-methods-conch-6220.

[Tom Prince]

(In [37284]) Apply conch-deprecate-v3.patch.

Refs #6220.

[Tom Prince]

Thanks for your work on this.

1. Please change UNIXPasswordDatabaseTests.assertLoggedIn to use successResultOf, as suggested by exarkun.
2. There are a number of places where you rewrapped assertions that hides the parallel nature of the assertion. For example

   -        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)
   

   I think it is easier to see what is being tested in the previous version.
3. In test_insults, PrintableCharacters.verifyResults: based on the message, the assertFalse you changed should probably be assertEqual(..., []) 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.

[Julian Berman]

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.

[Tom Prince]

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.

[Tom Prince]

Note twisted doesn't actually have asssertIs (see ​build results).

[Julian Berman]

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.

[Tom Prince]

I think it would make more sense to do all the existing changes, before adding yet another method to change.

[Julian Berman]

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.

[Jean-Paul Calderone]

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

[Julian Berman]

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:

1. Add all unittest2 assertions
2. Rename usages of to-be-deprecated assertions in the suite to the ones in unittest2
3. 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.

[Julian Berman]

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.

[Jean-Paul Calderone]

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.

[Julian Berman]

Putting this back up now that #6350 landed. I'll get to #6351 for the successResultOf stuff as soon as I figure out how to use successResultOf (which I'm sure is fairly simple).

[Richard Wall]

(In [38320]) Branching to 'remove-deprecated-test-methods-conch-6220-2'

[Richard Wall]

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:
  • ​https://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/remove-deprecated-test-methods-conch-6220
• Merged forward. See log:branches/remove-deprecated-test-methods-conch-6220-2
• Added news file
• New Build Results look good:
  • ​http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/remove-deprecated-test-methods-conch-6220-2
• 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:

1. 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)
   
   

2. branches/remove-deprecated-test-methods-conch-6220-2/twisted/conch/test/test_transport.py
   1. 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.

[Julian Berman]

I think those assertions seem to be from newer commits (post patch). I've fixed them though, along with tidying some more indentation.

​Build results

[Tom Prince]

This looks good, please merge.

[julian]

(In [38988]) Branching to remove-deprecated-test-methods-conch-6220-3.

[julian]

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