9100-evilham-pop3-spaces-in-pass #769

evilham · 2017-04-20T00:42:53Z

PR Fixing 9100: http://twistedmatrix.com/trac/ticket/9100

…aceInPASS.

codecov · 2017-04-20T01:06:50Z

Codecov Report

Merging #769 into trunk will decrease coverage by 0.57%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##            trunk     #769      +/-   ##
==========================================
- Coverage   91.82%   91.25%   -0.58%     
==========================================
  Files         840      840              
  Lines      148582   147378    -1204     
  Branches    12978    12892      -86     
==========================================
- Hits       136436   134485    -1951     
- Misses      10031    10672     +641     
- Partials     2115     2221     +106

exarkun

Thanks! This is quite a good contribution, particularly for your first. I've left a few comments inline. Sorry they're not all for things you could have discovered on your own by looking at developer docs. I hope they're all clear and reasonable. Please let me know if the case appears to be otherwise.

Apart from the inline comments, I'd also like to suggest that formatting changes (like inserting blank lines and re-wrapping lines to <80 columns) are better done as a separate patch - either before or after the more interesting change you want to submit. It makes the review of both pieces easier.

Thanks again.

exarkun · 2017-04-20T01:30:42Z

src/twisted/mail/pop3.py

@@ -36,7 +36,7 @@
 from twisted import cred

 ##
-## Authentication
+#  Authentication


Probably should leave the comment style alone unless you want to make a case for changing it throughout the module.

exarkun · 2017-04-20T01:33:01Z

src/twisted/mail/pop3.py

@@ -855,7 +858,13 @@ def do_PASS(self, password):

        @type password: L{bytes}
        @param password: A password.
+
+        @type args: L{tuple} or L{None}
+        @param args: Other words composing the password.


It can't actually be None because it's *args. It can be any tuple, including an empty tuple.

My bad, I copied the description from do_AUTH where that of processCommand would have been much better. Will change to:

@type args: L{tuple} of L{bytes} @param args: Other words composing the password to be joined with a space.

exarkun · 2017-04-20T01:33:38Z

src/twisted/mail/pop3.py

@@ -565,7 +565,8 @@ def state_COMMAND(self, line):
            return self.processCommand(*line.split(' '))
        except (ValueError, AttributeError, POP3Error, TypeError) as e:
            log.err()
-            self.failResponse('bad protocol or server: %s: %s' % (e.__class__.__name__, e))
+            self.failResponse(
+                'bad protocol or server: %s: %s' % (e.__class__.__name__, e))


If you want to change these lines, I'd suggest this style:

self.failResponse( 'bad protocol or server: %s: %s' % (e.__class__.__name__, e) )

exarkun · 2017-04-20T01:34:44Z

src/twisted/mail/pop3.py

        """
+        if args:
+            password += ' ' + ' '.join(args)


Or password = " ".join((password,) + args).

Regardless of that, b" " - not ' '. Byte strings need to be explicit now.

This brings me down a rabbit hole: doesn't that mean stuff like return self.processCommand(*line.split(' ')) in line 565 should use b' ' as well? On a quick check, I see this overall in pop3.py.
If that's the case, am I correct to assume we should open a new ticket and fix it there?

Those are also good changes to make. And filing a new ticket to handle those changes is the right idea, yea. These particular strings should probably still change since they're new in this branch (but you could also leave them as long as you promise to fix the whole module after this branch is merged 😄 ).

exarkun · 2017-04-20T01:35:25Z

src/twisted/mail/test/test_pop3.py

@@ -1,3 +1,4 @@
+# -*- test-case-name: twisted.mail.pop3 -*-


This is backwards. twisted.mail.test.test_pop3 is the test-case-name for twisted/mail/pop3.py. Declaring this variable with this value here won't do anything.

:-D I wasn't sure at all with this one, this needs to be better in the Wiki! What does it do when properly set up anyway?

It does a few things:

It defines a buffer-local variable when the file is opened in emacs. The variable name is test-case-name and the value is twisted.mail.pop3.

This, in turn, advises emacs' twisted-dev-mode that when you hit F9 to run the test suite for this file it should pass twisted.mail.pop3 to trial (this is why it doesn't make sense - trial twisted.mail.pop3 won't run any tests).

It also advises the trial itself what tests to run (see the --testmodule option).

And this is also used by the BuildBot "quick" builder to run a subset of the test suite related to files being changed to try to get some results back more quickly than if you have to run the whole test suite. However, I think the "quick" builder was removed a long time ago.

exarkun · 2017-04-20T01:37:00Z

src/twisted/mail/test/test_pop3.py

    def _cbTestAuthListing(self, ignored, client):
        self.assertTrue(client.response[1].startswith('+OK'))
        self.assertEqual(sorted(client.response[2:5]),
                         ["AUTH1", "AUTHLAST", "SECONDAUTH"])
        self.assertEqual(client.response[5], ".")

+
    def testIllegalPASS(self):


Please add a docstring for this modified test and rename it to follow the naming convention - test_illegalPASS.

exarkun · 2017-04-20T01:38:07Z

src/twisted/mail/test/test_pop3.py

+             '+OK '])
+
+
+    def testWhiteSpaceInPASS(self):


Please add a docstring for this new test method and rename it to follow the naming convention. Also, please add more test coverage for the new functionality. For example, multiple words separated by a space and multiple consecutive spaces. Other kinds of whitespace would also be an interesting thing to test (I don't think that's broken but having the test coverage would be beneficial).

Also, a positive-path test where passwords with spaces in them lead to an authentication success would be good. Otherwise the test suite just shows it isn't a parse error - it doesn't show it actually works.

exarkun · 2017-04-20T01:39:49Z

src/twisted/mail/test/test_pop3.py

 class POP3MiscTests(unittest.TestCase):
    """
    Miscellaneous tests more to do with module/package structure than
    anything to do with the Post Office Protocol.
    """
-    def test_all(self):
+    def testAll(self):


This is a transformation in the wrong direction. 😄 Please revert.

exarkun · 2017-04-20T01:40:33Z

src/twisted/topfiles/9100.bugfix

@@ -0,0 +1 @@
+twisted.mail.pop3 now accepts passwords that contain spaces.


Very nice. I'd say twisted.mail.pop3.POP3 ... or The POP3 server implemented by twisted.mail.pop3 ... though to avoid client/server confusion

evilham · 2017-04-20T08:51:02Z

Great! Thanks for the comments, will fix 'em later.
However:
some of the changes I had done, were a direct consequence of running twistedchecker and trying to kill some Warnings / Errors it was giving. Does that mean, we should file a bug (or maybe even a couple) against twistedchecker?
I mean it particularly because of the POP3MiscTests.testAll / test_all naming thingy :).

I clarify, I picked this bug as an easy one to get to know Twisted's ways of contributing, and maybe even improving the documentation for that ;).

exarkun · 2017-04-20T12:11:38Z

Does that mean, we should file a bug (or maybe even a couple) against twistedchecker?
I mean it particularly because of the POP3MiscTests.testAll / test_all naming thingy :).

Hmmm yes, sounds like it. twistedchecker should definitely not be telling people to rename things like test_all to things like testAll.

evilham · 2017-04-25T13:07:45Z

@exarkun: can we merge this branch and close this ticket?

I feel like I'm fixing other stuff due to the rookie mistake of messing up with coding standards on the same PR :).

Also, I think the pop3 module is simple enough for me to haunt some of its bugs listed here.

Incidentally, I just opened Trac Issue 9120 referring to this module's Python3 support and coding standards.

I guess it's a good idea to bring up some of the issues we detected in the mailing list?

glyph · 2017-06-07T04:21:16Z

The one remaining problem here is the minor gap in diff coverage; it looks like tried_user is always passed in https://codecov.io/gh/twisted/twisted/compare/d3a7f6a5f8f673e52398f630566a303edf6e82d3...05f23e3c0ff5cd0ef5ff84c404c1dd6b88603f5c/diff#D1-521 and this log.err call is never invoked: https://codecov.io/gh/twisted/twisted/compare/d3a7f6a5f8f673e52398f630566a303edf6e82d3...05f23e3c0ff5cd0ef5ff84c404c1dd6b88603f5c/diff#D2-792

Can you address these two minor issues, and merge trunk, so we can land? Thanks!

evilham · 2017-06-07T16:24:35Z

Hi @glyph,

As for the tried_user argument:
that should not be the case, on line pop3.py:521 there is a test calling self.run_PASS(b'testuser', b'fooz barz'); which means it should branch into that line.
My local run of trial --coverage does not show an issue here.

I reverted the indentation change that was causing the coverage gap. As mentioned before, that was being caused by the rookie mistake of mixing coding standards fix and bug fix on the same PR.

I'd think this is acceptable, as the thing keeping me from working on Trac Issue 9120 is precisely that this hasn't landed :-). 9120 should result in Python3 support in t.m.pop3 and better tests and coverage for the module.

PS: Travis says 4190.6 fails because of topfile and there is a topfile, am I doing this wrong?

evilham · 2017-06-07T16:51:20Z

My bad, I was also interpreting it as tried_user never being passed, turns out, the case that was not covered was it being passed :).

Just added a test to account for a wrong user being passed and that should solve the partial hit.

glyph · 2017-06-08T06:00:25Z

PS: Travis says 4190.6 fails because of topfile and there is a topfile, am I doing this wrong?

The topfile has changed recently with our recent conversion from our newsfiles stuff to towncrier. You just need to put it in a different directory now.

https://twistedmatrix.com/trac/ticket/8133

…/evilham/twisted into 9100-evilham-pop3-spaces-in-pass

evilham · 2017-06-08T11:02:26Z

Thank you, I had missed that and didn't think to re-check the docs.
Build is fine now in addition to yesterday's coverage-related changes.

adiroiban · 2017-09-30T22:45:55Z

src/twisted/mail/pop3.py

@@ -565,7 +565,9 @@ def state_COMMAND(self, line):
            return self.processCommand(*line.split(' '))
        except (ValueError, AttributeError, POP3Error, TypeError) as e:
            log.err()
-            self.failResponse('bad protocol or server: %s: %s' % (e.__class__.__name__, e))
+            self.failResponse(
+                        'bad protocol or server: %s: %s' % (e.__class__.__name__, e)


I am trying to understand what are the rules for this indentation... I see this double indentation in other parts of this PR.

adiroiban · 2017-09-30T22:50:47Z

wow... GitHub with "Update this branch" allows me to trigger a merge in others repos.

I hope that with the updated twistedchecker, this PR will get all the errors related to the coding convention

rodrigc

@evilham Can you close this PR, and submit a new one based on latest trunk?

The code changed a lot in trunk after #878

adiroiban · 2017-10-11T17:05:39Z

I assume that you can just update with trunk and fix the conflicts and push a new change to this same PR. no need to close this one and open a new one

rodrigc · 2017-10-11T17:08:51Z

@adiroiban No, actually in this case I prefer a new PR, thanks.

exarkun · 2017-10-17T15:19:02Z

@adiroiban No, actually in this case I prefer a new PR, thanks.

Why? Updating the branch behind a PR is standard practice.

rodrigc · 2017-10-17T15:47:33Z

@exarkun Normally I agree but in this case, @evilham made a lot of style changes which overlap with other changes made elsewhere, so to make a clean start, I'm requesting a new patch.

While it may be a "standard practice", I don't believe in one size fits all practices, and it isn't too hard to submit a new PR, which is why I asked.

evilham · 2017-10-17T16:27:48Z

Great, will close now and open a new one with only the code changes.
Note to self: update trac.

evilham added 5 commits April 20, 2017 00:03

Applied coding standards to test_pop3.

48518e2

Refactored testIllegalPASS and testEmptyPASS; implemented testWhiteSp…

fd18d7a

…aceInPASS.

Fixed twisted#9100, handle passwords with spaces in pop3.

89a741c

Improved code style compliance.

7ee557d

Added topfile for 9100.

7acf6bf

exarkun requested changes Apr 20, 2017

View reviewed changes

evilham added 3 commits April 20, 2017 16:49

Added more tests and corrected comments by @exarkun.

98b7437

Added test for unknown commands in t.m.pop3.POP3 implementation

e5a2cfc

Merge branch 'trunk' into 9100-evilham-pop3-spaces-in-pass

0f28499

evilham closed this Apr 25, 2017

evilham reopened this Apr 25, 2017

Merge branch 'trunk' into 9100-evilham-pop3-spaces-in-pass

05f23e3

evilham added 2 commits June 7, 2017 16:31

Merge branch 'trunk' into 9100-evilham-pop3-spaces-in-pass

bc52986

Revert indenting for coverage.

4c50082

Added wrong user test.

65d1ba5

Merge branch 'trunk' into 9100-evilham-pop3-spaces-in-pass

429f413

evilham added 2 commits June 8, 2017 10:27

Fixed topfile with admin/fix-for-towncrier.py.

d6bba1a

Merge branch '9100-evilham-pop3-spaces-in-pass' of https://github.com…

df83d9f

…/evilham/twisted into 9100-evilham-pop3-spaces-in-pass

evilham added 3 commits June 12, 2017 17:00

Merge branch 'trunk' into 9100-evilham-pop3-spaces-in-pass

9bad651

Merge branch 'trunk' into 9100-evilham-pop3-spaces-in-pass

bfb0657

Merge branch 'trunk' into 9100-evilham-pop3-spaces-in-pass

3aa4494

evilham and others added 14 commits June 25, 2017 11:45

Merge branch 'trunk' into 9100-evilham-pop3-spaces-in-pass

47e7867

Merge branch 'trunk' into 9100-evilham-pop3-spaces-in-pass

964d52c

Merge branch 'trunk' into 9100-evilham-pop3-spaces-in-pass

ddc361d

Merge branch 'trunk' into 9100-evilham-pop3-spaces-in-pass

ae1a082

Merge branch 'trunk' into 9100-evilham-pop3-spaces-in-pass

6e5a07e

Merge branch 'trunk' into 9100-evilham-pop3-spaces-in-pass

0cdadcf

Merge branch 'trunk' into 9100-evilham-pop3-spaces-in-pass

fe8e11b

Merge branch 'trunk' into 9100-evilham-pop3-spaces-in-pass

9faef0b

Merge branch 'trunk' into 9100-evilham-pop3-spaces-in-pass

cd5cbad

Merge branch 'trunk' into 9100-evilham-pop3-spaces-in-pass

35c3fee

Merge branch 'trunk' into 9100-evilham-pop3-spaces-in-pass

94efedf

Merge branch 'trunk' into 9100-evilham-pop3-spaces-in-pass

83dcfa4

Merge branch 'trunk' into 9100-evilham-pop3-spaces-in-pass

c91c8d6

Merge branch 'trunk' into 9100-evilham-pop3-spaces-in-pass

24b25f5

adiroiban reviewed Sep 30, 2017

View reviewed changes

rodrigc suggested changes Oct 11, 2017

View reviewed changes

evilham closed this Oct 17, 2017

evilham deleted the 9100-evilham-pop3-spaces-in-pass branch July 23, 2019 12:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

9100-evilham-pop3-spaces-in-pass #769

9100-evilham-pop3-spaces-in-pass #769

evilham commented Apr 20, 2017

codecov bot commented Apr 20, 2017 •

edited

Loading

exarkun left a comment

exarkun Apr 20, 2017

exarkun Apr 20, 2017

evilham Apr 20, 2017

exarkun Apr 20, 2017

exarkun Apr 20, 2017

evilham Apr 20, 2017

exarkun Apr 20, 2017

exarkun Apr 20, 2017

evilham Apr 20, 2017

exarkun Apr 20, 2017

exarkun Apr 20, 2017

exarkun Apr 20, 2017

exarkun Apr 20, 2017

exarkun Apr 20, 2017

evilham commented Apr 20, 2017

exarkun commented Apr 20, 2017

evilham commented Apr 25, 2017

glyph commented Jun 7, 2017

evilham commented Jun 7, 2017

evilham commented Jun 7, 2017

glyph commented Jun 8, 2017

evilham commented Jun 8, 2017

adiroiban Sep 30, 2017

adiroiban commented Sep 30, 2017

rodrigc left a comment

adiroiban commented Oct 11, 2017

rodrigc commented Oct 11, 2017

exarkun commented Oct 17, 2017

rodrigc commented Oct 17, 2017

evilham commented Oct 17, 2017

		@@ -0,0 +1 @@
		twisted.mail.pop3 now accepts passwords that contain spaces.

9100-evilham-pop3-spaces-in-pass #769

9100-evilham-pop3-spaces-in-pass #769

Conversation

evilham commented Apr 20, 2017

codecov bot commented Apr 20, 2017 • edited Loading

Codecov Report

exarkun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evilham commented Apr 20, 2017

exarkun commented Apr 20, 2017

evilham commented Apr 25, 2017

glyph commented Jun 7, 2017

evilham commented Jun 7, 2017

evilham commented Jun 7, 2017

glyph commented Jun 8, 2017

evilham commented Jun 8, 2017

Choose a reason for hiding this comment

adiroiban commented Sep 30, 2017

rodrigc left a comment

Choose a reason for hiding this comment

adiroiban commented Oct 11, 2017

rodrigc commented Oct 11, 2017

exarkun commented Oct 17, 2017

rodrigc commented Oct 17, 2017

evilham commented Oct 17, 2017

codecov bot commented Apr 20, 2017 •

edited

Loading