fix: pending issue with tests on mac #548
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
![pr-agent-monadical[bot]](https://avatars.githubusercontent.com/u/29933591?s=60&v=4){kind=small}
| # This is safe - only removes networks with no attached containers | ||
| subprocess.run(["docker", "network", "prune", "-f"], capture_output=True) |
There was a problem hiding this comment.
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]
| # This is safe - only removes networks with no attached containers | |
| subprocess.run(["docker", "network", "prune", "-f"], capture_output=True) | |
| # Clean up any unused networks (includes orphaned pytest networks) | |
| # This is safe - only removes networks with no attached containers | |
| result = subprocess.run(["docker", "network", "prune", "-f"], capture_output=True) | |
| if result.returncode != 0: | |
| print(f"Warning: Docker network prune failed: {result.stderr.decode()}") |
There was a problem hiding this comment.
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
| 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}") |
There was a problem hiding this comment.
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]
| 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}") | |
| # If Docker services fail to start, clean up and retry | |
| try: | |
| subprocess.run(["docker", "network", "prune", "-f"], capture_output=True, check=False) | |
| subprocess.run( | |
| ["docker", "compose", "-p", "reflector_test", "down", "-v"], | |
| capture_output=True, | |
| check=False, | |
| ) | |
| except Exception as cleanup_error: | |
| print(f"Warning: Cleanup after Docker failure also failed: {cleanup_error}") | |
| raise pytest.skip(f"Docker services failed to start: {e}") |
| 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__))), | ||
| ) |
There was a problem hiding this comment.
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]
| 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__))), | |
| ) | |
| 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__))), | |
| timeout=60, # Add timeout to prevent hanging | |
| ) |
| # This is safe - only removes networks with no attached containers | ||
| subprocess.run(["docker", "network", "prune", "-f"], capture_output=True) |
There was a problem hiding this comment.
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
| REDIS_HOST=localhost CELERY_BROKER_URL=redis://localhost:6379/1 CELERY_RESULT_BACKEND=redis://localhost:6379/1 uv run pytest | ||
|
|
||
| # Run specific test file | ||
| uv run pytest tests/test_transcripts.py | ||
| REDIS_HOST=localhost CELERY_BROKER_URL=redis://localhost:6379/1 CELERY_RESULT_BACKEND=redis://localhost:6379/1 uv run pytest tests/test_transcripts.py | ||
|
|
||
| # Run tests with verbose output | ||
| uv run pytest -v | ||
| REDIS_HOST=localhost CELERY_BROKER_URL=redis://localhost:6379/1 CELERY_RESULT_BACKEND=redis://localhost:6379/1 uv run pytest -v |
There was a problem hiding this comment.
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.
deardarlingoose
changed the title
User description
https://www.loom.com/share/4dcfb2729e4a4a308a7d09f5a5e064c0?sid=c4139292-4f51-4876-9aac-fa0325215b69
PR Type
Bug fix, Tests
Description
Fixed Docker network cleanup in tests
Added explicit environment variables for Mac tests
Improved test reliability with better error handling
Added consistent Docker project naming
Changes walkthrough 📝
conftest.py
Improve Docker test reliability with cleanup and error handlingserver/tests/conftest.py
proliferation
CLAUDE.md
Update test documentation with required environment variablesCLAUDE.md
parameters