-
Notifications
You must be signed in to change notification settings - Fork 768
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Asyncio RawSocket #679
Asyncio RawSocket #679
Conversation
peer = transport.get_extra_info('peername') | ||
self.peer = peer2str(peer) | ||
self.log.debug('RawSocker Asyncio: Connection made with peer {peer}'.format(peer=self.peer)) | ||
self._buffer = b'' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use structured logging, not string interpolation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean like this?
self.log.debug('RawSocker Asyncio: Connection made with peer {peer}', peer=self.peer)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, exactly
Thanks for working on this! As requirement would be Python 3 + 2 support plus all tests green. As you mentioned, you see some potential for avoiding code duplication .. this is highly welcome! Maybe you could break this change up in multiple smaller PRs? Easier/quicker to review .. |
OK - I'll look into clean CI tests firsts - fixed pep8 already will look into python2 |
I have interesting problem with unittests - in twisted tests I got this error:
It basically happens even before my test module is imported - parent modules Would removing of /autobahn/asyncio/websocket.py", line 32, in be safe? Because it seems as cause of problem. Or how to exclude searching for tests in |
@izderadicka regarding the testing, there's a Removing the |
Also, thanks for the PR! This is definitely a nice missing feature to have :) |
@meejah - I cannot solve problem with unittest just by using USE_TWISTED or USE_ASYNCIO - problem is that when running twisted tests ( for instance |
Python2 compatibility
Current coverage is 59.29%
@@ master #679 diff @@
==========================================
Files 63 66 +3
Lines 10278 10739 +461
Methods 0 0
Messages 0 0
Branches 1667 1643 -24
==========================================
+ Hits 5894 6367 +473
- Misses 3824 3860 +36
+ Partials 560 512 -48
|
Fixed all issues with clean build. Theoretically should be compatible with python2, but example is for python3 (yield from).
|
@izderadicka using the "whole file" Also, what about something like this: meejah@356bcec (Seems to work for me, but I'm not entirely sure why the pytest import doesn't fail in e.g. the py27 envs ...). The idea being: bail out early and avoid a "whole file" indent-level... |
@meejah And it works in py34-twtrunk or in other twisted test environments? It did not for me. The other tip looks fine - nice way how to avoid ident |
With changes as on the branch from #679 (comment) all the environments pass for me .. not entirely sure why? Hmm, maybe the "pytest-twisted" req for the Twisted environments ends up installing pytest in them, too... In any case, I think your first instinct to put these tests in IMO, the ApplicationRunner stuff is best dealt with in a follow-up PR; then at least we can get this code in and worry about the best syntax for ApplicationRunner separately. FWIW, your proposed URL syntax looks fine -- but there might be implications I'm not thinking of... |
p.s. just to clarify explicitly: IMO it's totally fine to have the asyncio example only work with python3 (i.e. "yield from" is fine for the examples). |
Returned test back to autobahn.asyncio.test (see what'll do)
I removed ApplicationRunnerRawSocket.
or this
Both fail for me because of imports in collection phase. Before it was not problem because there were no tests in It about organization of imports - question: are imports in @meejah - if you really can make test working in |
@izderadicka Ah, okay, I was missing a I'll see if I can make it work, but for now let's stick with the one which does work -- i.e. putting the tests in the "non ideal" place; we can always fix it later if there's a nicer way. Alternatively, if you'd prefer to keep them in the "ideal" place, you can keep it under the |
@meejah I see - that's cool trick - if test is not package, then it'll not import parent packages. I leave decision to you as maintainers - what is smaller evil - test in non-ideal place or in right place, but not as package ( all other test directories are packages - e.g. have |
I'm still not really sure what's going on, specifically why the Twisted tests work fine with a similar setup (including |
Ah, I think I know what's happening: the mere presence of a |
See izderadicka#1 |
https://github.com/meejah/AutobahnPython into meejah-issue679-asyncio-rawsocket-from-izderadicka Conflicts: autobahn/test/test_asyncio_rawsocket.py
Conflicts: tox.ini
Merged @meejah solution for unittest. Two more things were needed - adding Frankly spoken after playing a while with tests, maybe the simplest solution was not to try to put this test into |
Personally, I like separating tests from "real code". Some people prefer the opposite. But it might make the whole asyncio vs twisted vs generic test-organization a lot nicer to do something like:
I'm not sure if this would remove all the hack-ish things to get around test-collection etc. That is, trial would still have to be "tricked" into not importing There's also the "distribute the test-code with your module or not" question (e.g. that's one of the reasons people prefer tests "in" with the code, so they always come along). Since Autobahn has "small" systems as a target audience this might be a good reason to not include that code at all (i.e. you only get the Maybe for this PR @izderadicka's original solution is really the best? :/ ...and we could consider a wholesale move of the tests to their own tree as a separate ticket? |
@izderadicka cool! thanks a lot! |
Hi,
this is my first attempt for rawsocket transport for aio.
Few comments for current code:
utils
module ( which probably should be shared with remaining codebase) andWampRawSocketMixinGeneral
inrawsocket
module, which contains exactly same code as rawsocket transport for twisted.ApplicationRunnerRawSocket
- again made not to infer with existing code. Potentially it can be easily integrated into current runner - just use additional URI schemes tcp:// and unix:Let me know your thoughts - thanks
Ivan