Skip to content

tests: DockerComposer contextmanager improvements #2591

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

Merged
merged 8 commits into from
Mar 10, 2025

Conversation

SchoNie
Copy link
Contributor

@SchoNie SchoNie commented Feb 28, 2025

Improvements to the DockerComposer contextmanager as mentioned in #2584 (review)

I run the test suite as container, and found that if errors occur or on keyboardinterrupt the containers and/or networks are not cleaned properly leaving ghost stuff around and preventing new test runs.
In Github CI not a problem because after tests the runner is terminated.

  • Improve the DockerComposer contextmanager to make sure teardown happens by using try except block.
  • Move the self. declarations up so we are sure they are set even if an exception occurs.
  • set self._networks
  • self._docker_compose_file = None >> self._docker_compose_files = None (files typo)
  • Change pytest.fail to a logging error because pytest.fail makes DockerComposer ContextManager exit and no _down teardown is executed.
  • Add KeyboardInterrupt logic
  • skip test_proxy-host-network-mode when PYTEST_RUNNING_IN_CONTAINER as this does not work

A review from @pini-gh is highly appreciated to make sure Debian Stable is still OK.

@pini-gh
Copy link
Contributor

pini-gh commented Feb 28, 2025

The tests run fine on Debian.

Copy link
Member

@buchdag buchdag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to run this on Darwin / macOS but somehow tests are broken again on my mac with PyCharm (not this branch specifically, they're broken on the main branch too).

I'll try again tomorrow, if I still can't get the tests to work I'll merge the PR anyway.

SchoNie added 8 commits March 10, 2025 09:53
-Improve the DockerComposer contextmanager to make sure teardown happens by using try except block.
-Move the self. declarations up so we are sure they are set even if an exception occurs.
-set self._networks
Change pytest.fail to a logging error because pytest.fail makes DockerComposer ContextManager exit and no _down teardown is executed.
make sure we teardown on KeyboardInterrupt
If running from a container host-network-mode test fail because we cannot connect to host network. This prevents the cleanup.
So make sure we always return network for later removal.

Explicitly return None for readability
To make same as in test_wildcard-host.yml
…INER

Connecting to host network not supported when pytest is running in container
@SchoNie SchoNie force-pushed the tests-contextmanager-fixes branch from d8a4c02 to 005b0bd Compare March 10, 2025 08:54
@buchdag buchdag merged commit 1842acb into nginx-proxy:main Mar 10, 2025
2 checks passed
@buchdag buchdag added the type/test PR that add missing tests or correct existing tests label Mar 10, 2025
@SchoNie SchoNie deleted the tests-contextmanager-fixes branch March 11, 2025 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/test PR that add missing tests or correct existing tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants