test: add direct unit tests for grafana config normalization and auth…#1186
Conversation
Greptile SummaryThis PR adds 12 unit tests for Confidence Score: 5/5Safe to merge — all test assertions are correct and no production code is changed. Only P2 findings remain: a missing module docstring and a suggestion to split the single URL normalisation test case into parametrized variants. Neither affects test correctness or production behaviour. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[GrafanaAccountConfig input]
A --> B[Normalize URL: strip whitespace and trailing slashes]
B --> C{URL empty?}
C -- Yes --> D[is_configured equals False]
C -- No --> E{Credential present?}
E -- Yes --> F[is_configured equals True]
E -- No --> G{Host is loopback?}
G -- Yes --> H[local_anon equals True, is_configured equals True]
G -- No --> I[local_anon equals False, is_configured equals False]
Reviews (1): Last reviewed commit: "test: add direct unit tests for grafana ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| from __future__ import annotations | ||
|
|
||
| import pytest | ||
|
|
||
| from app.services.grafana.config import GrafanaAccountConfig |
There was a problem hiding this comment.
Every other Grafana test file in tests/services/ opens with a module docstring (e.g. test_grafana_loki.py, test_grafana_mimir.py). Adding one here would be consistent with the project convention and make the file's intent clear.
Context Used: Testing conventions and patterns (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| def test_grafana_config_normalizes_instance_url() -> None: | ||
| """Test that trailing slashes and whitespace are stripped from the instance URL.""" | ||
| config = GrafanaAccountConfig( | ||
| account_id="test-acc", | ||
| instance_url=" https://grafana.example.com/ ", | ||
| read_token="secret", | ||
| ) | ||
| assert config.instance_url == "https://grafana.example.com" |
There was a problem hiding this comment.
The test bundles leading whitespace and a trailing slash into one input, so a regression that only strips slashes (but not whitespace) would still pass. Consider adding parametrized cases that isolate each normalisation step — trailing slash only, whitespace only, multiple consecutive slashes, and a URL that is already clean — consistent with how the other two tests in this file exercise individual conditions.
|
🛸 Aliens watching our repo just upgraded @PrakharJain345's threat level to: do not engage — too competent. 👽
👋 Join us on Discord - OpenSRE : hang out, contribute, or hunt for features and issues. Everyone's welcome. |
2 participants
Fixes #879
Description
Added comprehensive unit tests for
app/services/grafana/config.py.Changes Made
🧪 Test Coverage Includes
URL Normalization
Verifies trailing slashes and whitespace are stripped from
instance_urlLocal Anonymous Auth
Tests security logic allowing
localhost/127.0.0.1without tokens while enforcing tokens for hosted instancesConfig Validation (
is_configured)Ensures config works for both token-based and local setups
Demo / Verification
Verified locally using pytest:
python -m pytest tests/services/test_grafana_config.py -v # Output: 12 passed in 2.11sCode Understanding and AI Usage
Did you use AI assistance?
If yes:
Implementation Approach
Implemented a test suite in
tests/services/test_grafana_config.pyusingpytest.mark.parametrizeto efficiently cover multiple edge cases related to authentication and URL formatting. This ensures strict enforcement of local anonymous auth rules and predictable behavior of the configuration model.Checklist