tests: factor out base nginx-proxy config and enable local testing on macOS / Darwin #2570
Conversation
|
Local run on a MacBook Pro M1 with PyCharm:
|
|
Ready to review if you have some spare time @SchoNie |
There was a problem hiding this comment.
Nice work good improvements!
8000
Code looks good for me.
A few missed strings and 2 grammar suggestions.
Environment variables (VIRTUAL_HOST and some other) could use some linting. Good time to choose the consistent standard as there are already lot of changes in each file.
test/test_fallback/test_fallback.data/nohttp-with-missing-cert.yml
Outdated
Show resolved
Hide resolved
| return | ||
| docker_compose_up(docker_compose_file) | ||
| docker_compose_up(docker_compose_files, project_name) | ||
| self._networks = connect_to_all_networks() | ||
| wait_for_nginxproxy_to_be_ready() | ||
| time.sleep(3) # give time to containers to be ready |
There was a problem hiding this comment.
Not really related to this PR but why we sleep 3 seconds here?
wait_for_nginxproxy_to_be_ready() checks already to have nginx-proxy ready (real-time stream)
So that sleeps just seems to be for the 'web' container. But I don't think that needs 3 seconds.
And as we do like 90 compose up's thats already 270 seconds of waiting (4,5 min).
A good win to reduce test time I think.
There was a problem hiding this comment.
That's something I tried to work on, one problem is that in its current state wait_for_nginxproxy_to_be_ready() does not seem to really tell us if the proxy is actually ready or not.
def wait_for_nginxproxy_to_be_ready():
"""
If one (and only one) container started from image nginxproxy/nginx-proxy:test is found,
wait for its log to contain substring "Watching docker events"
"""
containers = docker_client.containers.list(filters={"ancestor": "nginxproxy/nginx-proxy:test"})
if len(containers) != 1:
return
container = containers[0]
for line in container.logs(stream=True):
if b"Watching docker events" in line:
logging.debug("nginx-proxy ready")
break- If
docker_client.containers.list()found zero or more than one container, the function will return without any further check. - I'm not certain that
for line in container.logs(stream=True):mean that the function will wait for the expected log line to appear, for me it will just iterate over the the log lines generated by the container up to that point, return early if it findWatching docker eventsor return anyway when it reach the end of the generator. - the appearance of the
Watching docker eventslog line mean that docker-gen is ready, not that nginx has loaded the docker-gen generated conf, which is what we're actually testing.
We're automatically doing a 9s backoff on 404 or 503 when using the nginxproxy fixture's get() which kind of automatically wait for the containers to be ready, but not all tests are using this.
There was a problem hiding this comment.
Ah you are right, that ready state is indeed based of docker-gen.
Changing the bytestring to something else makes the for loop block. So it will wait. But the Watching docker events is probably always there.
Adding follow=True in for line in container.logs(stream=True, follow=True): makes the generator follow new events in the stream.
This comment was marked as off-topic.
This comment was marked as off-topic.
7ae6ff2 to
f9692e2
Compare
Co-authored-by: Niek <100143256+SchoNie@users.noreply.github.com>
f9692e2 to
aa8145b
Compare
|
I think I'll do a linting pass on every Python files later 👍 |
Okay, this one might be a bit hard to read.
Tests don't work on macOS / Darwin because they all resolve the test domains to the IP of the nginx-proxy container (thanks to
monkey_patch_urllib_dns_resolver()), but it's explicitly not possible to route traffic directly to containers with Docker Desktop.The solution to this is "simply" to use port forwarding and resolve the test domains to localhost's IP instead of the IP of the nginx-proxy container.
This in turn means port publishing must be added to the nginx-proxy container on almost every test compose file:
We're already duplicating the nginx-proxy base config on every compose file (with the container sometime named
sut, sometimenginx-proxy, sometime something else ...) so I decided to change the way thedocker_composefixture work to reduce the quantity of boilerplate code.By default it will use those files for every test module:
test/compose.base.yml(base nginx-proxy config reused for most tests)test/{test_dir}/compose.base.override.ymlif the file exist (override and/or containers for all tests in a given dir)test/{test_dir}/{test_module_name}.yml(override and/or containers for a specific test module)The compose files are merged in this order.
This automatic merge can be bypassed by using a file named
test/{test_dir}/{test_module_name}.base.yml. When this file exist, it will be the only used by the test and no merge will automatically occur.The
docker_composefixture also set thePYTEST_MODULE_PATHenvironment variable to the absolute path of the current test directory, so it can be used to mount files or directory relatives to the current test.The support for test compose files with the
.yamlextension has been removed because it did not serve any purpose.The support for test compose files named
docker-compose.ymlthat were treated as the default compose file for a whole test directory has also been removed because it wasn't really used.I've never been fond of naming the nginx-proxy service
sut, so I've standardized tonginx-proxy.