FIX #109: Implement auto-testing for all backend APIs #111
Conversation
WalkthroughThis PR establishes an automated backend API testing infrastructure. It introduces a GitHub Actions workflow to run tests on pull requests and pushes, configures pytest for test discovery, adds a comprehensive smoke test covering all major backend endpoints, implements graceful degradation for the Chromadb dependency in vector_store.py, and updates test fixture data. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Backend/agent/vector_store.py (1)
51-101: Add logging for update failures and narrow exception handling.Two concerns:
- Lines 99-100: Catching bare
Exceptionsuppresses all errors including programming bugs- No logging makes it impossible to diagnose why updates fail in CI/production
Apply this pattern:
- except Exception: + except (OSError, json.JSONDecodeError, RuntimeError) as e: + import logging + logging.error(f"Vector store update failed: {e}") return False
🧹 Nitpick comments (2)
.github/workflows/backend-tests.yml (2)
34-38: Consider moving pytest to a requirements file.Installing pytest inline works but makes the dependency implicit. Consider adding a
requirements-dev.txtorrequirements-test.txtfile to explicitly track test dependencies.Example
Backend/requirements-test.txt:pytest>=7.4.0 pytest-cov>=4.1.0Then update the workflow:
- name: Install dependencies run: | python -m pip install --upgrade pip pip install -r requirements.txt - pip install pytest + pip install -r requirements-test.txt
40-41: Consider adding coverage reporting and failure artifacts.The basic pytest run works well, but consider enhancing with:
- Coverage reporting to track test effectiveness
- Uploading test results on failure for easier debugging
- name: Run tests - run: pytest -q + run: pytest -v --cov=. --cov-report=term-missing + + - name: Upload test results on failure + if: failure() + uses: actions/upload-artifact@v4 + with: + name: test-results + path: Backend/.pytest_cache/
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/backend-tests.yml(1 hunks)Backend/agent/vector_store.py(3 hunks)Backend/cache/context_default.json(1 hunks)Backend/pytest.ini(1 hunks)Backend/tests/test_api_smoke.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.8)
Backend/agent/vector_store.py
5-5: Do not catch blind exception: Exception
(BLE001)
26-26: Do not catch blind exception: Exception
(BLE001)
48-48: Probable use of insecure hash functions in hashlib: md5
(S324)
80-81: try-except-pass detected, consider logging the exception
(S110)
80-80: Do not catch blind exception: Exception
(BLE001)
97-97: Consider moving this statement to an else block
(TRY300)
99-99: Do not catch blind exception: Exception
(BLE001)
115-115: Consider moving this statement to an else block
(TRY300)
117-117: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (10)
Backend/pytest.ini (1)
1-4: LGTM! Clean pytest configuration.The configuration appropriately restricts test discovery to API smoke tests and provides useful summary output with
-ra.Backend/agent/vector_store.py (2)
46-48: MD5 usage is acceptable for change detection.The static analysis tool flagged MD5 as insecure, but this is a false positive. MD5 is appropriate here for detecting file changes (non-cryptographic use case). No security concern.
51-53: LGTM on the availability guards.The early returns when Chromadb is unavailable properly implement graceful degradation and allow the backend to function in CI without the optional dependency.
Also applies to: 104-105
Backend/cache/context_default.json (1)
1-123: LGTM! Test fixture data properly updated.The additions of
blood_pressureanddischargetracking subdatasets align with the new API endpoints tested intest_api_smoke.py. The fixture data structure is consistent and appropriate for testing.Backend/tests/test_api_smoke.py (4)
1-9: LGTM on sys.path setup.The sys.path manipulation correctly ensures the Backend directory is importable when tests run from Backend/. This is necessary for the lazy import pattern used in the test.
12-13: LGTM on the unique identifier helper.Using UUID with a prefix provides collision-resistant test data identifiers without external dependencies.
28-29: Excellent coverage of core API endpoints.The smoke test comprehensively exercises all major backend endpoints with appropriate assertions on status codes and response structure. This will effectively catch breaking changes.
Also applies to: 33-51, 55-70, 73-93, 96-102, 105-110, 113-128, 131-145, 148-162
164-188: LGTM on agent endpoint tests.The tests correctly handle both success (200) and not-found (404) responses for optional agent features, and verify the response structure when data is available. The comment about using an offline stub is helpful context.
.github/workflows/backend-tests.yml (2)
1-12: LGTM on workflow triggers.The path filters appropriately limit when tests run, avoiding unnecessary CI runs for unrelated changes. Triggering on both PRs and main branch pushes ensures comprehensive coverage.
26-33: LGTM on Python setup with caching.Python 3.11 is a good choice, and the pip cache configuration with the dependency path will speed up CI runs.
| try: | ||
| import chromadb | ||
| from chromadb.utils import embedding_functions | ||
| _CHROMA_AVAILABLE = True | ||
| except Exception: | ||
| chromadb = None | ||
| embedding_functions = None | ||
| _CHROMA_AVAILABLE = False |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Narrow the exception handling for imports.
Catching bare Exception on line 5 is too broad. Use ImportError to specifically handle missing module imports, as other exceptions during import (syntax errors, etc.) should not be silently caught.
Apply this diff:
try:
import chromadb
from chromadb.utils import embedding_functions
_CHROMA_AVAILABLE = True
-except Exception:
+except ImportError:
chromadb = None
embedding_functions = None
_CHROMA_AVAILABLE = False📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| import chromadb | |
| from chromadb.utils import embedding_functions | |
| _CHROMA_AVAILABLE = True | |
| except Exception: | |
| chromadb = None | |
| embedding_functions = None | |
| _CHROMA_AVAILABLE = False | |
| try: | |
| import chromadb | |
| from chromadb.utils import embedding_functions | |
| _CHROMA_AVAILABLE = True | |
| except ImportError: | |
| chromadb = None | |
| embedding_functions = None | |
| _CHROMA_AVAILABLE = False |
🧰 Tools
🪛 Ruff (0.14.8)
5-5: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In Backend/agent/vector_store.py around lines 1 to 8, the import block currently
catches a broad Exception; narrow it to catch only ImportError (or
ModuleNotFoundError) so real import-time errors (like SyntaxError) are not
swallowed: replace "except Exception:" with "except ImportError:" (optionally
"except (ImportError, ModuleNotFoundError):") and keep setting chromadb = None,
embedding_functions = None, and _CHROMA_AVAILABLE = False in that except block.
| if _CHROMA_AVAILABLE: | ||
| try: | ||
| os.makedirs("db/chromadb", exist_ok=True) | ||
| client = chromadb.PersistentClient(path="db/chromadb") | ||
|
|
||
| # Use default embedding function instead of sentence transformers | ||
| collection = client.get_or_create_collection( | ||
| "pregnancy_guidelines", | ||
| embedding_function=embedding_functions.DefaultEmbeddingFunction() | ||
| ) | ||
| collection = client.get_or_create_collection( | ||
| "pregnancy_guidelines", | ||
| embedding_function=embedding_functions.DefaultEmbeddingFunction() | ||
| ) | ||
| except Exception: | ||
| # If init fails, degrade gracefully | ||
| client = None | ||
| collection = None | ||
| _CHROMA_AVAILABLE = False |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add logging and narrow exception handling for initialization.
The initialization block has two issues:
- Line 26 catches bare
Exception, which suppresses all errors including programming mistakes - No logging makes debugging initialization failures difficult in CI
Consider this approach:
if _CHROMA_AVAILABLE:
try:
os.makedirs("db/chromadb", exist_ok=True)
client = chromadb.PersistentClient(path="db/chromadb")
collection = client.get_or_create_collection(
"pregnancy_guidelines",
embedding_function=embedding_functions.DefaultEmbeddingFunction()
)
except (RuntimeError, OSError) as e:
# If init fails, degrade gracefully
import logging
logging.warning(f"Chromadb initialization failed: {e}")
client = None
collection = None
_CHROMA_AVAILABLE = False🧰 Tools
🪛 Ruff (0.14.8)
26-26: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In Backend/agent/vector_store.py around lines 17 to 30, replace the broad
"except Exception" with a narrowed exception handler (e.g., except
(RuntimeError, OSError) as e) and add logging: import logging and emit a warning
that includes the exception details (e.g., logging.warning(f"Chromadb
initialization failed: {e}")) before degrading gracefully by setting client =
None, collection = None and _CHROMA_AVAILABLE = False; this preserves useful
error information for CI while avoiding suppression of unrelated programming
errors.
| try: | ||
| count = collection.count() | ||
| if count > 0: | ||
| collection.delete(where={"source": {"$ne": "none"}}) | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add logging to the silent error handler.
The try-except-pass block silently ignores errors when clearing the collection. This makes debugging difficult in production. Consider logging the exception.
try:
count = collection.count()
if count > 0:
collection.delete(where={"source": {"$ne": "none"}})
- except Exception:
- pass
+ except Exception as e:
+ import logging
+ logging.warning(f"Failed to clear collection: {e}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| count = collection.count() | |
| if count > 0: | |
| collection.delete(where={"source": {"$ne": "none"}}) | |
| except Exception: | |
| pass | |
| try: | |
| count = collection.count() | |
| if count > 0: | |
| collection.delete(where={"source": {"$ne": "none"}}) | |
| except Exception as e: | |
| import logging | |
| logging.warning(f"Failed to clear collection: {e}") |
🧰 Tools
🪛 Ruff (0.14.8)
80-81: try-except-pass detected, consider logging the exception
(S110)
80-80: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In Backend/agent/vector_store.py around lines 76-81, the try/except currently
swallows all exceptions; update it to catch Exception as e and log the error
(including stacktrace) instead of passing so failures when counting/deleting the
collection are visible in logs; if a module logger exists use
logger.exception(...) (or import logging and call
logging.exception(...)/logging.getLogger(__name__).exception(...)) and keep the
existing behavior of not re-raising the exception.
| def query_vector_store(query: str, n_results: int = 3): | ||
| """Query the vector store for relevant guidelines.""" | ||
| if not _CHROMA_AVAILABLE or collection is None: | ||
| return [] | ||
|
|
||
| try: | ||
| results = collection.query( | ||
| query_texts=[query], | ||
| n_results=n_results | ||
| ) | ||
|
|
||
| if results and results.get('documents'): | ||
| return results['documents'][0] | ||
| return [] | ||
|
|
||
| except Exception as e: | ||
| print(f"Error querying vector store: {e}") | ||
|
|
||
| if results and results.get("documents"): | ||
| return results["documents"][0] | ||
| return [] | ||
|
|
||
| except Exception: | ||
| return [] |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add logging for query failures and narrow exception handling.
Similar to update_vector_store, the query function catches bare Exception without logging, making debugging difficult.
- except Exception:
+ except (RuntimeError, KeyError) as e:
+ import logging
+ logging.warning(f"Vector store query failed: {e}")
return []🧰 Tools
🪛 Ruff (0.14.8)
115-115: Consider moving this statement to an else block
(TRY300)
117-117: Do not catch blind exception: Exception
(BLE001)
|
|
||
| # Symptoms: POST + GET | ||
| r = client.post("/symptoms", json={"week_number": 8, "symptom": "nausea", "note": "pytest"}) | ||
| assert r.status_code in (200, 201) |
There was a problem hiding this comment.
Remove unnecessary 200/201 status code permissiveness in assertions.
The test assertions at lines 106, 141, and 158 accept both 200 and 201 status codes, but the API implementation only returns 200. All three POST endpoints (/post_symptoms, /post_blood_pressure, /post_set_discharge_log) in Backend/app.py use Flask's jsonify() function without explicitly specifying a status code, which defaults to 200 OK. There is no logic to return 201 (Created). Consider changing these assertions to expect only 200 to match the actual API behavior and make test intent clearer.
🤖 Prompt for AI Agents
In Backend/tests/test_api_smoke.py around lines 106, 141 and 158, the assertions
currently allow both 200 and 201 but the Flask endpoints always return 200;
update each assertion to require exactly 200 (e.g., replace "assert
r.status_code in (200, 201)" with "assert r.status_code == 200") so the tests
accurately reflect the API behavior.
1 participant
Closes #109
📝 Description
This PR adds automated backend API testing to ensure core endpoints do not break on future changes.
A GitHub Actions workflow is introduced to run these tests on every pull request and push to
main.🔧 Changes Made
chromadbas an optional dependency📷 Screenshots or Visual Changes (if applicable)
N/A
🤝 Collaboration
Collaborated with: N/A
✅ Checklist
Our Team Name is EtherX
Members:
-Sirjan Singh
-Ritigya Gupta
-Heeral Mandolia
Summary by CodeRabbit
Chores
Tests
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.