Add uwsgi_plugin clog message header #41
Conversation
It turns out attempting to marshal.loads data which isn't marshal serialized can hang indefinitely. Let's not do that.
testing/sandbox.py
Outdated
| pass | ||
|
|
||
| sys.modules['uwsgi'] = uwsgi | ||
| sys.modules['uwsgidecorators'] = uwsgidecorators |
There was a problem hiding this comment.
this should at the very least be a context manager which undoes this when exiting. It should also probably use mock.patch.dict(...)
(otherwise this is a good source of test pollution)
tests/test_uwsgi_plugin.py
Outdated
| @@ -0,0 +1,62 @@ | |||
| from testing.sandbox import install_fake_uwsgi | |||
| install_fake_uwsgi() | |||
There was a problem hiding this comment.
based on your test ordering, this'll leak into the other tests
I'd suggest instead doing these tests in a subprocess (similar to how the clog config tests work)
There was a problem hiding this comment.
And actually, maybe you should just use uwsgi to demonstrate this?
There was a problem hiding this comment.
@asottile I'd really like to simply use uwsgi, in fact we don't even need the module, we mock out everything since we're just testing logic specific to the plugin functions. However we need to be able to import uwsgi, but that would require running the tests themselves inside a uwsgi application server since the uwsgi python plugin injects the module into the environment at run time after initializing the interpreter. I'll see about wrapping the import inside a pytest startup/teardown.
There was a problem hiding this comment.
It's actually really easy to spin up uwsgi. (Internal) if you look at yelp-wsgi-encoding's tests they spin up and down uwsgi to test their behaviour.
There was a problem hiding this comment.
Oh yea? Do you think that would be simpler than patching sys.modules when all we want to accomplish importing something since we mock out the actual uwsgi behavior? I'm open to either approach.
There was a problem hiding this comment.
shrugs, maybe not? But it would get you closer to an integration test.
There was a problem hiding this comment.
Very true. Hmm... taking a look at the yelp-wsgi-encoding I don't see any invocation (or reference) to uwsgi. It looks like they're building a wsgi app, but not actually running uwsgi itself right? We would actually have to run the tests from inside the uwsgi application to get the uwsgi_pymodule available I think :/
There was a problem hiding this comment.
damn, maybe that was an old version -- I swore I used uwsgi at some point, maybe I moved it to raw sockets
There was a problem hiding this comment.
Wups, nvm I'm just blind.
There was a problem hiding this comment.
@asottile no you're definitely right, but they spin it up and query against the provided socket, but they don't actually run the test inside the server, so I don't think they'd have access to the module. I'll poke around with it though, because it really would be great to have a solid integration test for this.
tests/test_uwsgi_plugin.py
Outdated
|
|
||
| def test_uwsgi_mule_msg_header_apply(): | ||
| # for some reason python3 marshal serializes arguments | ||
| # differently when their handled via expansion/compaction |
|
Updated tests should isolate the |
tests/test_uwsgi_plugin.py
Outdated
| @pytest.mark.usefixtures('uwsgi_plugin') | ||
| class TestUwsgiPlugin(object): | ||
|
|
||
| def test_uwsgi_handle_msg_header(self, uwsgi_plugin): |
There was a problem hiding this comment.
Can we split this in two tests?
tests/test_uwsgi_plugin.py
Outdated
| message = ('this is a fake stream', 'this is a fake line!') | ||
| pickle_data = pickle.dumps(message) | ||
| mule_msg_data = uwsgi_plugin._encode_mule_msg(*message) | ||
| with mock.patch.object(uwsgi_plugin, '_orig_log_line') as orig_line: |
tests/test_uwsgi_plugin.py
Outdated
| uwsgi_plugin._plugin_mule_msg_shim(mule_msg_data) | ||
| orig_line.assert_called_with(*map(six.b, message)) | ||
|
|
||
|
|
There was a problem hiding this comment.
Only one blank line. Apply also below.
tests/test_uwsgi_plugin.py
Outdated
| uwsgi_plugin._mule_msg(*args, **kwargs) | ||
| mm.assert_called_with(expected_serialized, 1) | ||
|
|
||
| def test_decode_mule_msg_exc_tag(self, uwsgi_plugin): |
There was a problem hiding this comment.
rename test_decode_mule_msg_exc_tag to test_decode_mule_msg_tag_error ?
tests/test_uwsgi_plugin.py
Outdated
| with pytest.raises(ValueError): | ||
| uwsgi_plugin._decode_mule_msg(msg) | ||
|
|
||
| def test_decode_mule_msg_exc_len(self, uwsgi_plugin): |
In the event that a uwsgi_plugin test fails, the user may re-run with POLLUTE=True set to run the tests in the primary process and see the exceptions/tracebacks. Simply: > POLLUTE=True tox Will do the trick
|
After trying to figure out a clever way to pass exceptions from child processes to the parent to properly raise to pytest, I've decided it's probably easier to simply allow the user to rerun the tests without the process offloading (i.e. allowing pollution). If a uwsgi_plugin test fails and the failure isn't obvious, the user can simply: |
It turns out attempting to marshal.loads data which isn't marshal
serialized can hang indefinitely. Let's not do that.