-
Notifications
You must be signed in to change notification settings - Fork 0
test: añadir tests para security, telemetry y logging #113
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
Conversation
- test_security.py: 14 tests para security.py (0% -> 100%) - test_telemetry.py: 5 tests para telemetry.py (0% -> 81%) - test_logging.py: 8 tests para logging.py (32% -> 100%) - Cobertura total: 72% -> 96% - 34 tests passing
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.
Pull request overview
This PR adds comprehensive test coverage for three core utility modules (security, telemetry, and logging), increasing overall test coverage from 72% to 96%. The tests are well-organized using class-based groupings and cover both happy paths and edge cases.
- Added 14 tests for security.py achieving 100% coverage
- Added 5 tests for telemetry.py achieving 81% coverage
- Added 8 tests for logging.py improving coverage from 32% to 100%
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| app/tests/test_telemetry.py | Tests for telemetry module covering setup_telemetry and log_request_metrics functions with various scenarios |
| app/tests/test_security.py | Tests for security module covering configuration validation, key generation, and health checks |
| app/tests/test_logging.py | Tests for logging module covering logger setup with different log levels and the get_logger function |
Comments suppressed due to low confidence (1)
app/tests/test_telemetry.py:4
- Import of 'MagicMock' is not used.
Import of 'patch' is not used.
from unittest.mock import MagicMock, patch
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """Tests for telemetry module.""" | ||
|
|
||
| import logging | ||
| from unittest.mock import MagicMock, patch |
Copilot
AI
Dec 13, 2025
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.
The MagicMock and patch imports from unittest.mock are not used anywhere in this test file. These unused imports should be removed to keep the code clean.
| from unittest.mock import MagicMock, patch |
| } | ||
| with patch.dict(os.environ, env, clear=False): | ||
| result = validate_production_config() | ||
| assert any("16 characters" in w for w in result["warnings"]) |
Copilot
AI
Dec 13, 2025
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.
The assertion message checks for "16 characters" in the warning, but according to the security.py source code (line 47), the actual warning message says "at least 16 characters long". The test should check for the complete or more specific phrase to ensure it matches the actual warning message exactly.
| assert any("16 characters" in w for w in result["warnings"]) | |
| assert any("at least 16 characters long" in w for w in result["warnings"]) |
| status_code=500, | ||
| duration_ms=100.0, | ||
| ) | ||
| assert "500" in caplog.text or "GET" in caplog.text |
Copilot
AI
Dec 13, 2025
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.
The assertion on line 69 uses an OR condition that will almost always pass. Since the test is checking for an error status code (500), the assertion should verify that "500" appears in the log output, not fall back to checking for "GET" which would be present in all GET requests. This makes the test less effective at catching issues with error status code logging.
| assert "500" in caplog.text or "GET" in caplog.text | |
| assert "500" in caplog.text |
| env = {"SECRET_KEY": "test-secret", "API_KEY": ""} | ||
| with patch.dict(os.environ, env, clear=False): | ||
| os.environ.pop("API_KEY", None) |
Copilot
AI
Dec 13, 2025
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.
The test patches the environment with a SECRET_KEY and API_KEY but then immediately removes API_KEY with os.environ.pop(). This creates confusion about the test's intent. The environment should either be set up correctly from the start (without API_KEY) or the patch.dict should not include it. The current approach unnecessarily patches then removes the value.
| env = {"SECRET_KEY": "test-secret", "API_KEY": ""} | |
| with patch.dict(os.environ, env, clear=False): | |
| os.environ.pop("API_KEY", None) | |
| env = {"SECRET_KEY": "test-secret"} | |
| with patch.dict(os.environ, env, clear=False): |
| env = {"CORS_ORIGINS": "*", "API_KEY": "", "SECRET_KEY": ""} | ||
| with patch.dict(os.environ, env, clear=False): | ||
| os.environ.pop("API_KEY", None) | ||
| os.environ.pop("SECRET_KEY", None) |
Copilot
AI
Dec 13, 2025
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.
Similar to the test_missing_api_key test, this test patches environment variables but then immediately removes them with os.environ.pop(). This is unnecessarily complex. The environment variables should simply not be included in the patch.dict in the first place if the goal is to test their absence.
| # Verify only one handler exists | ||
| assert len(root.handlers) == 1 | ||
|
|
||
|
|
Copilot
AI
Dec 13, 2025
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.
The test_setup_logging_clears_existing_handlers test has a flaw: it adds a dummy handler but then only checks that exactly one handler exists after setup_logging is called. This doesn't actually verify that the dummy handler was cleared - it only verifies the final count. The test would pass even if setup_logging never cleared handlers as long as it only added one handler. A more robust test would verify the handler identity or check that the dummy handler is no longer present.
| # Verify only one handler exists | |
| assert len(root.handlers) == 1 | |
| # Verify only one handler exists and dummy_handler is not present | |
| assert len(root.handlers) == 1 | |
| assert dummy_handler not in root.handlers |
| import os | ||
| from unittest.mock import patch | ||
|
|
||
| import pytest |
Copilot
AI
Dec 13, 2025
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.
Import of 'pytest' is not used.
| import pytest |
| import os | ||
| from unittest.mock import patch | ||
|
|
||
| import pytest |
Copilot
AI
Dec 13, 2025
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.
Import of 'pytest' is not used.
| import pytest |
| import logging | ||
| from unittest.mock import MagicMock, patch | ||
|
|
||
| import pytest |
Copilot
AI
Dec 13, 2025
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.
Import of 'pytest' is not used.
| import pytest |
🚀 Pull Request: Complete Railway Deployment Optimization
📋 Descripción del Cambio
Este PR implementa la solución completa para el problema de crashes de Railway después de 2 minutos, junto con la funcionalidad completa del dashboard administrativo para el sistema bancario NeuroBank FastAPI.
🎯 Problema Solucionado
✅ Solución Implementada
🔧 Cambios Técnicos Implementados
🚂 Railway Deployment
railway.json] Configuración con health checks y restart policiesstart.sh] Script de inicio inteligente con validacionesDockerfile] Optimización single worker + uvloop📊 Admin Dashboard
admin_transactions.html] Panel transacciones completo con Chart.jsadmin_users.html] Gestión usuarios con búsqueda en tiempo realadmin_reports.html] Reportes avanzados con exportación CSV/Excelrouter.py] Conexiones específicas (no más templates genéricos)🔄 CI/CD Pipeline
.github/workflows/production-pipeline.yml] Pipeline de 8 etapas📚 Documentation Suite
HOTFIX_RAILWAY_CRASH.md] Análisis técnico del problema RailwayWORKFLOW.md] Procedimientos de desarrolloGIT_COMMANDS_HOTFIX.md] Comandos de despliegue🧪 Testing & Validation
✅ Funcionalidad Validada
/healthoperativo🔒 Security Checks
⚡ Performance Tests
🎯 Business Impact
🚀 Deployment Instructions
Pre-merge Checklist
RAILWAY_TOKENconfigurado en GitHub SecretsPost-merge Actions
main👥 Review Requirements
🔍 Areas de Focus para Review
railway.jsonystart.sh🎯 Expected Reviewers
📝 Additional Notes
🔄 Future Improvements
📚 Related Documentation
✅ Ready to Merge Criteria
🎉 Este PR convierte NeuroBank FastAPI en una aplicación bancaria de nivel empresarial con despliegue automático y funcionalidad completa!