-
Couldn't load subscription status.
- Fork 5
Testing: overriding env with test env #321
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
Changes from all commits
84578a3
b63ff7b
ac7a136
4733919
25ad418
460b367
9eb8c5a
69278b5
89e80d4
6a829f5
4d0c11a
84dcee6
cd7bc8a
d204f1a
2edfd21
c4cf1e7
19567f3
4a55218
eaa661e
046221d
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 | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,30 @@ | ||||||||||||||||||||||||||
| ENVIRONMENT=testing | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| PROJECT_NAME="AI Platform" | ||||||||||||||||||||||||||
| STACK_NAME=ai-platform | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| #Backend | ||||||||||||||||||||||||||
| SECRET_KEY=changethis | ||||||||||||||||||||||||||
| FIRST_SUPERUSER=superuser@example.com | ||||||||||||||||||||||||||
| FIRST_SUPERUSER_PASSWORD=changethis | ||||||||||||||||||||||||||
| EMAIL_TEST_USER="test@example.com" | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| # Postgres | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| POSTGRES_SERVER=localhost | ||||||||||||||||||||||||||
| POSTGRES_PORT=5432 | ||||||||||||||||||||||||||
| POSTGRES_DB=ai_platform_test | ||||||||||||||||||||||||||
| POSTGRES_USER=postgres | ||||||||||||||||||||||||||
| POSTGRES_PASSWORD=postgres | ||||||||||||||||||||||||||
|
Comment on lines
+14
to
+18
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. 💡 Verification agent 🧩 Analysis chainDocker Compose gotcha: localhost won’t reach the DB container. If tests run via docker compose, POSTGRES_SERVER should be the service name (commonly "postgres"), not localhost. Otherwise the app container can’t reach the DB and tests will fail/flap. Consider documenting an alternative in the example: POSTGRES_SERVER=localhost
+# If running via Docker Compose, prefer the service name:
+# POSTGRES_SERVER=postgresRun this to verify Compose service naming and usage across the repo: 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "Searching for compose files and postgres service names..."
fd -HI 'docker-compose*.yml' -t f || true
fd -HI 'docker-compose*.yaml' -t f || true
fd -HI 'compose*.yml' -t f || true
fd -HI 'compose*.yaml' -t f || true
echo "Look for a postgres service definition..."
rg -n -A3 -i 'services:|^\s*postgres:|container_name:\s*postgres' || true
echo "Check where POSTGRES_SERVER is consumed..."
rg -n 'POSTGRES_SERVER' || true
echo "Check for DATABASE_URL usage that may bypass POSTGRES_*..."
rg -n 'DATABASE_URL' || trueLength of output: 1779 Update Docker Compose host to POSTGRES_SERVER=localhost
+# If running tests via Docker Compose, use the `db` service:
+# POSTGRES_SERVER=db
POSTGRES_PORT=5432
POSTGRES_DB=ai_platform_test
POSTGRES_USER=postgres
POSTGRES_PASSWORD=postgres
📝 Committable suggestion
Suggested change
🧰 Tools🪛 dotenv-linter (3.3.0)[warning] 15-15: [UnorderedKey] The POSTGRES_PORT key should go before the POSTGRES_SERVER key [warning] 16-16: [UnorderedKey] The POSTGRES_DB key should go before the POSTGRES_PORT key [warning] 18-18: [UnorderedKey] The POSTGRES_PASSWORD key should go before the POSTGRES_PORT key 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| # Configure these with your own Docker registry images | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| DOCKER_IMAGE_BACKEND=backend | ||||||||||||||||||||||||||
| DOCKER_IMAGE_FRONTEND=frontend | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| # AWS | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| AWS_ACCESS_KEY_ID=this_is_a_test_key | ||||||||||||||||||||||||||
| AWS_SECRET_ACCESS_KEY=this_is_a_test_key | ||||||||||||||||||||||||||
| AWS_DEFAULT_REGION=ap-south-1 | ||||||||||||||||||||||||||
| AWS_S3_BUCKET_PREFIX="bucket-prefix-name" | ||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,7 +6,7 @@ node_modules/ | |
| /playwright/.cache/ | ||
|
|
||
| # Environments | ||
| .env | ||
| .env* | ||
| .venv | ||
| env/ | ||
| venv/ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| import secrets | ||
| import warnings | ||
| import os | ||
| from typing import Annotated, Any, Literal | ||
| from typing import Any, Literal | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| from pydantic import ( | ||
| EmailStr, | ||
|
|
@@ -15,30 +15,30 @@ | |
| from typing_extensions import Self | ||
|
|
||
|
|
||
| def parse_cors(v: Any) -> list[str] | str: | ||
| if isinstance(v, str) and not v.startswith("["): | ||
| return [i.strip() for i in v.split(",")] | ||
| elif isinstance(v, list | str): | ||
| return v | ||
| raise ValueError(v) | ||
| def parse_cors(origins: Any) -> list[str] | str: | ||
| # If it's a plain comma-separated string, split it into a list | ||
| if isinstance(origins, str) and not origins.startswith("["): | ||
| return [origin.strip() for origin in origins.split(",")] | ||
| # If it's already a list or JSON-style string, just return it | ||
| elif isinstance(origins, (list, str)): | ||
| return origins | ||
| raise ValueError(f"Invalid CORS origins format: {origins!r}") | ||
|
|
||
|
|
||
| class Settings(BaseSettings): | ||
| model_config = SettingsConfigDict( | ||
| # Use top level .env file (one level above ./backend/) | ||
| env_file="../.env", | ||
| # env_file will be set dynamically in get_settings() | ||
| env_ignore_empty=True, | ||
| extra="ignore", | ||
| ) | ||
| LANGFUSE_PUBLIC_KEY: str | ||
| LANGFUSE_SECRET_KEY: str | ||
| LANGFUSE_HOST: str # 🇪🇺 EU region | ||
| OPENAI_API_KEY: str | ||
|
|
||
| API_V1_STR: str = "/api/v1" | ||
| SECRET_KEY: str = secrets.token_urlsafe(32) | ||
| # 60 minutes * 24 hours * 1 days = 1 days | ||
| ACCESS_TOKEN_EXPIRE_MINUTES: int = 60 * 24 * 1 | ||
| ENVIRONMENT: Literal["local", "staging", "production"] = "local" | ||
| ENVIRONMENT: Literal[ | ||
| "development", "testing", "staging", "production" | ||
| ] = "development" | ||
|
|
||
| PROJECT_NAME: str | ||
| SENTRY_DSN: HttpUrl | None = None | ||
|
|
@@ -84,7 +84,7 @@ def _check_default_secret(self, var_name: str, value: str | None) -> None: | |
| f'The value of {var_name} is "changethis", ' | ||
| "for security, please change it, at least for deployments." | ||
| ) | ||
| if self.ENVIRONMENT == "local": | ||
| if self.ENVIRONMENT in ["development", "testing"]: | ||
| warnings.warn(message, stacklevel=1) | ||
| else: | ||
| raise ValueError(message) | ||
|
|
@@ -100,4 +100,17 @@ def _enforce_non_default_secrets(self) -> Self: | |
| return self | ||
|
|
||
|
|
||
| settings = Settings() # type: ignore | ||
| def get_settings() -> Settings: | ||
| """Get settings with appropriate env file based on ENVIRONMENT.""" | ||
| environment = os.getenv("ENVIRONMENT", "development") | ||
|
|
||
| # Determine env file | ||
| env_files = {"testing": "../.env.test", "development": "../.env"} | ||
| env_file = env_files.get(environment, "../.env") | ||
|
|
||
| # Create Settings instance with the appropriate env file | ||
| return Settings(_env_file=env_file) | ||
|
Comment on lines
+103
to
+112
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. 💡 Verification agent 🧩 Analysis chainRelative
Apply within this hunk: -def get_settings() -> Settings:
- """Get settings with appropriate env file based on ENVIRONMENT."""
- environment = os.getenv("ENVIRONMENT", "development")
-
- # Determine env file
- env_files = {"testing": "../.env.test", "development": "../.env"}
- env_file = env_files.get(environment, "../.env")
-
- # Create Settings instance with the appropriate env file
- return Settings(_env_file=env_file)
+@lru_cache
+def get_settings() -> Settings:
+ """Get settings with appropriate env file based on ENVIRONMENT."""
+ environment = os.getenv("ENVIRONMENT", "development")
+
+ # Resolve project root from this file: backend/app/core/config.py -> repo root
+ base_dir = Path(__file__).resolve().parents[3]
+
+ # Map env → filename; staging/prod typically rely on real env vars, so no file
+ mapping = {
+ "testing": base_dir / ".env.test",
+ "development": base_dir / ".env",
+ }
+ env_file_path = mapping.get(environment)
+ if env_file_path is not None and not env_file_path.exists():
+ # Fall back to OS env only if the file isn't present
+ env_file_path = None
+
+ return Settings(_env_file=env_file_path)Add these imports at the top of the file: from functools import lru_cache
from pathlib import PathRecommended (outside this hunk): provide an explicit reload hook for tests and fixtures that currently do def reload_settings() -> Settings:
"""Clear cached settings and rebuild after dotenv changes in tests."""
get_settings.cache_clear()
return get_settings()And replace any I can push a follow-up patch updating conftest.py to call To confirm the risks and cleanup opportunities: Also applies to: 113-114 🏁 Script executed: #!/bin/bash
# Where is settings reinitialized?
rg -nP -C2 --type=py 'settings\.__init__\s*\('
# Do we accidentally rely on CWD for dotenv files elsewhere?
rg -nP -C2 --type=py '\.\./\.env(\.test)?'
# Validate the actual presence of example/test dotenv at repo root
fd -H --max-depth 2 '.env*' -x ls -al {} || trueLength of output: 2080 Absolute path resolution and caching for To avoid mis-resolved relative paths when the CWD changes (pytest, Docker, IDE), refactor both modules to compute the project root from • backend/app/core/config.py -from functools import lru_cache
+from functools import lru_cache
from os import getenv
-from pathlib import Path
+from pathlib import Path
@lru_cache
def get_settings() -> Settings:
"""Get settings with appropriate env file based on ENVIRONMENT."""
environment = getenv("ENVIRONMENT", "development")
# Resolve project root from this file
- base_dir = Path(__file__).resolve().parents[3]
+ base_dir = Path(__file__).resolve().parents[3]
# Map env → filename; staging/prod rely on real env vars
mapping = {
"testing": base_dir / ".env.test",
"development": base_dir / ".env",
}
env_file_path = mapping.get(environment)
if env_file_path is not None and not env_file_path.exists():
# Fall back to OS env only if file isn't present
env_file_path = None
return Settings(_env_file=env_file_path)• backend/app/load_env.py -from dotenv import load_dotenv
-# Use the same path as config.py expects (one level above backend/)
-env_file = "../.env"
-if env == "testing":
- env_file = "../.env.test"
-load_dotenv(env_file)
+from dotenv import load_dotenv
+from pathlib import Path
+# Resolve project root (load_env.py → backend/app → backend → repo root)
+base_dir = Path(__file__).resolve().parents[2]
+env_files = {
+ "testing": base_dir / ".env.test",
+ "development": base_dir / ".env",
+}
+env_file_path = env_files.get(env, base_dir / ".env")
+if env_file_path.exists():
+ load_dotenv(str(env_file_path))
+else:
+ # Skip file-based loading; rely on OS env vars
+ passOptional (for tests): add a reload hook instead of calling def reload_settings() -> Settings:
"""Clear cached settings and rebuild after dotenv changes in tests."""
get_settings.cache_clear()
return get_settings()Note: no occurrences of 🤖 Prompt for AI Agents |
||
|
|
||
|
|
||
| # Export settings instance | ||
| settings = get_settings() | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| import os | ||
| from dotenv import load_dotenv | ||
|
|
||
|
|
||
| def load_environment(): | ||
| env = os.getenv("ENVIRONMENT", "development") | ||
|
|
||
| # Use the same path as config.py expects (one level above backend/) | ||
| env_file = "../.env" | ||
| if env == "testing": | ||
| env_file = "../.env.test" | ||
|
|
||
| load_dotenv(env_file) |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,6 +2,16 @@ | |||||||||||||||||||||||||
| set -e | ||||||||||||||||||||||||||
| set -x | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| # Set environment for testing | ||||||||||||||||||||||||||
| export ENVIRONMENT=testing | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| python app/tests_pre_start.py | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| # Run pending migrations for test database | ||||||||||||||||||||||||||
| uv run alembic upgrade head | ||||||||||||||||||||||||||
| if [ $? -ne 0 ]; then | ||||||||||||||||||||||||||
| echo 'Error: Test database migrations failed' | ||||||||||||||||||||||||||
| exit 1 | ||||||||||||||||||||||||||
| fi | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
Comment on lines
+10
to
+16
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. 💡 Verification agent 🧩 Analysis chainRedundant exit-code check under set -e With set -e, the script already exits on a non-zero status. The manual $? check is unnecessary. Apply this diff: -# Run pending migrations for test database
-uv run alembic upgrade head
-if [ $? -ne 0 ]; then
- echo 'Error: Test database migrations failed'
- exit 1
-fi
+# Run pending migrations for test database
+uv run alembic upgrade headAlso verify that the uv CLI is available in all environments running this script, or fall back to invoking Alembic directly: 🏁 Script executed: #!/usr/bin/env bash
# Verify uv availability and suggest a fallback to plain alembic if missing
if ! command -v uv >/dev/null 2>&1; then
echo "Warning: 'uv' not found. Consider using 'alembic upgrade head' instead of 'uv run alembic upgrade head'."
fiLength of output: 252 Remove redundant exit-code check and ensure Alembic runs whether or not With • File: -# Run pending migrations for test database
-uv run alembic upgrade head
-if [ $? -ne 0 ]; then
- echo 'Error: Test database migrations failed'
- exit 1
-fi
+# Run pending migrations for test database
+if command -v uv >/dev/null 2>&1; then
+ uv run alembic upgrade head
+else
+ alembic upgrade head
+fiThis preserves the “fail fast” behavior of 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||
| bash scripts/test.sh "$@" | ||||||||||||||||||||||||||
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.
ENVIRONMENTlikely ignored – backend code expectsAPP_ENVAccording to the PR description and
conftest.py, the configuration loader branches onAPP_ENV="testing".Because this file sets
ENVIRONMENT=testing, the loader will never see the flag and will fall back to the default (usually “production”) settings, defeating the purpose of the test-isolation work.If you still need
ENVIRONMENTfor other tooling, keep both lines.📝 Committable suggestion
🤖 Prompt for AI Agents