test: add unit tests for services/utils/logging_config.py (#40)#90
test: add unit tests for services/utils/logging_config.py (#40)#90Aharshi3614 wants to merge 4 commits into
Conversation
|
@Aharshi3614 is attempting to deploy a commit to the s3dfx-cyber's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Warning Review limit reached
More reviews will be available in 9 minutes and 21 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new unit test module ChangesLogging Configuration Unit Tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/unit/test_logging_config.py (1)
11-11: ⚡ Quick winConsider using a more robust import mechanism.
The
sys.path.insertapproach is fragile and depends on the test file's location relative to the services directory. If tests are run from different working directories or the project structure changes, imports may fail.Consider one of these approaches:
- Make
servicesa proper Python package with__init__.pyfiles and install it in development mode (pip install -e .)- Use pytest's
pythonpathconfiguration inpyproject.tomlorpytest.ini📦 Example pytest configuration approach
In
pyproject.tomlorpytest.ini:[tool.pytest.ini_options] pythonpath = ["services"]Then simplify the import:
-sys.path.insert(0, str(Path(__file__).parent.parent.parent / "services" / "utils")) - -from logging_config import setup_logging +from utils.logging_config import setup_logging🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_logging_config.py` at line 11, The test currently manipulates import paths via sys.path.insert with Path(__file__).parent... which is fragile; instead make the services tree a proper package (add __init__.py files and install in dev mode with pip install -e .) or configure pytest to add services to PYTHONPATH (add pythonpath = ["services"] under [tool.pytest.ini_options] in pyproject.toml or pytest.ini) and then remove the sys.path.insert line and switch to normal imports (e.g., import from services.utils or the specific module names used in the test).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/test_logging_config.py`:
- Around line 110-115: The test test_logger_name_not_sensitive is ineffective
because it passes a safe name; either implement name sanitization in
setup_logging or change the test to match the documented behavior — update
setup_logging to call a new helper sanitize_logger_name(name) that detects
sensitive substrings like "password", "secret", "token" and masks or strips them
before creating the logger, then change test_logger_name_not_sensitive to pass a
name containing those substrings (e.g., "service_password_token") and assert the
returned logger.name does not contain the raw sensitive words and contains the
masked form; alternatively, if you choose not to change implementation, replace
this unit test with one asserting callers must not pass sensitive data and/or
move coverage to the log filtering/formatting tests.
- Around line 94-104: The test_log_format_contains_required_fields currently may
pass silently if no handler has a formatter; update the test that calls
setup_logging("test_format") to first assert that at least one handler in
logger.handlers has a formatter (e.g. assert any(h.formatter for h in
logger.handlers)), then iterate over all handlers and assert each handler has a
formatter and that handler.formatter._fmt contains "%(asctime)s", "%(name)s",
"%(levelname)s", and "%(message)s"; reference the existing test function name
test_log_format_contains_required_fields and the setup_logging call to locate
where to add these checks.
- Around line 29-33: The test test_default_log_level_is_info mutates os.environ
directly; change it to use unittest.mock.patch.dict to ensure environment
isolation for LOG_LEVEL while calling setup_logging("test_default_level") so the
original environment is restored after the test. Replace the
os.environ.pop("LOG_LEVEL", None) line with a patch.dict context (or decorator)
that sets ENV without LOG_LEVEL (or sets LOG_LEVEL to None/absent) so
logger.level can be asserted as logging.INFO without polluting other tests.
- Around line 117-125: The test function
test_logger_functional_for_normal_messages has a critical indentation mistake:
the body (calls to setup_logging, logger.info/warning/error and the try/except)
must be indented under the def to form a valid function block; fix by indenting
lines inside test_logger_functional_for_normal_messages so the logger =
setup_logging(...), try/except, and pytest.fail calls are all nested within the
function body.
---
Nitpick comments:
In `@tests/unit/test_logging_config.py`:
- Line 11: The test currently manipulates import paths via sys.path.insert with
Path(__file__).parent... which is fragile; instead make the services tree a
proper package (add __init__.py files and install in dev mode with pip install
-e .) or configure pytest to add services to PYTHONPATH (add pythonpath =
["services"] under [tool.pytest.ini_options] in pyproject.toml or pytest.ini)
and then remove the sys.path.insert line and switch to normal imports (e.g.,
import from services.utils or the specific module names used in the test).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cff25995-dea6-4d21-bb0e-bbc0341eb7ee
📒 Files selected for processing (1)
tests/unit/test_logging_config.py
|
@Aharshi3614 needs fixes , syntax issues |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/test_logging_config.py (1)
121-129: 💤 Low valueRedundant try/except around logging calls.
If any
logger.*call raised, pytest would already fail the test, so thetry/except+pytest.failwrapper adds no value and triggers Ruff BLE001 (blindExceptioncatch). Letting the exception propagate is simpler and preserves the original traceback.♻️ Proposed simplification
def test_logger_functional_for_normal_messages(self): """Test that logger works normally without raising exceptions.""" logger = setup_logging("test_sensitive") - try: - logger.info("User logged in successfully") - logger.warning("Test warning") - logger.error("Test error") - except Exception as e: - pytest.fail(f"Logger raised an exception: {e}") + logger.info("User logged in successfully") + logger.warning("Test warning") + logger.error("Test error")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_logging_config.py` around lines 121 - 129, The test test_logger_functional_for_normal_messages wraps logger calls in a redundant try/except that catches Exception (triggering Ruff BLE001); remove the try/except and pytest.fail wrapper and simply call setup_logging("test_sensitive") and then logger.info/warning/error directly so any exception will naturally fail the test and preserve the original traceback; locate the test function by name and the setup_logging call to make this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/unit/test_logging_config.py`:
- Around line 121-129: The test test_logger_functional_for_normal_messages wraps
logger calls in a redundant try/except that catches Exception (triggering Ruff
BLE001); remove the try/except and pytest.fail wrapper and simply call
setup_logging("test_sensitive") and then logger.info/warning/error directly so
any exception will naturally fail the test and preserve the original traceback;
locate the test function by name and the setup_logging call to make this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 42f1b86b-0a85-431f-9712-d1e03d40f7d8
📒 Files selected for processing (1)
tests/unit/test_logging_config.py
|
I've fixed all the issues flagged by CodeRabbit — resolved the indentation error, environment isolation, and improved the sensitive data test. All 18 tests pass locally. Could you please approve the workflows so CI can run? |
S3DFX-CYBER
left a comment
S3DFX-CYBER
left a comment
There was a problem hiding this comment.
lgtm i will merge it on 1st June dw
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
2 participants
What this does
Adds 18 unit tests for the logging configuration utility module.
Related Issue
Closes #40
Type of Change
How Has This Been Tested?
pytest tests/unit/test_logging_config.py -v
18 passed in 0.11s
Test Coverage
Summary by CodeRabbit