Fix test DB connection: use DATABASE_URL with dotenv#2517
Open
jucor wants to merge 1 commit into
Open
Conversation
This was referenced Mar 30, 2026
jucor
changed the title
jucor
force-pushed
the
spr/edge/b9062b50
branch
2 times, most recently
from
8dc9313 to
abee548
Compare
jucor
force-pushed
the
spr/edge/ea747196
branch
from
07c707d to
e046e08
Compare
jucor
force-pushed
the
spr/edge/b9062b50
branch
from
abee548 to
517908a
Compare
ballPointPenguin
approved these changes
Apr 26, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Updates Delphi’s Python test harness and DB/service connectivity so integration-style tests fail fast (instead of hanging) and Postgres connections use DATABASE_URL loaded via dotenv, aligning with broader repo conventions.
Changes:
- Switch Postgres test connection logic to prefer
DATABASE_URL(dotenv-loaded) and add shortconnect_timeoutto avoid hangs. - Add
require_dynamodb()/require_s3()helpers and apply them in tests to fail/skip quickly when Docker services are down. - Add per-test timing visibility via
pytest-timestamper, plus a few small test hardening tweaks (deferred imports, extra mocking).
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
delphi/tests/test_umap_narrative_pipeline.py |
Defers heavy imports until test execution to reduce xdist collection-time risk. |
delphi/tests/test_postgres_real_data.py |
Loads dotenv + uses DATABASE_URL (or fallback) and adds DynamoDB precheck + faster Postgres connect timeout. |
delphi/tests/test_pakistan_conversation.py |
Converts DB-unavailable behavior from skip to fail. |
delphi/tests/test_minio_access.py |
Adds S3 availability precheck to avoid hangs. |
delphi/tests/test_math_pipeline_runs_e2e.py |
Adds DynamoDB availability precheck in fixture. |
delphi/tests/test_batch_id.py |
Adds DynamoDB availability precheck in fixture and minor formatting fix. |
delphi/tests/test_501_calculate_comment_extremity.py |
Mocks PostgresClient to avoid real DB connections when Docker is unavailable. |
delphi/tests/profile_postgres_data.py |
Adds connect_timeout to profiling DB connection. |
delphi/tests/conftest.py |
Introduces require_dynamodb() / require_s3() helpers for fast failure/skip behavior. |
delphi/scripts/regression_download.py |
Adds connect_timeout to Postgres connection. |
delphi/pyproject.toml |
Adds pytest-timestamper to dev dependencies. |
delphi/polismath/run_math_pipeline.py |
Adds connect_timeout for Postgres connections. |
delphi/polismath/regression/utils.py |
Fixes compute_all_stages return type annotation. |
delphi/polismath/regression/comparer.py |
Normalizes timing_stats_compared structure to avoid None handling issues. |
delphi/polismath/database/postgres.py |
Adds SQLAlchemy engine connect_timeout and pool_pre_ping. |
.github/workflows/python-ci.yml |
Removes redundant POSTGRES_* env injection in the exec step (relies on .env). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
283
to
+287
| pool_size=self.config.pool_size, | ||
| max_overflow=self.config.max_overflow, | ||
| pool_recycle=300, # Recycle connections after 5 minutes | ||
| connect_args={"connect_timeout": 5}, | ||
| pool_pre_ping=True, |
Comment on lines
+26
to
+34
| def require_dynamodb( | ||
| endpoint: str | None = None, | ||
| timeout: float = 3.0, | ||
| ) -> None: | ||
| """Fail the test immediately if DynamoDB is not responding. | ||
|
|
||
| Performs a ``list_tables`` call with short timeouts and zero retries | ||
| so the test fails in seconds rather than hanging indefinitely. | ||
| """ |
Comment on lines
+116
to
+120
| name = os.environ.get('DATABASE_NAME') | ||
| user = os.environ.get('DATABASE_USER') | ||
| password = os.environ.get('DATABASE_PASSWORD', '') | ||
| if host and name and user: | ||
| database_url = f"postgres://{user}:{password}@{host}:{port}/{name}" |
## Summary - **Absorbed PR #2434** (DB connect_timeout): adds `connect_timeout=5` to all DB connections so tests fail fast instead of hanging when Postgres is unavailable. Reverts timeout from production code (a 5s timeout could break real deployments), keeping it only in test code. - Adds `require_dynamodb()` and `require_s3()` helpers to conftest that probe services with short timeouts and `pytest.fail()` immediately — applied to all tests that previously hung when Docker services were down. - Adds `pytest-timestamper` for per-test timing visibility. - Fixes `test_postgres_real_data.py` to use `DATABASE_URL` (consistent with all other delphi Python code) via `python-dotenv`. Refs #2442 ## Test plan - [x] `test_conversation_from_postgres` passes - [x] `test_pakistan_conversation_batch` passes (9 min, 400K votes) - [x] Full delphi test suite: 294 passed, no regressions 🤖 Generated with [Claude Code](https://claude.com/claude-code) ## Squashed commits - Add connect_timeout to all DB connections, fail tests on DB unavailable - Fail tests fast when services are unavailable instead of hanging - Fix conftest docstring: require_dynamodb/require_s3, not require_service - Skip (not fail) test_minio_access when MinIO is unavailable - Fail test_conversation_from_postgres fast when DynamoDB is unavailable - Fail test_run_math_pipeline_e2e fast when DynamoDB is unavailable - Add pytest-timestamper and fix test_501 hang on Docker unavailable - Defer heavy import in test_umap_narrative_pipeline (defensive) - Address Copilot review: fix timing_stats_compared None bug, fix return type - Fix test DB connection: use DATABASE_URL with dotenv - Fix CI: use find_dotenv() and fall back to DATABASE_* vars - Restore get_or_compute_conversation fixture and test reordering hook - Remove dead xdist_group markers from make_dataset_params commit-id:b9062b50
jucor
changed the title
Delphi Coverage Report
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
connect_timeout=5to all DB connections so tests fail fast instead of hanging when Postgres is unavailable. Reverts timeout from production code (a 5s timeout could break real deployments), keeping it only in test code.require_dynamodb()andrequire_s3()helpers to conftest that probe services with short timeouts andpytest.fail()immediately — applied to all tests that previously hung when Docker services were down.pytest-timestamperfor per-test timing visibility.test_postgres_real_data.pyto useDATABASE_URL(consistent with all other delphi Python code) viapython-dotenv.Refs #2442
Test plan
test_conversation_from_postgrespassestest_pakistan_conversation_batchpasses (9 min, 400K votes)🤖 Generated with Claude Code
Squashed commits
commit-id:b9062b50
Stack: