Support SOCKS over Unix domain sockets #40
Conversation
|
The Coveralls fails on Azure appear to not be directly related to this PR. |
aiorpcx/util.py
Outdated
|
|
||
| def __eq__(self, other): | ||
| # pylint: disable=protected-access | ||
| return self._path == other._path |
There was a problem hiding this comment.
This needs to test isinstance before testing the member
aiorpcx/socks.py
Outdated
|
|
||
| auth is an authentication method to use when connecting, or None. | ||
| ''' | ||
| if not isinstance(address, NetAddress): | ||
| if not isinstance(address, NetAddress) and not isinstance(address, UnixAddress): |
There was a problem hiding this comment.
Please combine these. isinstance takes a tuple to test multiple cases.
|
Addressed feedback, thanks for catching those. Coveralls still unhappy as before due to a missing token. I'm pretty sure the Py38_Win pytest failure is unrelated to this PR as well (it seems to be in a completely unrelated area of the code). |
|
Hi Jeremy, the issue with the missing var seems to be fixed (I renamed it; unclear what the problem was as it used to work and I didn't change anything). So coveralls now runs. Can you please provide 100% test coverage of the new lines of code. |
|
@kyuupichan What's the preferred way to test the Unix socket code? Tweak |
|
Whatever is simplest for you. I don't think you need all the tests that already exist as the socks code is network-independent. I'm mainly interested in code coverage. So a single test that tests a successful connection is made would suffice - the assumption being if it works for a successful connection then all the other cases have no reason not work too. You might need to add a dummy test for one or two of the methods of the class you add, or they might all get used anyway; I'm not sure. |
|
Coveralls on Azure still seems unhappy about a missing token. :/ |
|
git HEAD is not - as you can see from recent changes I've made - are you building on HEAD? |
I think so? This PR's parent commit is d3f5ac5. That should have the fix, right? |
|
It makes no sense to me - when you run it, azure pipelines doesn't replace $(CRT) with the variable's value; when I run it, it does. I suggest you just add the tests that pass for you and that you think give coverage, and I just merge it. |
Fixes #39 ; tested with a lightly patched spesmilo/electrumx on Whonix 15 and seems to work fine with the SOCKS Unix socket that Whonix provides.
Let me know if anything needs improvement, or if you need tests added.