py3k: don't attempt to decode str objects

[nphilipp]

Python 3 str objects are Unicode already and don't need decoding.

#59

[codecov-io]

Current coverage is 85.17%

Merging #60 into develop will decrease coverage by -0.11% as of 5bcf81e

@@            develop     #60   diff @@
=======================================
  Files            10      10       
  Stmts           768     769     +1
  Branches          0       0       
  Methods           0       0       
=======================================
  Hit             655     655       
  Partial           0       0       
- Missed          113     114     +1

Review entire Coverage Diff as of 5bcf81e

Powered by Codecov. Updated on successful CI builds.

[jochumdev]

+1 This blocks the SMTP Transport on Py3k.

[nphilipp]

Unfortunately I don't have an idea how to get this new code "covered" under testing that deserves the name.

[nandoflorestan]

+1 for this pull request.

On Python 3.4, marrow.mailer 4.0.1 fails for me with this stacktrace:

reply: retcode (235); Msg: b'2.0.0 Authentication successful'
[2016-05-05 23:49:22,271: ERROR/Worker-2] <146250296044.2888.9856732595929587517@nando-Aspire-E5-571G> DEFERRED AttributeError
Traceback (most recent call last):
  File "/home/nando/.virtualenvs/fair/lib/python3.4/site-packages/marrow.mailer-4.0.1-py3.4.egg/marrow/mailer/transport/smtp.py", line 115, in send_with_smtp
    recipients = [addr.decode('utf-8') for addr in message.recipients.string_addresses]
  File "/home/nando/.virtualenvs/fair/lib/python3.4/site-packages/marrow.mailer-4.0.1-py3.4.egg/marrow/mailer/transport/smtp.py", line 115, in <listcomp>
    recipients = [addr.decode('utf-8') for addr in message.recipients.string_addresses]
AttributeError: 'str' object has no attribute 'decode'
send: 'quit\r\n'
reply: b'221 2.0.0 Bye\r\n'
reply: retcode (221); Msg: b'2.0.0 Bye'
[2016-05-05 23:49:22,418: INFO/MainProcess] Task divvymaster.tasks.mail.send_mail[4c355bd5-053c-4c06-ab38-ece1d90f3bce] succeeded in 1.9728077170000233s: None

The solution given in this pull request reads like it's good enough. And it's urgent, since marrow.mailer still cannot be used in Python 3 without it.

[nandoflorestan]

OK, only now do I realize that the "develop" branch already has simply removed the decode() call entirely:

59c6cd2

I will be using the "develop" branch for now, but I still think this bugfix, by itself, is enough cause for a new release.

[nphilipp]

@nandoflorestan, I'm not sure if skipping .decode() in Python 2 is the right thing to do -- in the following code, recipients will be a byte string then in Python 2 and a unicode string in Python 3 and that's almost begging for problems IMO ;).

[nandoflorestan]

@nphilipp you could be right (depends on what is going to be done with the string), but it worries me that you assert it WILL be a bytestring in Python 2. Actually it could also be a unicode string -- in fact, back when I used Python 2.6 and 2.7, I always ensured the strings in my system were all unicode strings, and this is a practice that I really recommend for those who still haven't upgraded.

That said, I agree that your patch is probably better for its stricter behaviour.

[nandoflorestan]

@amcgregor do you get a notification for each message, or only when someone @ calls you?

[amcgregor]

@nandoflorestan Looks like @mentions on this issue. Huh. Apparently I need to fix that, so thanks for the ping! As a quick note, could this be re-tested on current develop (not the PR)? I may have corrected the particular SMTP transport issue already.

[amcgregor]

After browsing the full discussion, I agree. My intention was to more "properly" correct this issue across Python 2 and Python 3 and rolling a release, but the test suite has suffered rather extreme bit rot. Two of the testing dependencies are simply not available on Python 3 (PyDNS + the mock SMTP server in use currently) and the suite needs to be rewritten to a) find replacements for these components, and b) update to the modern py.test + codecov practices from other Marrow packages. Not a small task.

And alas, even though it's a bug fix, I want a working test suite for any release.

[nphilipp]

@nandoflorestan:

Actually it could also be a unicode string...

You're quite right. I think it's better to put the check for the .decode() method into the loop and check per item rather than doing this assuming all items are of a certain type, do you agree?

@amcgregor:

even though it's a bug fix, I want a working test suite for any release

Agreed. That brings me back to my previous comment:

Unfortunately I don't have an idea how to get this new code "covered" under testing that deserves the name.

To expound on that, is there a test harness where we can test this with actually submitting mails through SMTP? That's what I think needs to be done to get this change covered. Am I right?

[amcgregor]

To expound on that, is there a test harness where we can test this with actually submitting mails through SMTP? That's what I think needs to be done to get this change covered. Am I right?

The current test suite used PyMTA to provide a mock SMTP server that the marrow.mailer transport would communicate with via local port, to perform full integration testing. Unfortunately, PyMTA is abandoned (last release in 2010) and hasn't been updated for Python 3. The reason the test suite is currently "passing" is that the entire SMTP test suite is currently disabled.

There are some alternatives, for example pytest-localserver. However, there's the peculiar requirement of Pypy 3, and thus Python 3.2, which a large number of these mocking packages will not support.

[nandoflorestan]

@nphilipp I agree. My library nine helps figuring these things out.

@amcgregor I do not think we need to do integration tests with a local SMTP server. After all, integrated tests are a scam. [Link redacted.]

[amcgregor]

@nandoflorestan Yeah, six and nine aren't touching marrow codebases, though nine is better than six in terms of dead/unused code (224 SLoC vs. 620), only about a dozen lines are actually necessary. (Ref the "compat" module in more recent Marrow projects.)

So… you aren't a fan of end-to-end verification of correct operation and graceful handling of error states? Unfortunate, but still, that's going to be present for any release given that the suite is already written, and certain conditions for the SMTP transport are most easily testable when communicating with a mock SMTP server.

[nandoflorestan]

@amcgregor sure, I see your points, no problem. But... did you watch the talk?

Or this one: [Link redacted.]

[amcgregor]

@nandoflorestan This particular ticket isn't the place for such discussion. Feel free to open a new ticket for the "you're doing testing wrong" aspect of things. Pull requests are preferred, however. I'll be closing this ticket, the minimal fix in develop is "sufficient patching for now".

[nandoflorestan]

Jesus, @amcgregor , I was just chatting with you, not trying to argue. I thought you would like to watch the talks, instead you seem to react as if I didn't respect your intelligence or something. I mean, if you ever offered me your favourite talks, I would feel very pleased, not insulted.

You are right that this isn't the right venue to chat, so OK, my mistake, pardon me. But did I really just cause you to lose your time?

@nphilipp I am sorry for inadvertently closing a pull request that wasn't even mine.

[amcgregor]

@nandoflorestan Your reaction would seem to indicate personal offence. None was intended. Oh well. This pull request was no longer needed, the effective change was already made, so it was closed. Had nothing to do with you.