-
Notifications
You must be signed in to change notification settings - Fork 0
fix: pending issue with tests on mac #548
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,4 +1,5 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import os | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import subprocess | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from tempfile import NamedTemporaryFile | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from unittest.mock import patch | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -27,16 +28,63 @@ def vcr_config(): | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "filter_headers": [("authorization", "DUMMY_API_KEY")], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @pytest.fixture(scope="session") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def docker_compose_file(pytestconfig): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return os.path.join(str(pytestconfig.rootdir), "tests", "docker-compose.test.yml") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @pytest.fixture(scope="session") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def docker_compose_project_name(): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Use a consistent project name to avoid creating new networks each run.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return "reflector_test" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @pytest.fixture(scope="session", autouse=True) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def cleanup_docker_resources(): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Clean up Docker test resources before and after test session.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def cleanup(): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Stop and remove any existing test containers | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # This will also remove the reflector_test network if it exists | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| subprocess.run( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| [ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "docker", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "compose", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "-p", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "reflector_test", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "down", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "-v", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "--remove-orphans", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| capture_output=True, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cwd=os.path.dirname(os.path.dirname(os.path.abspath(__file__))), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+49
to
+61
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Add a timeout parameter to the subprocess call to prevent tests from hanging indefinitely if Docker operations take too long or get stuck. [general, importance: 7]
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Clean up any unused networks (includes orphaned pytest networks) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # This is safe - only removes networks with no attached containers | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| subprocess.run(["docker", "network", "prune", "-f"], capture_output=True) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+63
to
+64
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Check the return code of the subprocess call to ensure the Docker command executed successfully. If the command fails, it could silently fail and cause test issues. [general, importance: 7]
Suggested change
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, i don't want the project to prune my network, some network can be created without being attached to a container, does not mean you want to prune. Filter to pytest then |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Clean before tests | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cleanup() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| yield | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Clean after tests | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cleanup() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @pytest.fixture(scope="session") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def postgres_service(docker_ip, docker_services): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Ensure that PostgreSQL service is up and responsive.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| port = docker_services.port_for("postgres_test", 5432) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| port = docker_services.port_for("postgres_test", 5432) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| except Exception as e: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # If Docker services fail to start, clean up and retry | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| subprocess.run(["docker", "network", "prune", "-f"], capture_output=True) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| subprocess.run( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ["docker", "compose", "-p", "reflector_test", "down", "-v"], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| capture_output=True, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise pytest.skip(f"Docker services failed to start: {e}") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+82
to
+87
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Add error handling for the cleanup commands in the exception handler. If these cleanup commands fail, they could mask the original error and make debugging more difficult. [general, importance: 8]
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def is_responsive(): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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.
No, again this is specific to you because you change your env to have stuff running in a docker, then add stuff about how to run without your env again.
We could do the same way for REDIS_HOST to be populated with pytest-docker the same way as postgres.