Opened 5 years ago

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

conch-deprecate.patch (25.6 KB) - added by Julian Berman 5 years ago.
conch-deprecate-v2.patch (35.9 KB) - added by Julian Berman 5 years ago.
conch-deprecate-v3.patch (36.4 KB) - added by Julian Berman 4 years ago.
conch-deprecate-v4.patch (1.1 KB) - added by Julian Berman 4 years ago.

Download all attachments as: .zip

Change History (38)

comment:1 Changed 5 years ago by DefaultCC Plugin

Cc: z3p added

Changed 5 years ago by Julian Berman

Attachment: conch-deprecate.patch added

comment:2 Changed 5 years ago by Tom Prince

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

comment:3 Changed 5 years ago by Tom Prince

Keywords: review removed

comment:4 Changed 5 years ago by Tom Prince

Owner: set to Julian Berman

comment:5 Changed 5 years ago by Julian Berman

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 5 years ago by Julian Berman

Attachment: conch-deprecate-v2.patch added

comment:6 Changed 5 years ago by Julian Berman

Owner: Julian Berman deleted

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

Owner: set to Jean-Paul Calderone
Status: newassigned

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

Keywords: review removed
Owner: changed from Jean-Paul Calderone to Julian Berman
Status: assignednew

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.

comment:9 Changed 5 years ago by 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?

comment:10 Changed 5 years ago by 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.

comment:11 Changed 4 years ago by Julian Berman

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

comment:12 Changed 4 years ago by Julian Berman

Sorry. #4991.

Changed 4 years ago by Julian Berman

Attachment: conch-deprecate-v3.patch added

comment:13 Changed 4 years ago by Tom Prince

Owner: set to Tom Prince

comment:14 Changed 4 years ago by Tom Prince

Author: tomprince
Branch: branches/remove-deprecated-test-methods-conch-6220

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

comment:15 Changed 4 years ago by Tom Prince

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

Refs #6220.

comment:16 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: changed from Tom Prince to Julian Berman

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.

comment:17 Changed 4 years ago by 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.

comment:18 Changed 4 years ago by 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.

comment:19 Changed 4 years ago by Tom Prince

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

comment:20 Changed 4 years ago by 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.

comment:21 Changed 4 years ago by Tom Prince

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

comment:22 Changed 4 years ago by 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.

comment:23 Changed 4 years ago by 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...

comment:24 Changed 4 years ago by 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.

comment:25 Changed 4 years ago by 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.

Changed 4 years ago by Julian Berman

Attachment: conch-deprecate-v4.patch added

comment:26 Changed 4 years ago by 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.

comment:27 Changed 4 years ago by Julian Berman

Keywords: review added
Owner: Julian Berman deleted

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

comment:28 Changed 4 years ago by Richard Wall

Owner: set to Richard Wall
Status: newassigned

comment:29 Changed 4 years ago by Richard Wall

Author: tomprincetomprince, rwall
Branch: branches/remove-deprecated-test-methods-conch-6220branches/remove-deprecated-test-methods-conch-6220-2

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

comment:30 Changed 4 years ago by Richard Wall

Keywords: review removed
Owner: changed from Richard Wall to Julian Berman
Status: assignednew

Code Review

Thanks Julian.

Notes:

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.

comment:31 Changed 4 years ago by Julian Berman

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.

Build results

comment:32 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Julian Berman

This looks good, please merge.

comment:33 Changed 4 years ago by julian

Author: tomprince, rwalljulian, tomprince, rwall
Branch: branches/remove-deprecated-test-methods-conch-6220-2branches/remove-deprecated-test-methods-conch-6220-3

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

comment:34 Changed 4 years ago by julian

Resolution: fixed
Status: newclosed

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

Note: See TracTickets for help on using tickets.