Testing: overriding env with test env #284
Conversation
WalkthroughThis change set introduces improved test environment management and database connection handling. It adds a Changes
Sequence Diagram(s)sequenceDiagram
participant Pytest
participant conftest.py
participant utils.py
participant Database
Pytest->>conftest.py: Start session (autouse fixture)
conftest.py->>utils.py: load_environment(".env.test")
utils.py->>utils.py: Load env vars, validate, check "test" DB
conftest.py->>Database: Seed baseline data
Pytest->>conftest.py: Start test (autouse fixture)
conftest.py->>Database: Run test with session
conftest.py->>Database: Dispose engine after test
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/tests/conftest.py(1 hunks)backend/app/tests/utils/utils.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/app/tests/conftest.py (2)
backend/app/api/deps.py (1)
get_db(33-35)backend/app/tests/utils/utils.py (2)
get_superuser_token_headers(38-47)load_environment(90-98)
🪛 Ruff (0.12.2)
backend/app/tests/conftest.py
4-4: os imported but unused
Remove unused import: os
(F401)
8-8: dotenv.load_dotenv imported but unused
Remove unused import: dotenv.load_dotenv
(F401)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
🔇 Additional comments (3)
backend/app/tests/utils/utils.py (1)
5-6: LGTM! Imports are correctly added for the new functionality.The
osanddotenv.load_dotenvimports are properly added to support the new environment loading functionality.backend/app/tests/conftest.py (2)
19-21: LGTM! Fixture implementation is correct.The
load_test_envfixture is properly configured with session scope and autouse enabled, ensuring test environment variables are loaded automatically before any tests run. The implementation correctly calls theload_environmentfunction from the utils module.
15-15: LGTM! Import is correctly added.The import of
load_environmentfrom the utils module is properly added to support the new fixture functionality.
nishika26
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.env.test.example (1)
8-13: Keep quoting style and key order consistentSome values (
EMAIL_TEST_USER,PROJECT_NAME) are quoted while adjacent ones are not, and linter warnings flag the ordering.
Uniform style improves readability and avoids false-positive drift with tools likedotenv-linter. Consider:-FIRST_SUPERUSER=superuser@example.com -FIRST_SUPERUSER_PASSWORD=changethis -EMAIL_TEST_USER="test@example.com" +EMAIL_TEST_USER=test@example.com +FIRST_SUPERUSER=superuser@example.com +FIRST_SUPERUSER_PASSWORD=changethis(Similarly adjust other sections.)
This is cosmetic but helps future diff hygiene.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.env.test.example(1 hunks)
🧰 Additional context used
🪛 dotenv-linter (3.3.0)
.env.test.example
[warning] 10-10: [UnorderedKey] The FIRST_SUPERUSER key should go before the SECRET_KEY key
[warning] 11-11: [UnorderedKey] The FIRST_SUPERUSER_PASSWORD key should go before the SECRET_KEY key
[warning] 12-12: [QuoteCharacter] The value has quote characters (', ")
[warning] 12-12: [UnorderedKey] The EMAIL_TEST_USER key should go before the FIRST_SUPERUSER key
[warning] 17-17: [UnorderedKey] The POSTGRES_PORT key should go before the POSTGRES_SERVER key
[warning] 18-18: [UnorderedKey] The POSTGRES_DB key should go before the POSTGRES_PORT key
[warning] 20-20: [UnorderedKey] The POSTGRES_PASSWORD key should go before the POSTGRES_PORT key
[warning] 33-33: [UnorderedKey] The AWS_DEFAULT_REGION key should go before the AWS_SECRET_ACCESS_KEY key
[warning] 34-34: [SpaceCharacter] The line has spaces around equal sign
[warning] 34-34: [UnorderedKey] The AWS_S3_BUCKET_PREFIX key should go before the AWS_SECRET_ACCESS_KEY key
[warning] 38-38: [QuoteCharacter] The value has quote characters (', ")
[warning] 39-39: [QuoteCharacter] The value has quote characters (', ")
[warning] 39-39: [UnorderedKey] The LANGFUSE_PUBLIC_KEY key should go before the OPENAI_API_KEY key
[warning] 40-40: [QuoteCharacter] The value has quote characters (', ")
[warning] 40-40: [UnorderedKey] The LANGFUSE_SECRET_KEY key should go before the OPENAI_API_KEY key
[warning] 41-41: [QuoteCharacter] The value has quote characters (', ")
[warning] 41-41: [UnorderedKey] The LANGFUSE_HOST key should go before the LANGFUSE_PUBLIC_KEY key
[warning] 45-45: [QuoteCharacter] The value has quote characters (', ")
🔇 Additional comments (1)
.env.test.example (1)
38-42: Placeholders are fine, but double-check secrets aren’t realThe example keys for OpenAI and Langfuse are clearly dummy values—good. Ensure the real
.env.testis git-ignored so secrets never land in the repo.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/app/tests/conftest.py (1)
9-9: Remove unused import.The
load_dotenvimport is not used directly in this file since environment loading is handled by theload_environmentfunction.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/tests/conftest.py(2 hunks)backend/app/tests/utils/utils.py(2 hunks)
🧬 Code Graph Analysis (1)
backend/app/tests/conftest.py (1)
backend/app/tests/utils/utils.py (1)
load_environment(98-106)
🪛 Ruff (0.12.2)
backend/app/tests/conftest.py
9-9: dotenv.load_dotenv imported but unused
Remove unused import: dotenv.load_dotenv
(F401)
22-22: Undefined name load_environment
(F821)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/tests/utils/utils.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/app/tests/conftest.py (1)
backend/app/tests/utils/utils.py (1)
load_environment(98-106)
🪛 Ruff (0.12.2)
backend/app/tests/conftest.py
9-9: dotenv.load_dotenv imported but unused
Remove unused import: dotenv.load_dotenv
(F401)
22-22: Undefined name load_environment
(F821)
🔇 Additional comments (1)
backend/app/tests/conftest.py (1)
20-23: Well-designed fixture for test environment isolation.The session-scoped, auto-use fixture appropriately ensures test environment variables are loaded once at the beginning of test execution, achieving the PR objective of environment isolation.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.env.test.example (1)
21-21: Remove the spaces around the=to prevent an invalid key name.The current line has spaces around the equal sign which can cause the key to include trailing spaces, making environment variable lookups fail silently.
-AWS_S3_BUCKET_PREFIX = "bucket-prefix-name" +AWS_S3_BUCKET_PREFIX="bucket-prefix-name"backend/app/tests/utils/utils.py (1)
108-108: Settings reinitialization may not work properly.The
settings.__init__()call may not properly reload environment variables from the newly loaded .env.test file, depending on how the Settings class is implemented. Based on the Settings class inbackend/app/core/config.py, it uses Pydantic's BaseSettings which loads environment variables during instantiation.Consider creating a new settings instance or using a more reliable approach:
- settings.__init__() + # Force reload by creating new settings instance + from importlib import reload + from app.core import config + reload(config)Alternatively, verify that the current approach works correctly with your Settings implementation.
🧹 Nitpick comments (3)
.env.test.example (1)
6-6: Remove unnecessary quotes from environment variable values.Environment variables don't need quotes unless the values contain spaces or special characters that require escaping. The quotes will be included as part of the actual values.
-EMAIL_TEST_USER="test@example.com" +EMAIL_TEST_USER=test@example.com-OPENAI_API_KEY="this_is_not_a_secret" +OPENAI_API_KEY=this_is_not_a_secretAlso applies to: 25-25
backend/app/tests/utils/utils.py (1)
13-13: Remove unused import.The
Settingsimport is not used in this file.-from app.core.config import settings, Settings +from app.core.config import settingsbackend/app/tests/conftest.py (1)
35-35: Remove debug print statements before merging.The debug print statements should be removed before merging to production, as they will clutter test output.
- print("enginer_1", engine)- print("enginer_2", engine)- print("Seeding baseline data...")Also note the typo in "enginer" - should be "engine" if the print statements are kept temporarily.
Also applies to: 54-54, 56-56
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.env.test.example(1 hunks)backend/app/tests/conftest.py(2 hunks)backend/app/tests/utils/utils.py(2 hunks)
🧬 Code Graph Analysis (1)
backend/app/tests/utils/utils.py (1)
backend/app/core/config.py (1)
Settings(26-100)
🪛 Ruff (0.12.2)
backend/app/tests/utils/utils.py
13-13: app.core.config.Settings imported but unused
Remove unused import: app.core.config.Settings
(F401)
🪛 dotenv-linter (3.3.0)
.env.test.example
[warning] 4-4: [UnorderedKey] The FIRST_SUPERUSER key should go before the SECRET_KEY key
[warning] 5-5: [UnorderedKey] The FIRST_SUPERUSER_PASSWORD key should go before the SECRET_KEY key
[warning] 6-6: [QuoteCharacter] The value has quote characters (', ")
[warning] 6-6: [UnorderedKey] The EMAIL_TEST_USER key should go before the FIRST_SUPERUSER key
[warning] 11-11: [UnorderedKey] The POSTGRES_PORT key should go before the POSTGRES_SERVER key
[warning] 12-12: [UnorderedKey] The POSTGRES_DB key should go before the POSTGRES_PORT key
[warning] 14-14: [UnorderedKey] The POSTGRES_PASSWORD key should go before the POSTGRES_PORT key
[warning] 20-20: [UnorderedKey] The AWS_DEFAULT_REGION key should go before the AWS_SECRET_ACCESS_KEY key
[warning] 21-21: [SpaceCharacter] The line has spaces around equal sign
[warning] 21-21: [UnorderedKey] The AWS_S3_BUCKET_PREFIX key should go before the AWS_SECRET_ACCESS_KEY key
[warning] 25-25: [QuoteCharacter] The value has quote characters (', ")
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/app/tests/utils/utils.py (1)
backend/app/core/config.py (1)
Settings(26-100)
🪛 Ruff (0.12.2)
backend/app/tests/utils/utils.py
13-13: app.core.config.Settings imported but unused
Remove unused import: app.core.config.Settings
(F401)
🪛 dotenv-linter (3.3.0)
.env.test.example
[warning] 4-4: [UnorderedKey] The FIRST_SUPERUSER key should go before the SECRET_KEY key
[warning] 5-5: [UnorderedKey] The FIRST_SUPERUSER_PASSWORD key should go before the SECRET_KEY key
[warning] 6-6: [QuoteCharacter] The value has quote characters (', ")
[warning] 6-6: [UnorderedKey] The EMAIL_TEST_USER key should go before the FIRST_SUPERUSER key
[warning] 11-11: [UnorderedKey] The POSTGRES_PORT key should go before the POSTGRES_SERVER key
[warning] 12-12: [UnorderedKey] The POSTGRES_DB key should go before the POSTGRES_PORT key
[warning] 14-14: [UnorderedKey] The POSTGRES_PASSWORD key should go before the POSTGRES_PORT key
[warning] 20-20: [UnorderedKey] The AWS_DEFAULT_REGION key should go before the AWS_SECRET_ACCESS_KEY key
[warning] 21-21: [SpaceCharacter] The line has spaces around equal sign
[warning] 21-21: [UnorderedKey] The AWS_S3_BUCKET_PREFIX key should go before the AWS_SECRET_ACCESS_KEY key
[warning] 25-25: [QuoteCharacter] The value has quote characters (', ")
🔇 Additional comments (5)
.env.test.example (1)
1-26: Good template structure for test environment configuration.The file provides a comprehensive template covering all necessary environment variables for testing including database, AWS, and API configurations.
backend/app/tests/utils/utils.py (1)
126-128: Inconsistent error handling approach.The function now raises a
FileNotFoundErrorwhen the .env.test file is missing, which differs from the warning approach suggested in past reviews. This makes the function more strict but may not align with the original intent of graceful fallback.Please clarify the intended behavior:
- Should missing .env.test file cause tests to fail (current implementation)?
- Or should it warn and continue with default settings (past review suggestion)?
The current approach ensures test environment isolation but requires .env.test to always exist.
backend/app/tests/conftest.py (3)
19-22: Good session-scoped fixture for test environment loading.The
load_test_envfixture properly loads the test environment at session scope before any tests run, ensuring consistent environment setup. The relative path../.env.testcorrectly references the expected location.
24-30: Excellent centralization of database connection management.The
engine_connectionfixture provides a clean way to share database engine and connection across test fixtures, improving resource reuse and test performance.
32-48: Good refactoring to use shared database connection.The updated
dbfixture properly utilizes the shared connection fromengine_connection, maintaining transaction isolation while improving resource efficiency.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
backend/app/tests/utils/utils.py (2)
99-99: Replace hardcoded absolute path with relative path.The hardcoded absolute path
/Users/nishikayadav/Desktop/platform/.env.testwill fail for other developers and CI environments.Apply this fix:
- env_test_path: str = "./Users/nishikayadav/Desktop/platform/.env.test", + env_test_path: str = "../.env.test",
108-108: Potential issue with settings reinitialization.The
settings.__init__()call may not properly reinitialize the settings object, as it depends on the internal implementation of the settings class. This was previously flagged and the concern remains valid.Consider verifying that
settings.__init__()properly reloads all environment variables for your specific settings implementation, or use an alternative approach to force settings reload.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/tests/utils/utils.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/app/tests/utils/utils.py (1)
backend/app/models/assistants.py (1)
Assistant(29-40)
🪛 Ruff (0.12.2)
backend/app/tests/utils/utils.py
132-132: Blank line contains whitespace
(W293)
133-133: SyntaxError: unindent does not match any outer indentation level
🪛 GitHub Actions: AI Platform CI
backend/app/tests/utils/utils.py
[error] 133-133: Black formatting failed: unindent does not match any outer indentation level (tokenize error at line 133).
[error] Pre-commit hook 'trailing-whitespace' failed and modified the file to fix trailing whitespace issues.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/app/tests/utils/utils.py (1)
102-139: Address previous review feedback and align with PR objectives.The current implementation contradicts the PR objectives and ignores previous review feedback. The function should provide graceful fallback when
.env.testis missing, not raise an exception that will cause tests to fail.Based on the past review comments and PR objectives, apply these improvements:
+import logging + +logger = logging.getLogger(__name__) + def load_environment(env_test_path: str = "../.env.test"): """Loads the test environment variables if the .env.test file exists. - Raises an error if any required PostgreSQL credentials are missing or if the - POSTGRES_DB value does not contain the word 'test', to ensure a safe test database is used. + Falls back to default environment if .env.test is missing. + Validates PostgreSQL credentials and ensures test database usage when .env.test exists. """ if os.path.exists(env_test_path): load_dotenv(dotenv_path=env_test_path, override=True) - settings.__init__() + # Force reload of settings - verify this works with your settings implementation + settings.__init__() required_vars = [ "POSTGRES_USER", "POSTGRES_PASSWORD", "POSTGRES_SERVER", "POSTGRES_PORT", "POSTGRES_DB", ] missing_vars = [var for var in required_vars if not os.getenv(var)] if missing_vars: raise ValueError( f"Missing the following PostgreSQL credentials in {env_test_path}: {', '.join(missing_vars)}" ) db_name = os.getenv("POSTGRES_DB", "").lower() if "test" not in db_name: raise RuntimeError( f"Connected to database '{db_name}', which doesn't appear to be a test database" ) else: - raise FileNotFoundError( - f"{env_test_path} not found. No environment variables loaded." - ) + logger.warning( + f"{env_test_path} not found. Using default environment settings." + ) return settings
🧹 Nitpick comments (3)
backend/app/tests/utils/utils.py (1)
98-99: Remove unused imports identified by static analysis.The
make_urlandcreate_engineimports from SQLAlchemy are not used in this file and should be removed to clean up the code.-from sqlalchemy.engine.url import make_url -from sqlalchemy import create_enginebackend/app/tests/conftest.py (2)
4-4: Remove unused import.The
create_engineimport from sqlmodel is not used in this file.-from sqlmodel import Session, create_engine +from sqlmodel import Session
13-23: Reorganize imports to follow PEP 8 while maintaining fixture functionality.The current import organization violates PEP 8 by having module-level imports scattered throughout the file. However, the
load_test_envfixture needs theload_environmentfunction imported.Reorganize the imports to the top of the file:
from collections.abc import Generator import pytest from fastapi.testclient import TestClient from sqlmodel import Session from sqlalchemy import event + +from app.core.config import settings +from app.core.db import engine +from app.api.deps import get_db +from app.main import app +from app.models import APIKeyPublic +from app.seed_data.seed_data import seed_database +from app.tests.utils.user import authentication_token_from_email +from app.tests.utils.utils import ( + get_superuser_token_headers, + get_api_key_by_email, + load_environment, +) @pytest.fixture(scope="session", autouse=True) def load_test_env(): load_environment("../.env.test") -from app.api.deps import get_db -from app.main import app -from app.models import APIKeyPublic -from app.tests.utils.user import authentication_token_from_email - - -@pytest.fixture(scope="session", autouse=True) -def load_test_env(): - load_environment("../.env.test") - - -from app.core.config import settings -from app.tests.utils.utils import ( - get_superuser_token_headers, - get_api_key_by_email, - load_environment, -) -from app.seed_data.seed_data import seed_database -from app.core.db import engine - - @pytest.fixture(scope="session", autouse=True)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/app/tests/conftest.py(1 hunks)backend/app/tests/utils/utils.py(2 hunks)backend/app/tests_pre_start.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/app/tests_pre_start.py (3)
backend/app/tests/utils/utils.py (1)
load_environment(102-139)backend/app/backend_pre_start.py (1)
main(32-35)backend/app/tests/scripts/test_test_pre_start.py (1)
test_init_successful_connection(8-33)
🪛 GitHub Actions: AI Platform CI
backend/app/tests_pre_start.py
[error] 9-9: RuntimeError: Connected to database 'ai_platform', which doesn't appear to be a test database
🪛 Ruff (0.12.2)
backend/app/tests/conftest.py
4-4: sqlmodel.create_engine imported but unused
Remove unused import: sqlmodel.create_engine
(F401)
18-18: Module level import not at top of file
(E402)
19-23: Module level import not at top of file
(E402)
24-24: Module level import not at top of file
(E402)
25-25: Module level import not at top of file
(E402)
backend/app/tests/utils/utils.py
98-98: Module level import not at top of file
(E402)
98-98: sqlalchemy.engine.url.make_url imported but unused
Remove unused import: sqlalchemy.engine.url.make_url
(F401)
99-99: Module level import not at top of file
(E402)
99-99: sqlalchemy.create_engine imported but unused
Remove unused import: sqlalchemy.create_engine
(F401)
🔇 Additional comments (2)
backend/app/tests_pre_start.py (1)
39-41: Useful debugging addition for database URL logging.The database URL logging with password masking is a helpful addition for debugging connection issues during test setup.
backend/app/tests/conftest.py (1)
28-33: Well-designed session-scoped fixture for baseline data seeding.The session-scoped autouse fixture for seeding baseline data is an excellent design choice that will improve test performance by avoiding repeated seeding operations.
The print statement provides useful feedback during test setup, and the session scope ensures the seeding happens once per test session.
backend/app/tests_pre_start.py
Outdated
| from app.tests.utils.utils import load_environment | ||
|
|
||
| load_environment("../.env.test") |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Good placement of environment loading, but pipeline failure needs attention.
The placement of load_environment before importing the database engine is correct and ensures test environment variables are loaded first.
However, the pipeline failure indicates the database validation is working correctly - it detected that 'ai_platform' is not a test database. Ensure the CI environment has:
- A proper
.env.testfile withPOSTGRES_DBcontaining "test" (e.g., "ai_platform_test") - The CI workflow correctly copies
.env.test.exampleto.env.testwith appropriate test database configuration
🏁 Script executed:
#!/bin/bash
# Verify CI workflow configuration for .env.test setup
fd -t f "continuous_integration.yml" --exec cat {} | grep -A 10 -B 5 "\.env\.test"Length of output: 84
I’ll search the workflow directory and the repo for any references to “.env.test” to confirm CI setup.
#!/bin/bash
# List GitHub Actions workflow files
echo "== Workflow files =="
fd -e yml -d 2 .github/workflows || echo "(no workflows directory)"
# Search entire repo for .env.test references
echo "== References to .env.test =="
rg -n "\.env\.test" || echo "(none found)"CI setup missing .env.test configuration
I searched the repository for any references to .env.test in CI workflows and found none. Without a step to copy or define .env.test, the call to load_environment("../.env.test") will fail in CI, triggering the “non-test database” runtime error.
Please update your CI configuration to:
- Include a
.env.testfile (e.g., copy from.env.test.example). - Add a workflow step that ensures
.env.testis available before tests run.
Example for GitHub Actions (.github/workflows/ci.yml):
jobs:
test:
runs-on: ubuntu-latest
steps:
# …checkout and setup steps…
- name: Copy test environment file
run: cp .env.test.example .env.test
- name: Run pre-start script
run: python backend/app/tests_pre_start.py
- name: Run tests
run: pytestOnce .env.test is correctly configured in CI, the pipeline should load the test database (POSTGRES_DB containing “test”) and pass validation.
🧰 Tools
🪛 GitHub Actions: AI Platform CI
[error] 9-9: RuntimeError: Connected to database 'ai_platform', which doesn't appear to be a test database
🤖 Prompt for AI Agents
In backend/app/tests_pre_start.py around lines 7 to 9, the code loads the
.env.test file, but the CI workflow does not ensure this file exists, causing
failures. Update the CI configuration (e.g., .github/workflows/ci.yml) to
include a step that copies .env.test.example to .env.test before running tests.
This ensures the environment file is present for
load_environment("../.env.test") and prevents runtime errors related to missing
test database configuration.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.env.test.example (2)
10-11: Remove unnecessary quotes for values without spaces
EMAIL_TEST_USERdoes not contain spaces, so the surrounding double-quotes are superfluous and triggerdotenv-linter’s QuoteCharacter warning.
Keeping quoting only where it’s needed (e.g.,PROJECT_NAME) improves consistency and silences the linter.-EMAIL_TEST_USER="test@example.com" +EMAIL_TEST_USER=test@example.com
25-25: Drop quotes fromAWS_S3_BUCKET_PREFIXfor linter complianceThe value has no spaces; removing the quotes avoids the QuoteCharacter warning while leaving the semantics unchanged.
-AWS_S3_BUCKET_PREFIX="bucket-prefix-name" +AWS_S3_BUCKET_PREFIX=bucket-prefix-name
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.env.test.example(1 hunks).github/workflows/benchmark.yml(0 hunks)backend/app/core/config.py(0 hunks)docker-compose.yml(0 hunks)
💤 Files with no reviewable changes (3)
- backend/app/core/config.py
- docker-compose.yml
- .github/workflows/benchmark.yml
🧰 Additional context used
🪛 dotenv-linter (3.3.0)
.env.test.example
[warning] 8-8: [UnorderedKey] The FIRST_SUPERUSER key should go before the SECRET_KEY key
[warning] 9-9: [UnorderedKey] The FIRST_SUPERUSER_PASSWORD key should go before the SECRET_KEY key
[warning] 10-10: [QuoteCharacter] The value has quote characters (', ")
[warning] 10-10: [UnorderedKey] The EMAIL_TEST_USER key should go before the FIRST_SUPERUSER key
[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
[warning] 24-24: [UnorderedKey] The AWS_DEFAULT_REGION key should go before the AWS_SECRET_ACCESS_KEY key
[warning] 25-25: [QuoteCharacter] The value has quote characters (', ")
[warning] 25-25: [UnorderedKey] The AWS_S3_BUCKET_PREFIX key should go before the AWS_SECRET_ACCESS_KEY key
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
🔇 Additional comments (1)
.env.test.example (1)
22-24: Clarify placeholder AWS credentials or mock providerEmpty
AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEYoften cause SDKs to raise credential-related exceptions.
If tests rely on real AWS clients, add dummy placeholders (e.g.,test) or switch the code to use a local mock such as LocalStack or moto whenENVIRONMENT=local.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
backend/app/tests/utils/utils.py (3)
97-97: Add default parameter value for env_test_path.The function parameter should have a default value as suggested in previous reviews to make it more convenient to use.
-def load_environment(env_test_path: str): +def load_environment(env_test_path: str = "../.env.test"):
106-106: Settings reinitialization may not work properly.The
settings.__init__()approach has been flagged in previous reviews as potentially unreliable for reloading environment variables, as it depends on the internal implementation of the settings class.Consider using a more robust approach:
- settings.__init__() + # Force reload of settings by reimporting the module + from app.core.config import settings as _settings + global settings + settings = _settingsAlternatively, verify that
settings.__init__()properly reloads all environment variables for your specific settings implementation.
129-132: Replace FileNotFoundError with warning to align with PR objectives.The current implementation raises a
FileNotFoundErrorwhen the.env.testfile is missing, but the PR objectives state the goal is "graceful fallback to default environment settings." Raising an exception contradicts this objective and will cause tests to fail when the file is missing.+import logging + +logger = logging.getLogger(__name__) + else: - raise FileNotFoundError( - f"{env_test_path} not found. No environment variables loaded." - ) + logger.warning( + f"{env_test_path} not found. Using default environment settings." + )This approach aligns with the PR objective of enabling graceful fallback when the test environment file is not available.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.env.example(0 hunks).github/workflows/continuous_integration.yml(2 hunks)backend/app/tests/api/routes/documents/test_route_document_upload.py(2 hunks)backend/app/tests/utils/utils.py(2 hunks)
💤 Files with no reviewable changes (1)
- .env.example
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/continuous_integration.yml
- backend/app/tests/api/routes/documents/test_route_document_upload.py
🧰 Additional context used
🪛 Ruff (0.12.2)
backend/app/tests/utils/utils.py
94-94: Module level import not at top of file
(E402)
94-94: Redefinition of unused load_dotenv from line 6
Remove definition: load_dotenv
(F811)
94-94: dotenv.find_dotenv imported but unused
Remove unused import: dotenv.find_dotenv
(F401)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/app/tests/utils/utils.py (1)
98-134: Improve environment loading implementation with better error handling.The
load_environmentfunction has several areas for improvement:
Settings reinitialization issue: The function doesn't actually reload the settings object after loading new environment variables. The
settingsobject is imported at module level and won't reflect the new environment variables.Security validation: Good implementation of database name validation to prevent accidental production database usage.
Error handling: Proper validation of required PostgreSQL variables.
Apply this improved implementation:
def load_environment(env_test_path: str = "../.env.test"): """Loads the test environment variables if the .env.test file exists. Raises an error if any required PostgreSQL credentials are missing or if the POSTGRES_DB value does not contain the word 'test', to ensure a safe test database is used. """ if os.path.exists(env_test_path): load_dotenv(env_test_path, override=True) required_vars = [ "POSTGRES_USER", "POSTGRES_PASSWORD", "POSTGRES_SERVER", "POSTGRES_PORT", "POSTGRES_DB", ] missing_vars = [var for var in required_vars if not os.getenv(var)] if missing_vars: raise ValueError( f"Missing the following PostgreSQL credentials in {env_test_path}: {', '.join(missing_vars)}" ) db_name = os.getenv("POSTGRES_DB", "").lower() if "test" not in db_name: raise RuntimeError( f"Connected to database '{db_name}', which doesn't appear to be a test database" ) + # Force reload of settings to pick up new environment variables + from importlib import reload + import app.core.config + reload(app.core.config) + from app.core.config import settings as reloaded_settings + return reloaded_settings else: logger.warning( f"{env_test_path} not found. Using default environment settings." ) - - return settings + return settingsThis addresses the settings reinitialization issue by properly reloading the config module, ensuring the new environment variables are reflected in the settings object.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/tests/utils/utils.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
backend/app/tests/utils/utils.py
6-6: typing.Type is deprecated, use type instead
(UP035)
13-13: Redefinition of unused load_dotenv from line 8
Remove definition: load_dotenv
(F811)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
There was a problem hiding this comment.
@AkhileshNegi @nishika26
I'm still not fully clear on how env.test is overriding or changing the value of settings in config.py
| @pytest.fixture(scope="session", autouse=True) | ||
| def seed_baseline(): | ||
| # Load test environment to ensure correct bucket name | ||
| path = find_dotenv(".env.test") |
There was a problem hiding this comment.
This will use .env file, if there is no .env.test available.
| ) | ||
|
|
||
| db_name = os.getenv("POSTGRES_DB", "").lower() | ||
| if "test" not in db_name: |
There was a problem hiding this comment.
there is no need to verify that test should be in db_name, if someone is using .env.test they know which db they want to use.
| def seed_baseline(): | ||
| # Load test environment to ensure correct bucket name | ||
| path = find_dotenv(".env.test") | ||
| load_environment(path) |
There was a problem hiding this comment.
@AkhileshNegi I have modified seed_baseline function with
And this is my output
app/tests/api/routes/test_api_key.py Loading environment variables for testing...
postgresql+psycopg://postgres:postgres@localhost:5432/aipostgres
postgresql+psycopg://postgres:postgres@localhost:5432/aipostgres
After loading also settings is having old reference of env variables. db should be aipostgres_test.
Summary
Target issue is #204
Explain the motivation for making this change. What existing problem does the pull request solve?
WIP
Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.Notes
This PR introduces the creation of a .env.test file for testing configurations, along with a utility function that loads the test environment variables from this file if it exists. It also adds an .env.test.example file as a reference for developers to easily create their own .env.test files. These changes ensure that testing can be done in an isolated environment without affecting the production settings.
Changes:
Created .env.test File:
Added .env.test.example File:
Utility Function to Load Test Environment Variables:
Created the load_environment function that:
Loads the environment variables from .env.test if the file exists.
If the .env.test file is missing, it prints a warning and uses the default environment settings.
After overriding the .env with .env.test, we call settings.init() to reinitialize the settings object, ensuring that it picks up the new environment variables defined in the test configuration
Updated conftest.py:
Summary by CodeRabbit
New Features
Bug Fixes
Chores
.gitignoreto exclude all.env*files.