Test: Add missing coverage for DatabaseManager, API routes, Sentinel ingest and RAG pipeline#174
Test: Add missing coverage for DatabaseManager, API routes, Sentinel ingest and RAG pipeline#174emon22-ts wants to merge 3 commits into
Conversation
…eManager and API routes
|
@parthrohit22 , @ritiksah141 - assigning the review to u both, seems like you are the code owners and do know the codebase well and what value add this brings in, please do touch base with this. |
|
Container scan - Broken CI, the push for the fix has been done to dev which triggers in the next commit, hence go ahead without minding that CI response. |
ritiksah141
left a comment
There was a problem hiding this comment.
I pulled the branch and reviewed it against the current architecture and latest dev. All 72 added tests pass, ruff passes, and the branch has no merge conflicts. The earlier Trivy failure is not part of this review, as requested in the PR discussion.
I found three coverage gaps that should be corrected before approval:
- The score tests reproduce arithmetic locally instead of executing DatabaseManager.get_score(), so regressions in production SQL aggregation and floor handling would not be detected.
- The Sentinel retry tests only assert the final False result and do not prove that three attempts occurred.
- The RAG chunking tests pass a chunk_overlap value but never verify overlap behavior.
Please also update the PR description because Closes #174 refers to this pull request itself rather than a related issue.
| class TestScoreCalculation: | ||
| """Tests for score calculation logic.""" | ||
|
|
||
| def test_score_starts_at_100(self): |
There was a problem hiding this comment.
These tests recreate the scoring arithmetic inside the test instead of calling DatabaseManager.get_score(). A regression in the query aggregation or the production floor logic would still pass. Please mock the connection rows and assert results returned by db.get_score(). Also add coverage for at least one persistence method or narrow the stated CRUD scope, since _make_db is currently unused.
| with patch("requests.post", return_value=mock_response): | ||
| with patch("time.sleep"): | ||
| result = ingest.send([ingest.normalise(RAW_FINDING, "scan-001")]) | ||
| assert result is False |
There was a problem hiding this comment.
This only verifies the final False result. It would still pass if send() stopped retrying after one request. Keep the requests.post mock and assert call_count == 3. The exception path should make the same assertion so the claimed retry behavior is protected.
| long_content = "word " * 300 | ||
| docs = [self._make_doc(long_content)] | ||
| chunks = chunk_documents(docs, chunk_size=100, chunk_overlap=10) | ||
| assert len(chunks) > 1 |
There was a problem hiding this comment.
The PR says chunk overlap is covered, but this assertion only proves that a long document creates multiple chunks. Please assert that adjacent chunk contents contain the requested overlap, or adjust the stated coverage scope.
m-khan-97
left a comment
There was a problem hiding this comment.
Went through this against the actual source for the three areas Ritik flagged, since I wanted to confirm they're real before piling on rather than just trusting the summary. All three check out:
- Score tests never call the real code path.
TestScoreCalculationintest_database_manager.pyreproduces the arithmetic inline (score -= SEVERITY_WEIGHTS["HIGH"]) instead of callingDatabaseManager.get_score(). A bug in the actual SQL aggregation/floor logic would sail through untouched. - Sentinel retry tests don't verify retry count. I checked
sentinel/ingest.py:64—send()genuinely doesfor attempt in range(1, 4)(3 attempts). Buttest_send_returns_false_after_retriesandtest_send_retries_on_exceptiononly assert the finalFalse, nevermock_post.call_count == 3. Change that loop torange(1, 2)and both tests would still pass. - Chunk overlap is asserted by count, not content. Confirmed in
ai/chunker.py:51—chunk_overlapdirectly controlsstart = split_pos - chunk_overlapfor the next chunk. None of the chunker tests check that consecutive chunks actually share the expected overlapping text; they only check chunk count andchunk_indexpresence.
Agreeing with Ritik's request for changes on this basis — the coverage numbers are real (72 tests, all passing), but for the specific claims this PR makes (CI-001 through CI-004 "closed"), these three gaps mean the corresponding regressions could land undetected. Worth fixing before merge rather than as a fast-follow, since the whole point of this PR is closing coverage gaps precisely so regressions get caught.
One separate housekeeping item, also from Ritik's review and still true: the PR description says "Closes #174" but #174 is this PR's own tracking issue for the coverage gaps — please point it at the actual issue number if there is one, or drop the auto-close keyword if #174 is just this PR.
Nice test for the HMAC signature itself, by the way — test_build_signature_uses_hmac_sha256 independently recomputes the expected signature and compares against the real function's output rather than mocking it away. That's the right pattern; would be good to see the retry-count and chunk-overlap tests follow the same approach.
|
@emon22-ts , there are quite some changes to be done, please do touch base with this PR. |
parthrohit22
left a comment
There was a problem hiding this comment.
I reviewed PR #174 against its four changed test files, the existing review threads, and the underlying testing requirements in issue #153.
I agree with @ritiksah141’s three unresolved findings, which @m-khan-97 also independently confirmed:
- The score tests reproduce the arithmetic instead of calling
DatabaseManager.get_score(). - The Sentinel tests assert only the final
Falseresult and do not prove that all three retry attempts occur. - The chunker tests verify the number of chunks but not the configured overlap between adjacent chunks.
I will not repeat those inline comments. I found two additional coverage gaps:
- Issue #153 explicitly identifies
ai/embed.pyas part of the untested RAG pipeline, but this PR does not testbuild_vectorstore()or the vector-store creation path. - The drift, resources, and prioritization success tests use empty database results and assert only
200/JSON. Their main comparison, aggregation, ranking, and response-mapping behavior is therefore not exercised.
The DatabaseManager concern is particularly important: this PR’s current head contains a duplicated GROUP BY severity inside get_score(), while all the new tests still pass because none execute the real method. This defect is inherited from the older base rather than introduced by this PR, and updating the branch from current dev should incorporate its correction. A real get_score() test should nevertheless be added to prevent recurrence.
The HMAC-SHA256 test is well designed and independently verifies the production function. Most CI checks are green, and I am not attributing the acknowledged Trivy failure to this PR.
Before approval, please:
- Address the three existing unresolved review threads.
- Add non-empty success-path tests for drift, resources, and prioritization.
- Add mocked coverage for
ai/embed.py. - Update the branch from
devand rerun CI. - Correct the related issue reference:
Closes #174currently refers to this pull request itself; the matching OpenShield testing issue is #153. - Correct the per-file test counts to 20 DatabaseManager, 20 API route, 13 Sentinel, and 19 RAG tests, which total the stated 72.
Requesting changes until the claimed coverage gaps and the underlying acceptance criteria are fully addressed.
TFT444
left a comment
There was a problem hiding this comment.
@emon22-ts fix the following changes. they asking for
What does this PR do?
Closes the four test coverage gaps identified in the audit by adding unit tests for DatabaseManager, five API route files, Sentinel HMAC signing and field mappings, and the RAG pipeline.
Type of change
Coverage gaps addressed
Files added
tests/test_database_manager.py— 16 teststests/test_api_routes.py— 17 teststests/test_sentinel_ingest.py— 13 teststests/test_rag_pipeline.py— 18 testsTesting
Related issue
Closes #174