Re-raising exception on deliver (fixes #49) #50
Conversation
In python3 it would be nice to chain it with smtplib.SMTPException.
|
Most intriguing. All these new errors on Travis come from this change? |
|
Yeah, those tests will need fixing. You'll also need to create a test for the new behaviour |
Current coverage is 93.58% (diff: 100%)@@ master #50 diff @@
==========================================
Files 15 15
Lines 1497 1497
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 1399 1401 +2
+ Misses 98 96 -2
Partials 0 0
|
|
OK, now it works and added an extra test case for the behavior (test_relay_asserts_no_relay). |
| @patch("salmon.server.smtplib.SMTP") | ||
| def test_relay_smtp(client_mock): | ||
| relay = server.Relay("localhost", port=8899) | ||
| relay = server.Relay("localhost", port=7899) |
There was a problem hiding this comment.
This is an unnecessary change: we mock out smtplib.SMTP already.
| @@ -16,7 +16,7 @@ | |||
| from .setup_env import setup_salmon_dirs, teardown_salmon_dirs | |||
|
|
|||
|
|
|||
| relay = relay(port=8899) | |||
| relay = relay(port=7899) | |||
There was a problem hiding this comment.
Why was the port number changed?
There was a problem hiding this comment.
Sorry, that port was in use in my machine, 8899 is not such an unusual port.
| @@ -164,7 +172,7 @@ def test_relay_smtp_ssl(client_mock): | |||
| def test_relay_deliver_mx_hosts(query): | |||
| query.return_value = [Mock()] | |||
| query.return_value[0].exchange = "localhost" | |||
| relay = server.Relay(None, port=8899) | |||
| relay = server.Relay(None, port=7899) | |||
There was a problem hiding this comment.
We should mock out smtplib here too
| @@ -191,7 +199,7 @@ def test_relay_resolve_relay_host(query): | |||
|
|
|||
|
|
|||
| def test_relay_reply(): | |||
| relay = server.Relay("localhost", port=8899) | |||
| relay = server.Relay("localhost", port=7899) | |||
|
|
||
|
|
||
| def setup_package(): | ||
| for path in dirs: | ||
| os.mkdir(path) | ||
| DUMMY_RELAY=subprocess.Popen("python -m smtpd -n -c DebuggingServer localhost:7899 > logs/smtpd.log", shell=True) | ||
| time.sleep(10) |
There was a problem hiding this comment.
I don't think we need this - see my comments on the relay tests
There was a problem hiding this comment.
It all boils down to this comment on the original code:
https://github.com/moggers87/salmon/blob/master/tests/salmon_tests/server_tests.py#L126
"this test actually delivers to a test server"
My understanding of the lamson testing code is that it assumes you're running a stmp at 8899. By virtue of silently failing if there is none, all these tests seem to be correct, when they were not.
The relay was not being tested. It was failing and continuing silently.
My initial fix was to set a "in_test" argument in Relay to mimic that behavior, but that didn't work with command_tests.
How do you want to reuse the smtplib mock?
There was a problem hiding this comment.
That's an old comment, it should be removed ☺
As far as I can see, all the tests that fail without a test server are all testing sending, not receiving.
You can use mock like tihs:
@patch("salmon.server.smtplib.SMTP")
def test_server_test(client_mock):
# test code
This is the documentation for the mock library we're using: http://www.voidspace.org.uk/python/mock/
|
OK, I think this could be it. Thanks for your help. If this is a go, let me know and I'll rebase it. |
|
Looks good to me, if you can undo your changes to the port numbers[1] used in tests and remove the unused imports in Sorry for the delay in reviewing your latest changes! [1]: I'll open a separate bug for dealing with port number clashes during testing. No matter what we choose, it will be in use on someone's system somewhere so let's do things properly :) |
|
Closing this PR as I've fixed this issue in master by not catching the exception at all. There's no real reason to catch an exception, log it, and then re-raise it. |
In python3 it would be nice to chain it with smtplib.SMTPException.