-
Notifications
You must be signed in to change notification settings - Fork 49
interop_test_runner: use Python 3 #170
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
Conversation
|
|
||
| test-interop: | ||
| $(DEV_DIR)/interop_test_runner -v | ||
| $(DEV_DIR)/interop_test_runner.py -v |
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.
what for?
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.
Otherwise pytest won't be able to load the script (see initial description for why pytest helps during development).
_dev/interop_test_runner.py
Outdated
| # 'ss -Htln' is the modern form, but boringssl image only includes | ||
| # netstat (via busybox). | ||
| info = c.exec_run('sh -c "ss -Htln 2>/dev/null || netstat -tln"') | ||
| if info.exit_code != 0: |
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.
remove != 0
_dev/interop_test_runner.py
Outdated
| self.wait_for_ports(c) | ||
| return c | ||
|
|
||
| def wait_for_ports(self, c): |
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.
can we do it better without calling shell at all?
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.
I couldn't really find a good alternative. I tried to retrieve container IP and try to port scan it from Python. Does not work on Docker for Mac, details.
An alternative is to start a container for the sole purpose of port-scanning a host (it could consist of a single Go binary), but the size of that support code will be slightly larger than the current approach. If you think that this would be better, I can change it though?
Another possibility is to add a HEALTCHECK command, but that would roughly have the same effect as checking ss, except that the logic is moved into the Docker image.
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.
I would double check docker-py. I wonder if it's not done by this wrapper itself.
If not can't we just "import socket" and "socket.connect"? It's hard for me to believe python doesn't have something equivalent to ss
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.
I have tried the socket approach before, but it won't work:
Does not work on Docker for Mac, details.
Checking for reachable ports is not something that can be done with the high-level docker-py API (docker/docker-py#2229). Usually healthchecks are used to test whether a service is ready (e.g. aside from checking that a TCP socket can be opened, verify that the HTTP service returns a proper response). In our case, it is sufficient if a TCP socket can be opened.
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.
ok, keep digging.
|
This change is a root cause of python issuing such warn's I'm actually getting a lot of those locally. It will need to be fixed before merging |
'python2' does not exist on macOS, it is called 'python2.7'. Porting to Python 3 is however more future-proof so do that instead. Rename the file such that it can be run with pytest. Do not listen on a fixed host port and use a random port instead. Otherwise tests could fail if something is listening on those ports.
5497468 to
a539641
Compare
|
I've dropped the whole server start optimization (and kept two versions at https://github.com/cloudflare/tls-tris/commits/pwu/tests-speedup-v0 and https://github.com/cloudflare/tls-tris/commits/pwu/tests-speedup-v1). Now it only fixes the Python 3 issues, pytest support and some potential resource leaks. |
'python2' does not exist on macOS, it is called 'python2.7'. Porting to
Python 3 is however more future-proof so do that instead. Rename the
file such that it can be run with pytest.
Do not listen on a fixed host port and use a random port instead.
Otherwise tests could fail if something is listening on those ports.
Note, with this modification, I noticed some ResourceWarnings after callingI removed the wait logic improvement due to the complexity and the fact that it triggers a bug in docker-py.exec_rundue to a Docker socket being leaked. These won't affect the test results though so I just left it there. See docker/docker-py#1293 (comment)I upgraded to Xenial to ensure a newer python3-six version to solve a failure importing docker. docker/docker-py#2294 (comment)
With the file rename, I can now do run a subset of the tests during development:
and add
-nautoto run those tests in parallel (requirespytest-xdist). This can complete all tests in 30 seconds (with 12 jobs). Another nice feature is the ability to pause when an assertion fails and inspect the context: