Opened 4 years ago

Last modified 3 years ago

#6545 task assigned

Replace usage of twisted.python.text in twisted.mail

Reported by: Thijs Triemstra Owned by: Thijs Triemstra
Priority: low Milestone:
Component: mail Keywords:
Cc: Thijs Triemstra Branch: branches/replace-text-mail-6545
branch-diff, diff-cov, branch-cov, buildbot
Author: thijs

Description

twisted.python.text is being deprecated (#6341) and its usage should be replaced in twisted.mail.

Change History (12)

comment:1 Changed 4 years ago by Thijs Triemstra

Author: thijs
Branch: branches/replace-text-mail-6545

(In [38636]) Branching to 'replace-text-mail-6545'

comment:2 Changed 4 years ago by Thijs Triemstra

(In [38637]) replace usage of t.python.text, refs #6545

comment:3 Changed 4 years ago by Thijs Triemstra

Keywords: review added
Owner: Thijs Triemstra deleted

Replaced the usage in twisted.mail. Added a copy of strFile (and tests, now with documentation) from twisted.python.text to twisted.mail.imap4 because this was the only user of it.

Forced a build.

comment:4 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Thijs Triemstra

Thanks for working on this. A couple of things:

  1. There appears to be an uncovered line in _strFile.
    • It also looks like there is a bug if len(searchString) > 2**2**2**2**2, since it looks like it will try to read 0 bytes.
    • In general, the tests don't seem designed to cover the corner cases in this function.
  2. The docstrinsg for test_searchBody should mention case-insensitivity.
  3. _strFile could perhaps have a more descriptive name.

Please resubmit for review after addressing the above points.

comment:5 Changed 4 years ago by Thijs Triemstra

(In [38980]) address review comments, refs #6545

comment:6 in reply to:  4 Changed 4 years ago by Thijs Triemstra

Keywords: review added
Owner: Thijs Triemstra deleted

Replying to tom.prince:

Thanks for working on this. A couple of things:

  1. There appears to be an uncovered line in _strFile.
    • It also looks like there is a bug if len(searchString) > 2**2**2**2**2, since it looks like it will try to read 0 bytes.

I'm not sure how to cover that line or trigger the bug you describe. could you create/describe a test case or open a new ticket?

  • In general, the tests don't seem designed to cover the corner cases in this function.

They're copied from the existing tests.

  1. The docstrinsg for test_searchBody should mention case-insensitivity.

Fixed.

  1. _strFile could perhaps have a more descriptive name.

Called it _searchFile...

comment:7 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Thijs Triemstra
  1. I've added a couple of test cases, exercising the behavior with longs strings.
    • Although, it might be easier to test if the buffer size was a (private) symbolic constant, that could be patched.
  • In general, the tests don't seem designed to cover the corner cases in this function.

They're copied from the existing tests.

That doesn't mean they are good tests (as the above bug demonstrates). _searchFile seems to be an essential part of the implementation of search for imap, and so should be well tested.

Thanks for working on this. Please resubmit for review after addressing 1-3.

comment:8 in reply to:  7 Changed 4 years ago by Thijs Triemstra

Keywords: review added
Owner: Thijs Triemstra deleted

Replying to tom.prince:

  • In general, the tests don't seem designed to cover the corner cases in this function.

They're copied from the existing tests.

That doesn't mean they are good tests (as the above bug demonstrates). _searchFile seems to be an essential part of the implementation of search for imap, and so should be well tested.

While I agree with that, I am not trying to fix a bug that already existed in t.p.text as well, I'm trying to get rid of the dependency in twisted.mail. So anything that is broken can be handled in a separate ticket?

comment:9 Changed 4 years ago by Jean-Paul Calderone

Generally we should allow code to be moved, unmodified, even if it does not have complete test coverage. We need to be careful to ensure that it is happy in its new home (necessary imports exist - and are of the correct module, not just a module with the same name!). This is necessary to facilitate proper maintenance of the 'structure' of the project even where the 'content' of the project has incomplete test coverage.

comment:10 Changed 4 years ago by Richard Wall

Owner: set to Richard Wall
Status: newassigned

Reviewing...

comment:11 Changed 4 years ago by Richard Wall

Keywords: review removed
Owner: changed from Richard Wall to Thijs Triemstra
Status: assignednew

Thanks Thijs,

Notes:

Points:

  1. test/test_imap.py
    1. Typos "bondary" "paased"
    2. It's not clear to me what the significance is of test_singleCharInFileThreeTimes and test_fourCharsInFile. Seems abitrary.
    3. Also the "huge string" in test_overLargeStringNotInFile doesn't seem huge at all. These tests need more explanation.
    4. Various C{False} which should be L{False}...C{str} should be L{str}....although it probably doesn't matter since these are test methods.
    5. I find the reference to self.io makes the tests difficult to read. You may as well just define the full test string and the search string in each test.
    6. Test failure -- I guess this is demonstrating the bug that Tom mentioned in comment:4
      [FAIL]
      Traceback (most recent call last):
        File "/home/buildslave/slaves/twisted/bot-idnar-debian64/easy-no-zope-2.6debian/venv/lib/python2.6/site-packages/Twisted-13.2.0-py2.6-linux-x86_64.egg/twisted/mail/test/test_imap.py", line 4298, in test_longString
          self.assertInFile(string)
        File "/home/buildslave/slaves/twisted/bot-idnar-debian64/easy-no-zope-2.6debian/venv/lib/python2.6/site-packages/Twisted-13.2.0-py2.6-linux-x86_64.egg/twisted/mail/test/test_imap.py", line 4277, in assertInFile
          "%.20r in file %r" % (inputString, self.io))
        File "/home/buildslave/slaves/twisted/bot-idnar-debian64/easy-no-zope-2.6debian/venv/lib/python2.6/site-packages/Twisted-13.2.0-py2.6-linux-x86_64.egg/twisted/trial/_synctest.py", line 308, in assertTrue
          raise self.failureException(msg)
      twisted.trial.unittest.FailTest: 'xxxxxxxxxxxxxxxxxxx in file <cStringIO.StringI object at 0x6dac030>
      
      twisted.mail.test.test_imap.SearchFileTests.test_longString
      
    7. Given exarkun's comment:9, maybe you should just open a new ticket for fixing bugs in _searchFile and move the new SearchFileTests to a different branch.
    8. It also occurs to me that _searchFile is actually a very useful thing (although maybe not implemented particularly well). I know exarkun suggested making it private (ticket:6341#comment:7), but I'd be inclined to leave it where it is and improve it. That's just my opinion, I'm not saying abandon this branch. Maybe it can be improved and made public again in a more suitable module in the future.

So....I say:

  1. Open tickets about the _searchFile bugs
  2. Attach the new tests as a patch
  3. Merge this branch forward
  4. Check for a clean build
  5. Then merge.

-RichardW.

comment:12 Changed 3 years ago by Thijs Triemstra

Status: newassigned
Note: See TracTickets for help on using tickets.