-
Notifications
You must be signed in to change notification settings - Fork 0
test: añadir tests para security, telemetry y logging #112
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
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,78 @@ | ||||||||||||
| """Tests for logging module.""" | ||||||||||||
|
|
||||||||||||
| import logging | ||||||||||||
| import os | ||||||||||||
| from unittest.mock import patch | ||||||||||||
|
|
||||||||||||
| import pytest | ||||||||||||
|
|
||||||||||||
| from app.utils.logging import get_logger, setup_logging | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| class TestSetupLogging: | ||||||||||||
| """Tests for setup_logging function.""" | ||||||||||||
|
|
||||||||||||
| def test_setup_logging_returns_logger(self): | ||||||||||||
| """Test that setup_logging returns a logger instance.""" | ||||||||||||
| logger = setup_logging() | ||||||||||||
| assert isinstance(logger, logging.Logger) | ||||||||||||
|
|
||||||||||||
| def test_setup_logging_default_level(self): | ||||||||||||
| """Test setup_logging with default INFO level.""" | ||||||||||||
| with patch.dict(os.environ, {}, clear=False): | ||||||||||||
| os.environ.pop("LOG_LEVEL", None) | ||||||||||||
|
Comment on lines
+22
to
+23
|
||||||||||||
| with patch.dict(os.environ, {}, clear=False): | |
| os.environ.pop("LOG_LEVEL", None) | |
| with patch.dict(os.environ, {}, clear=True): |
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.
This test may not reliably verify handler cleanup behavior. After calling setup_logging(), the test asserts exactly 1 handler exists, but this doesn't necessarily prove that the dummy_handler was removed - it could be that setup_logging() removes all handlers and adds exactly 1 new one, or it could keep the dummy handler and not add a new one. The test should verify the handler identity or type to ensure proper cleanup occurred.
| # Verify only one handler exists | |
| assert len(root.handlers) == 1 | |
| # Verify only one handler exists and it's not the dummy handler | |
| assert len(root.handlers) == 1 | |
| assert dummy_handler not in root.handlers |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,140 @@ | ||||||||||||||||||||||
| """Tests for security module.""" | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| import os | ||||||||||||||||||||||
| from unittest.mock import patch | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| import pytest | ||||||||||||||||||||||
|
||||||||||||||||||||||
| 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.
The test may not properly validate missing API_KEY behavior due to conftest.py setting default test environment variables. Even after popping API_KEY from os.environ, the patch.dict context only patches with the provided env dict and doesn't guarantee that API_KEY remains unset. The test should use clear=True in patch.dict to ensure complete isolation from conftest.py's default environment variables.
| 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=True): |
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.
This test has potential issues with test isolation. The test first sets "API_KEY" to an empty string in the patched environment, then immediately pops it. This two-step approach is confusing and could lead to flaky tests. The test should either use clear=True in patch.dict or only use pop without setting the empty string first.
| 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=True): |
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 API_KEY test, this test has confusing environment manipulation. Setting environment variables to empty strings and then immediately popping them creates unnecessary complexity. Use a cleaner approach with clear=True in patch.dict or only use the pop operation.
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 test_missing_api_key, this test may not work correctly due to conftest.py setting default environment variables. The patch.dict with clear=False means the test environment from conftest.py may still interfere. Consider using clear=True to ensure the test properly validates the unhealthy status when API_KEY and SECRET_KEY are truly missing.
| with patch.dict(os.environ, env, clear=False): | |
| os.environ.pop("API_KEY", None) | |
| os.environ.pop("SECRET_KEY", None) | |
| with patch.dict(os.environ, env, clear=True): |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,69 @@ | ||||||
| """Tests for telemetry module.""" | ||||||
|
|
||||||
| import logging | ||||||
| from unittest.mock import MagicMock, patch | ||||||
|
||||||
| 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.
Import of 'pytest' is not used.
| 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.
The assertions for this test are too weak. The test should verify that the status_code (200) and duration_ms (15.5) are also present in the logged output to ensure all parameters are being logged correctly. Currently, only the method and endpoint are checked.
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.
This test doesn't actually verify that the request_id is included in the logged output. The test should assert that "req-123-abc" appears in the captured log text to properly validate that the request_id parameter is being logged correctly.
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 here is too weak and doesn't verify the actual logged message. The test should verify that both the status code "500" AND the expected log information are present in the log output. The current assertion with "or" will pass even if only "GET" is found, which doesn't properly validate error logging behavior.
| assert "500" in caplog.text or "GET" in caplog.text | |
| assert "500" in caplog.text and "GET" in caplog.text |
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.