Conversation
- fix tests - remove unwanted logger call
- improve tests - rename file name to avoid confusion
There was a problem hiding this comment.
Pull Request Overview
This PR updates the application to use a configurable CSV file name instead of a hardcoded filename, making it easier to update data files without code changes. The changes introduce environment variable configuration for the CSV file path along with proper logging setup.
- Replaces hardcoded CSV filename with environment variable
CSV_SPONSORSHIP - Adds centralized logging configuration that must be initialized before other modules
- Updates test setup to properly mock the new environment-dependent CSV loading
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| app/services/search.py | Replaces hardcoded CSV filename with environment variable configuration |
| app/main.py | Adds logging setup and reorders imports to ensure logging is configured first |
| app/logging_config.py | New module providing centralized logging configuration |
| tests/services/test_search.py | Adds test fixture to mock environment variables and CSV loading |
| .env.example | Documents the new CSV_SPONSORSHIP environment variable |
| mock_read_csv.return_value = pd.DataFrame({"Organisation Name": ["dummy"]}) | ||
|
|
||
| # Import after setting up the environment | ||
| global search_companies |
There was a problem hiding this comment.
Using global variables in tests is generally discouraged as it can lead to test coupling and unexpected side effects. Consider importing the function locally within each test that needs it, or use a proper fixture to provide the function.
| global search_companies |
| global search_companies | ||
| from app.services.search import search_companies |
There was a problem hiding this comment.
Importing modules inside a fixture can cause issues with module caching and make tests harder to understand. Consider restructuring to import the module at the test level or use importlib to properly handle the dynamic import with environment setup.
| global search_companies | |
| from app.services.search import search_companies | |
| pass # Removed dynamic import; import will be handled at the test level. |
| XAI_MODEL_L=grok-3-latest | ||
| XAI_MODEL_XL=grok-4-0709 | ||
|
|
||
| # CSV_FOR_SPONSORSHIP |
There was a problem hiding this comment.
The comment 'CSV_FOR_SPONSORSHIP' doesn't match the actual environment variable name 'CSV_SPONSORSHIP'. Consider updating the comment to match: '# CSV_SPONSORSHIP'
| # CSV_FOR_SPONSORSHIP | |
| # CSV_SPONSORSHIP |
No description provided.