tests: refresh core unit/integration/e2e tests; training: add minimal helpers; update changelog#86
tests: refresh core unit/integration/e2e tests; training: add minimal helpers; update changelog#86uelkerd wants to merge 82 commits into
Conversation
…ck, rate limiter, testing config; update changelog
… helpers; update changelog
Reviewer's GuideThis PR refreshes the core testing suite by consolidating and cleaning up unit, integration, and e2e tests (removing duplicates, standardizing fixtures, and formatting), introduces minimal helper routines in training scripts, tidies testing configuration and scripts formatting, and records these updates in the changelog. Sequence diagram for the refreshed training workflow with helperssequenceDiagram
participant Main
participant TrainingHelper
participant Model
participant Logger
Main->>TrainingHelper: create_proper_training_data()
TrainingHelper->>Logger: info (Created training/val/test samples)
Main->>Model: SimpleBERTClassifier()
Main->>TrainingHelper: create_dataloader(train_data, model)
Main->>TrainingHelper: train_model(model, train_dataloader, val_dataloader, device, epochs)
TrainingHelper->>Logger: info (Training started)
TrainingHelper->>Model: to(device)
TrainingHelper->>Model: optimizer.step()
TrainingHelper->>Logger: info (Epoch results)
Main->>Model: load_state_dict(best_model)
Main->>TrainingHelper: evaluate_model(model, test_dataloader, device)
TrainingHelper->>Logger: info (Evaluation results)
Main->>Logger: info (Training completed)
Class diagram for new minimal training helpers in training scriptsclassDiagram
class SimpleBERTClassifier
class FocalLoss
class TrainingHelper {
+create_proper_training_data()
+create_dataloader(data, model, batch_size)
+train_model(model, train_data, val_data, device, epochs)
+evaluate_model(model, test_data, device)
}
SimpleBERTClassifier <|-- TrainingHelper
FocalLoss <|-- TrainingHelper
Class diagram for updated testing config helperclassDiagram
class TestingConfig {
+_get_base_url()
}
TestingConfig : env_url = (API_BASE_URL or CLOUD_RUN_API_URL or MODEL_API_BASE_URL)
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Warning Rate limit exceeded@uelkerd has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 12 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (23)
📒 Files selected for processing (58)
WalkthroughAdds a “Tests & Training” note to CHANGELOG. Removes two legacy test scripts. Introduces new/expanded tests (integration, rate-limiter, security, large-payload API tests). Updates fixed_focal_training with expanded data, tokenization padding/truncation, threshold sweep and persisted results. Enhances Colab setup to install more deps, handle errors broadly, and generate a runnable multi-cell notebook. Changes
Sequence Diagram(s)sequenceDiagram
participant Trainer
participant DataLoader
participant Tokenizer
participant Model
participant Evaluator
participant ResultsStore
Trainer->>DataLoader: build expanded dataset
DataLoader->>Tokenizer: tokenize(padding=True, truncation=True, max_length=...)
Tokenizer-->>DataLoader: tokenized batches
DataLoader->>Model: train/eval batches
Model-->>Evaluator: logits/probabilities
loop thresholds 0.1..0.9
Evaluator->>Evaluator: compute F1/Precision/Recall
end
Evaluator-->>Trainer: best_threshold + metrics
Trainer->>ResultsStore: save results JSON (incl. best_threshold)
sequenceDiagram
participant User
participant SetupScript
participant Pip
participant GitHub
participant GPU
participant CI
User->>SetupScript: run setup_colab_environment.py
SetupScript->>Pip: install expanded dependencies
SetupScript->>GitHub: git clone repo
SetupScript->>GPU: check CUDA availability
alt success
SetupScript->>User: generate runnable Colab notebook
User->>CI: run CI cell (logs stdout on success)
else failure
SetupScript-->>User: log error and return False
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Here's the code health analysis summary for commits Analysis Summary
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @uelkerd, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request primarily focuses on refreshing and standardizing the existing unit, integration, and end-to-end test suites across the project. It also introduces minor helper utilities for the training scripts. The changes aim to improve code consistency, ensure core functionalities are robustly tested, and provide foundational support for future training efforts, all while keeping the scope minimal to validate core flows and CI signal.
Highlights
- Test Suite Refresh: Extensive updates across various test files (scripts/testing/, tests/) to standardize formatting, remove redundant empty lines, and apply minor logical adjustments to existing test cases. This includes comprehensive API tests, model status checks, and security feature validations.
- Training Helper Enhancements: Minor formatting and structural cleanups in training-related scripts (scripts/training/fixed_focal_training.py, scripts/training/setup_colab_environment.py) to improve readability and consistency.
- Changelog Update: The CHANGELOG.md file has been updated to reflect the refresh of core tests and the addition of minimal training helpers, emphasizing a small scope for validating core flows and CI signal.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider splitting the whitespace-only formatting changes into a separate commit or PR to reduce review noise and clearly isolate functional updates.
- The TestWebSocketAuthentication tests currently use pass and have no assertions—either implement them or mark them as skipped to avoid silent test passes.
- Add lightweight validation tests for the new training helpers (e.g. verifying data lengths and label distributions) to ensure they produce the expected structures.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider splitting the whitespace-only formatting changes into a separate commit or PR to reduce review noise and clearly isolate functional updates.
- The TestWebSocketAuthentication tests currently use pass and have no assertions—either implement them or mark them as skipped to avoid silent test passes.
- Add lightweight validation tests for the new training helpers (e.g. verifying data lengths and label distributions) to ensure they produce the expected structures.
## Individual Comments
### Comment 1
<location> `tests/integration/test_priority1_features.py:487` </location>
<code_context>
pass
-
+
def test_websocket_with_valid_token(self):
"""Test WebSocket connection with valid token."""
# This would require a WebSocket client test
</code_context>
<issue_to_address>
WebSocket valid token test is a placeholder and does not verify functionality.
Please implement a test that connects via WebSocket and verifies authentication with a valid token, or remove the placeholder if this cannot be tested.
</issue_to_address>
### Comment 2
<location> `tests/integration/test_priority1_features.py:227` </location>
<code_context>
-
+
@patch('src.unified_ai_api.voice_transcriber')
def test_batch_transcription(self, mock_transcriber):
"""Test batch transcription endpoint."""
</code_context>
<issue_to_address>
No test for empty batch input in batch transcription endpoint.
Add a test to ensure the endpoint returns a meaningful error when called with an empty batch.
</issue_to_address>
### Comment 3
<location> `tests/integration/test_priority1_features.py:295` </location>
<code_context>
-
+
@patch('src.unified_ai_api.voice_transcriber')
def test_batch_transcription_partial_failures(self, mock_transcriber):
"""Test batch transcription with partial failures."""
</code_context>
<issue_to_address>
No test for all failures in batch transcription.
Please add a test that covers the scenario where every file in the batch fails, verifying the API's response and error handling in this case.
</issue_to_address>
### Comment 4
<location> `tests/integration/test_priority1_features.py:496` </location>
<code_context>
"""Test API endpoint validation and error handling."""
-
+
def test_voice_transcription_file_size_validation(self):
"""Test file size validation for voice transcription."""
# Login to get token
</code_context>
<issue_to_address>
No test for file size exactly at the limit for voice transcription.
Add a test case with an audio file size equal to the limit to confirm correct boundary handling.
Suggested implementation:
```python
def test_voice_transcription_file_size_validation(self):
"""Test file size validation for voice transcription."""
# Login to get token
def test_voice_transcription_file_size_at_limit(self):
"""Test file size exactly at the limit for voice transcription."""
# Login to get token
client = self.client if hasattr(self, "client") else None
if client is None:
from fastapi.testclient import TestClient
from main import app
client = TestClient(app)
login_data = {
"username": "testuser",
"password": "testpassword"
}
login_response = client.post("/auth/login", json=login_data)
access_token = login_response.json()["access_token"]
# Assume the file size limit is defined somewhere, e.g. 5MB
FILE_SIZE_LIMIT = 5 * 1024 * 1024 # 5MB in bytes
# Create a dummy audio file exactly at the limit
audio_content = b"\0" * FILE_SIZE_LIMIT
files = {
"audio": ("test_limit.wav", audio_content, "audio/wav")
}
headers = {"Authorization": f"Bearer {access_token}"}
response = client.post("/voice/transcribe", files=files, headers=headers)
# Expect success (200 or 202) or whatever is correct for your API
assert response.status_code in (200, 202), f"Unexpected status code: {response.status_code}"
```
- Adjust `FILE_SIZE_LIMIT` to match the actual limit enforced by your API if it differs.
- If your test setup uses fixtures for `client`, `login_data`, or other values, adapt the test to use those instead.
- If the endpoint or expected status code is different, update accordingly.
</issue_to_address>
### Comment 5
<location> `tests/unit/test_api_security.py:64` </location>
<code_context>
self.assertEqual(stats['concurrent_requests'], 0)
-
+
def test_rate_limit_exceeded(self):
"""Test rate limit exceeded scenario."""
client_ip = "192.168.1.2"
</code_context>
<issue_to_address>
No test for rate limiting with multiple user agents from the same IP.
Please add a test to ensure rate limiting distinguishes between user agents sharing the same IP, verifying correct behavior per client key.
</issue_to_address>
### Comment 6
<location> `tests/unit/test_api_security.py:116` </location>
<code_context>
self.assertEqual(reason, "IP not allowed")
-
+
def test_abuse_detection(self):
"""Test abuse detection functionality."""
client_ip = "192.168.1.4"
</code_context>
<issue_to_address>
No test for abuse detection reset after cooldown period.
Add a test to confirm that, after the cooldown period, abuse detection resets and requests from the client are permitted.
</issue_to_address>
### Comment 7
<location> `scripts/testing/test_cloud_run_api_endpoints.py:156` </location>
<code_context>
def test_invalid_inputs(self) -> Dict[str, Any]:
"""Test invalid input handling"""
logger.info("Testing invalid inputs...")
</code_context>
<issue_to_address>
No test for extremely large payloads in invalid input tests.
Add a test for the emotion detection endpoint using an extremely large payload to ensure the API remains stable under such conditions.
</issue_to_address>
### Comment 8
<location> `tests/unit/test_validation_enhanced.py:26` </location>
<code_context>
def test_check_missing_values_basic(self):
"""Test basic missing values check."""
missing_stats = self.validator.check_missing_values(self.test_df)
</code_context>
<issue_to_address>
No test for missing values in all columns.
Add a test where every column in the DataFrame has missing values to ensure the validator handles this case correctly.
</issue_to_address>
### Comment 9
<location> `tests/unit/test_api_rate_limiter.py:59` </location>
<code_context>
def test_allow_request_rate_limit_exceeded(self):
"""Test that allow_request returns False when rate limit exceeded."""
config = RateLimitConfig(
- requests_per_minute=1,
+ requests_per_minute=1,
burst_size=1,
enable_user_agent_analysis=False, # Disable abuse detection for testing
enable_request_pattern_analysis=False
</code_context>
<issue_to_address>
Consider adding tests for edge cases such as zero or negative rate limits.
Please add tests for cases where `requests_per_minute` or `burst_size` are zero or negative to verify correct handling.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
def test_allow_request_rate_limit_exceeded(self):
"""Test that allow_request returns False when rate limit exceeded."""
config = RateLimitConfig(
requests_per_minute=1,
burst_size=1,
enable_user_agent_analysis=False, # Disable abuse detection for testing
enable_request_pattern_analysis=False
=======
def test_allow_request_rate_limit_exceeded(self):
"""Test that allow_request returns False when rate limit exceeded."""
config = RateLimitConfig(
requests_per_minute=1,
burst_size=1,
enable_user_agent_analysis=False, # Disable abuse detection for testing
enable_request_pattern_analysis=False
def test_allow_request_zero_rate_limits(self):
"""Test that allow_request returns False when rate limits are zero."""
config = RateLimitConfig(
requests_per_minute=0,
burst_size=0,
enable_user_agent_analysis=False,
enable_request_pattern_analysis=False
)
limiter = ApiRateLimiter(config)
request = self._make_request()
allowed = limiter.allow_request(request)
assert allowed is False
def test_allow_request_negative_rate_limits(self):
"""Test that allow_request returns False when rate limits are negative."""
config = RateLimitConfig(
requests_per_minute=-1,
burst_size=-5,
enable_user_agent_analysis=False,
enable_request_pattern_analysis=False
)
limiter = ApiRateLimiter(config)
request = self._make_request()
allowed = limiter.allow_request(request)
assert allowed is False
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| pass | ||
|
|
||
| def test_websocket_with_valid_token(self): |
There was a problem hiding this comment.
issue (testing): WebSocket valid token test is a placeholder and does not verify functionality.
Please implement a test that connects via WebSocket and verifies authentication with a valid token, or remove the placeholder if this cannot be tested.
|
|
||
| @patch('src.unified_ai_api.voice_transcriber') | ||
| def test_batch_transcription(self, mock_transcriber): |
There was a problem hiding this comment.
suggestion (testing): No test for empty batch input in batch transcription endpoint.
Add a test to ensure the endpoint returns a meaningful error when called with an empty batch.
|
|
||
| @patch('src.unified_ai_api.voice_transcriber') | ||
| def test_batch_transcription_partial_failures(self, mock_transcriber): |
There was a problem hiding this comment.
suggestion (testing): No test for all failures in batch transcription.
Please add a test that covers the scenario where every file in the batch fails, verifying the API's response and error handling in this case.
| """Test API endpoint validation and error handling.""" | ||
|
|
||
| def test_voice_transcription_file_size_validation(self): |
There was a problem hiding this comment.
suggestion (testing): No test for file size exactly at the limit for voice transcription.
Add a test case with an audio file size equal to the limit to confirm correct boundary handling.
Suggested implementation:
def test_voice_transcription_file_size_validation(self):
"""Test file size validation for voice transcription."""
# Login to get token
def test_voice_transcription_file_size_at_limit(self):
"""Test file size exactly at the limit for voice transcription."""
# Login to get token
client = self.client if hasattr(self, "client") else None
if client is None:
from fastapi.testclient import TestClient
from main import app
client = TestClient(app)
login_data = {
"username": "testuser",
"password": "testpassword"
}
login_response = client.post("/auth/login", json=login_data)
access_token = login_response.json()["access_token"]
# Assume the file size limit is defined somewhere, e.g. 5MB
FILE_SIZE_LIMIT = 5 * 1024 * 1024 # 5MB in bytes
# Create a dummy audio file exactly at the limit
audio_content = b"\0" * FILE_SIZE_LIMIT
files = {
"audio": ("test_limit.wav", audio_content, "audio/wav")
}
headers = {"Authorization": f"Bearer {access_token}"}
response = client.post("/voice/transcribe", files=files, headers=headers)
# Expect success (200 or 202) or whatever is correct for your API
assert response.status_code in (200, 202), f"Unexpected status code: {response.status_code}"- Adjust
FILE_SIZE_LIMITto match the actual limit enforced by your API if it differs. - If your test setup uses fixtures for
client,login_data, or other values, adapt the test to use those instead. - If the endpoint or expected status code is different, update accordingly.
| self.assertEqual(stats['concurrent_requests'], 0) | ||
|
|
||
| def test_rate_limit_exceeded(self): |
There was a problem hiding this comment.
suggestion (testing): No test for rate limiting with multiple user agents from the same IP.
Please add a test to ensure rate limiting distinguishes between user agents sharing the same IP, verifying correct behavior per client key.
| for i in range(100): | ||
| # Mock different request contexts | ||
| with self.app.test_request_context('/'): | ||
| from flask import g, request | ||
| request.remote_addr = f'192.168.1.{i}' | ||
|
|
||
| # Generate request ID | ||
| self.middleware._before_request() | ||
| request_ids.add(g.request_id) |
There was a problem hiding this comment.
issue (code-quality): Avoid loops in tests. (no-loop-in-tests)
Explanation
Avoid complex code, like loops, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| # Check multiple environment variables for flexibility | ||
| env_url = (os.environ.get("API_BASE_URL") or | ||
| os.environ.get("CLOUD_RUN_API_URL") or | ||
| env_url = (os.environ.get("API_BASE_URL") or | ||
| os.environ.get("CLOUD_RUN_API_URL") or | ||
| os.environ.get("MODEL_API_BASE_URL")) | ||
| if env_url: |
There was a problem hiding this comment.
suggestion (code-quality): Use named expression to simplify assignment and conditional (use-named-expression)
| # Check multiple environment variables for flexibility | |
| env_url = (os.environ.get("API_BASE_URL") or | |
| os.environ.get("CLOUD_RUN_API_URL") or | |
| env_url = (os.environ.get("API_BASE_URL") or | |
| os.environ.get("CLOUD_RUN_API_URL") or | |
| os.environ.get("MODEL_API_BASE_URL")) | |
| if env_url: | |
| if env_url := ( | |
| os.environ.get("API_BASE_URL") | |
| or os.environ.get("CLOUD_RUN_API_URL") | |
| or os.environ.get("MODEL_API_BASE_URL") | |
| ): |
| @@ -26,10 +26,10 @@ def test_vertex_setup(): | |||
| config_dir = Path("configs/vertex_ai") | |||
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Extract code out into function (
extract-method) - Extract duplicate code into function (
extract-duplicate-method)
|
|
||
| if torch.cuda.is_available(): | ||
| logger.info(f"🎮 GPU detected: {torch.cuda.get_device_name(0)}") |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Extract code out into function (
extract-method) - Hoist repeated code outside conditional statement (
hoist-statement-from-if)
| anomalies = self.sanitizer.detect_anomalies(large_data) | ||
| self.assertGreater(len(anomalies), 0) | ||
| self.assertIn("Large string", anomalies[0]) | ||
|
|
||
| # Potential SQL injection anomaly | ||
| sql_data = {"text": "SELECT * FROM users"} | ||
| anomalies = self.sanitizer.detect_anomalies(sql_data) | ||
| self.assertGreater(len(anomalies), 0) | ||
| self.assertIn("SQL injection", anomalies[0]) |
There was a problem hiding this comment.
issue (code-quality): Extract duplicate code into method (extract-duplicate-method)
There was a problem hiding this comment.
Code Review
This pull request primarily focuses on refreshing tests and standardizing code formatting, which is a good step towards improving code consistency. The changes are mostly about removing extra whitespace. I've identified a critical syntax error in one of the test files due to incorrect indentation, which must be fixed. Additionally, I've suggested a refactoring for a repetitive code block in a training script to enhance maintainability. Once the critical issue is resolved, this PR will be in good shape.
| } | ||
| is_valid, info = self.validator.validate_version_compatibility(test_config) | ||
| # Note: This may fail with current PyTorch version, but that's expected behavior | ||
| # Note: This may fail with current PyTorch version, but that's expected behavior |
There was a problem hiding this comment.
The indentation of this comment is incorrect, which will cause a SyntaxError: unexpected indent. Please fix the indentation.
| # Note: This may fail with current PyTorch version, but that's expected behavior | |
| # Note: This may fail with current PyTorch version, but that's expected behavior |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (3)
scripts/testing/test_cloud_run_api_endpoints.py (1)
22-26: Ensurebase_urloverride is applied when instantiating the API clientThe tester computes and stores
self.base_url, but callingcreate_api_client()always builds a newTestConfig()(and thus its ownbase_url), so your requests may go to a different endpoint than you print.• File:
scripts/testing/test_cloud_run_api_endpoints.py
– Change how you build the client to use your overridden URL.Proposed diff:
from test_config import create_test_config, create_api_client +from test_config import TestConfig, APIClient class CloudRunAPITester: def __init__(self, base_url: str = None): - config = create_test_config() - self.base_url = base_url or config.base_url - self.client = create_api_client() + # Use the override (if provided) when constructing the TestConfig + config = TestConfig(base_url=base_url) + self.base_url = config.base_url + # Instantiate APIClient directly so it picks up our config.base_url + self.client = APIClient(config)If you prefer keeping the
create_api_clientfactory, add abase_urlparameter to its signature and pass it through toTestConfigthere instead.scripts/testing/test_model_status.py (1)
69-73: Propagate Base URL override to the API clientYour test mutates
config.base_url, butcreate_api_client()builds its own config internally and ignores this change. To ensure the override takes effect:• Update scripts/testing/test_config.py:
– Changecreate_api_client()to accept an optionalbase_urlparameter.
– Applybase_url.rstrip('/')inside the factory before constructing the client.• Update scripts/testing/test_model_status.py:
– Pass the overriddenconfig.base_urlintocreate_api_client().# scripts/testing/test_config.py -def create_api_client() -> APIClient: +def create_api_client(base_url: Optional[str] = None) -> APIClient: """Factory function to create API client""" - config = create_test_config() + config = create_test_config() + if base_url: + config.base_url = base_url.rstrip('/') return APIClient(config)# scripts/testing/test_model_status.py - client = create_api_client() + client = create_api_client(base_url=config.base_url)If you also need base-URL overrides in non-test code, mirror this change in scripts/testing/config.py.
scripts/testing/config.py (1)
138-149: Fix NameError risk fromrequests.Responsetype hints
APIClient.get/postreturn annotations referencerequests.Response, butrequestsisn’t imported at module scope. Without postponed annotations, this raises at import time in Python <3.11. Two straightforward fixes:
- Add a module-level
import requests(and remove the local one), or- Use postponed annotations (
from __future__ import annotations) and/or quote annotations.Minimal, explicit import approach below.
@@ import os import argparse import time from typing import Optional +import requests @@ def create_api_client(): """Create a reusable API client with common functionality.""" - import requests from requests.adapters import HTTPAdapter from urllib3.util.retry import RetryAlternatively (outside the selected lines), add at the top after the docstring:
"""
from future import annotations
"""
and change return annotations to strings:
def get(...) -> "requests.Response": ...
def post(...) -> "requests.Response": ...Also applies to: 7-11, 111-111
🧹 Nitpick comments (41)
tests/unit/test_csp_config.py (3)
44-51: Avoid globally patching os.path.join; patch module-local join insteadPatching 'os.path.join' globally can affect code outside the intended path resolution (including imports deep in libraries), making the test brittle. Patch the function inside the module under test to scope the effect.
Suggested change:
- with patch('os.path.join', return_value=config_path): + with patch('security_headers.os.path.join', return_value=config_path): middleware = SecurityHeadersMiddleware(self.app, self.config)
80-86: Scope the path patch to the target module for invalid YAML testSame rationale as above—limit the monkey patch to the module under test.
- with patch('os.path.join', return_value=config_path): + with patch('security_headers.os.path.join', return_value=config_path): middleware = SecurityHeadersMiddleware(self.app, self.config)
107-114: Limit os.path.join patch in “missing CSP key” test to avoid test flakinessPatch within the security_headers module namespace rather than globally.
- with patch('os.path.join', return_value=config_path): + with patch('security_headers.os.path.join', return_value=config_path): middleware = SecurityHeadersMiddleware(self.app, self.config)tests/unit/test_sandbox_executor.py (2)
122-129: Rename unused loop variable to underscoreThe loop control variable isn’t used. Rename to underscore to silence linters and clarify intent.
- threads = [] - for i in range(5): + threads = [] + for _ in range(5): thread = threading.Thread(target=worker_function) threads.append(thread) thread.start()
157-167: Timeout test doesn’t actually test timeoutself.executor is configured with max_wall_time=15, but slow_function sleeps for 2 seconds. The test never triggers a timeout, contradicting the docstring/comment. Consider creating a short-timeout executor inside this test.
def test_timeout_handling(self): """Test timeout handling.""" - def slow_function(): + # Use a short wall time to force timeout + local_executor = SandboxExecutor(max_wall_time=1, max_cpu_time=1) + + def slow_function(): time.sleep(2) # Sleep longer than max_wall_time return "Should timeout" - result, meta = self.executor.execute_safely(slow_function) + result, meta = local_executor.execute_safely(slow_function) - # Should either timeout or complete within limits - self.assertIsNotNone(result or meta.get('error')) + # Should timeout and return an error meta + self.assertIn('error', meta)If SandboxExecutor’s timeout is enforced via signals, this test may only pass on the main thread/Unix. If you need me to gate this test by platform, I can add a skip decorator accordingly.
tests/unit/test_secure_model_loader.py (5)
486-493: Prefer pathlib for existence check and remove redundant open() modeSmall cleanup to modernize and satisfy linters:
- Use Path.exists() instead of os.path.exists
- open() defaults to read mode; the explicit 'r' is unnecessary
- audit_log_path = os.path.join(self.temp_dir, "audit.log") - self.assertTrue(os.path.exists(audit_log_path)) + from pathlib import Path + audit_log_path = Path(self.temp_dir) / "audit.log" + self.assertTrue(audit_log_path.exists()) # Check audit log contains entries - with open(audit_log_path, 'r') as f: + with open(audit_log_path) as f: log_content = f.read() self.assertIn('AUDIT:', log_content)
263-269: Optional: adopt pathlib for path assemblyNot required, but replacing os.path.join with Path improves readability and lint friendliness.
- self.model_file = os.path.join(self.temp_dir, "test_model.pt") + from pathlib import Path + self.model_file = str(Path(self.temp_dir) / "test_model.pt")Note: Cast to str if downstream APIs expect a string path.
378-383: Optional: pathlib for model_file in integration testsSame as above; minor readability win.
- self.model_file = os.path.join(self.temp_dir, "test_model.pt") + from pathlib import Path + self.model_file = str(Path(self.temp_dir) / "test_model.pt")
448-456: Optional: pathlib for tampered model path- tampered_model_file = os.path.join(self.temp_dir, "tampered_model.pt") + from pathlib import Path + tampered_model_file = str(Path(self.temp_dir) / "tampered_model.pt")
224-231: Minor: comment indentationThe inline comment about potential version mismatch appears over-indented, which can read as a continuation of the previous block. It’s harmless, but aligning it to the method’s indentation improves readability.
- is_valid, info = self.validator.validate_version_compatibility(test_config) - # Note: This may fail with current PyTorch version, but that's expected behavior + is_valid, info = self.validator.validate_version_compatibility(test_config) + # Note: This may fail with current PyTorch version, but that's expected behaviortests/unit/test_hash_security.py (1)
45-49: Prefer setting remote_addr via environ_overrides instead of assigning request.remote_addrAssigning request.remote_addr directly can be brittle as it's a derived property in Flask. Use environ_overrides in test_request_context for reliability.
Apply this diff within the context block:
- with self.app.test_request_context('/'): - # Mock request.remote_addr - request.remote_addr = '192.168.1.1' + with self.app.test_request_context('/', environ_overrides={'REMOTE_ADDR': '192.168.1.1'}): + # request.remote_addr will reflect environ overridetests/unit/test_api_security.py (4)
86-89: Rename unused loop variable to_(ruff B007)The loop variable isn’t used. Rename to
_to satisfy linters and clarify intent.- for i in range(3): + for _ in range(3): allowed, reason, meta = self.rate_limiter.allow_request(client_ip, user_agent) self.assertTrue(allowed)
103-105: Rename unused loop variable to_(ruff B007)Same rationale as above.
- for i in range(3): + for _ in range(3): self.rate_limiter.release_request(client_ip, user_agent)
121-124: Rename unused loop variable to_(ruff B007)The index isn’t used; this is a simulated rapid-fire append.
- for i in range(11): # More than 10 requests in 1 second + for _ in range(11): # More than 10 requests in 1 second self.rate_limiter.request_history[self.rate_limiter._get_client_key(client_ip, user_agent)].append(time.time())
135-140: Rename unused loop variable to_(ruff B007)Minor lint/readability fix.
- for i in range(5): + for _ in range(5): allowed, _, _ = self.rate_limiter.allow_request(client_ip, user_agent) self.assertTrue(allowed) self.rate_limiter.release_request(client_ip, user_agent)tests/conftest.py (1)
102-112: Yield the TestClient from the fixture to ensure proper cleanupUsing a context manager avoids lingering connections/resources and keeps the fixture idiomatic.
@pytest.fixture def api_client(): """Provide FastAPI test client.""" - client = TestClient(app) - - # Reset rate limiter state before each test - if hasattr(app.state, 'rate_limiter'): - app.state.rate_limiter.reset_state() - - return client + with TestClient(app) as client: + # Reset rate limiter state before each test + if hasattr(app.state, 'rate_limiter'): + app.state.rate_limiter.reset_state() + yield clientCHANGELOG.md (1)
7-10: Nice, concise Unreleased notesClear scope and intent for the tests/training refresh. Consider linking the PR for traceability, e.g., “(see PR #86)”.
tests/integration/test_priority1_features.py (5)
164-174: Test is a no-op: it patches but never exercises the endpointThe test sets a return value but doesn’t call /transcribe/voice or assert any behavior. Add a minimal call and assertions.
@patch('src.unified_ai_api.voice_transcriber') def test_voice_transcription_endpoint(self, mock_transcriber): """Test enhanced voice transcription endpoint.""" # Mock transcription result mock_transcriber.return_value.transcribe.return_value = { "text": "This is a test transcription", "language": "en", "confidence": 0.95, "duration": 10.5 } + # Login to get token + login_data = {"username": "testuser@example.com", "password": "testpassword123"} + login_response = client.post("/auth/login", json=login_data) + access_token = login_response.json()["access_token"] + headers = {"Authorization": f"Bearer {access_token}"} + # Create a small temp wav payload + with tempfile.NamedTemporaryFile(suffix=".wav", delete=False) as tmp: + tmp.write(b"fake audio data") + path = tmp.name + try: + with open(path, "rb") as fh: + files = {"audio_file": ("test.wav", fh, "audio/wav")} + data = {"language": "en", "model_size": "base"} + resp = client.post("/transcribe/voice", files=files, data=data, headers=headers) + assert resp.status_code == 200 + body = resp.json() + assert "text" in body + assert "language" in body + assert "confidence" in body + assert "duration" in body + finally: + Path(path).unlink(missing_ok=True)
411-420: Summarization test also doesn’t hit the endpointSame as above: patch is set, but no /summarize/text call or assertions are made.
@patch('src.unified_ai_api.text_summarizer') def test_text_summarization_endpoint(self, mock_summarizer): """Test enhanced text summarization endpoint.""" # Mock summarization result mock_summarizer.return_value.summarize.return_value = { "summary": "This is a test summary of the input text.", "key_emotions": ["neutral"], "compression_ratio": 0.75 } + # Login to get token + login_data = {"username": "testuser@example.com", "password": "testpassword123"} + login_response = client.post("/auth/login", json=login_data) + access_token = login_response.json()["access_token"] + headers = {"Authorization": f"Bearer {access_token}"} + # Exercise the endpoint + data = {"text": "A sufficiently long input to summarize.", "model": "t5-small"} + resp = client.post("/summarize/text", data=data, headers=headers) + assert resp.status_code == 200 + body = resp.json() + assert "summary" in body + assert "key_emotions" in body + assert "compression_ratio" in body
195-201: Unify patching style for voice_transcriber to reduce surprisesSome tests use mock_transcriber.return_value.transcribe, others use mock_transcriber.transcribe. Prefer one consistent approach that matches how unified_ai_api references voice_transcriber (module vs. instance). If it’s a module-level function, patch the attribute and use mock.transcribe directly; if it’s an object instance, use return_value consistently.
Also applies to: 226-236, 298-307, 344-347, 378-381
506-516: Avoid allocating >50MB in tests; simulate size-limit check to keep CI leanConstructing >50MB in memory can slow CI or cause OOM. Prefer:
- Lowering the limit via a test-only config/env override (if supported) and generating a small payload just over that limit.
- Or monkeypatching the size check used by the endpoint.
If you share how the endpoint enforces the max size (constant, config, or filesystem stat), I can propose a concrete patch to simulate the violation without large allocations.
836-843: Timing-based assertion may be flaky in CIAsserting <100ms completion can fail under load. Consider relaxing to a higher threshold (e.g., <500ms) or asserting completion without strict timing.
- # Should complete quickly (less than 100ms) - assert (end_time - start_time) < 0.1 + # Should complete quickly; allow headroom for CI load + assert (end_time - start_time) < 0.5scripts/testing/test_cloud_run_api_endpoints.py (5)
49-64: Health endpoint fields appear too strict vs deployed server responseThe required fields
["status", "service", "version", "emotions_supported"]may not match the server’s health payload (e.g., the Cloud Run server snippet returnsstatus,model_loaded,model_loading,port,timestamp). Recommend validating minimally onstatusand treating the rest as optional.- # Validate expected fields for minimal API - required_fields = ["status", "service", "version", "emotions_supported"] + # Validate minimal health response; extended fields are optional + required_fields = ["status"] + optional_fields = ["service", "version", "emotions_supported"] if missing_fields := [field for field in required_fields if field not in data]: return { "success": False, "error": f"Missing required fields: {missing_fields}", "response": data }Optionally, log which optional fields are present to aid debugging.
171-178: “None payload” test path still sends an empty JSON bodyWhen
test_case is None, the code posts{}instead of testing a truly missing/absent JSON body. If you want to test “no body,” you’ll need a rawrequests.post(url)without thejson=parameter, or updatecreate_api_clientto support a “no JSON” call path.Would you like me to adjust the client to support sending requests without a JSON body for this test case?
259-261: Unused variable: rate_limited
rate_limitedis computed but unused. Either remove it or include it in the returned payload to aid debugging.- # Check if any requests were rate limited (429 status) - rate_limited = any(r.get("status") == "rate_limited" for r in rapid_requests) + # Check if any requests were rate limited (429 status) + rate_limiting_observed = any(r.get("status") == "rate_limited" for r in rapid_requests)And include it in the response:
return { "success": success, "rate_limiting_tested": True, + "rate_limiting_observed": rate_limiting_observed, "security_headers_tested": security_headers.get("tested", False), "note": "Minimal API - basic security features only" }
296-301: Assigned-but-unused variable: dataYou assign
data = self.client.post(...)but never usedata. Remove the assignment to avoid the unused variable.- data = self.client.post("/predict", payload) + self.client.post("/predict", payload)
438-446: Prefer pathlib for directory creation; handle errors consistentlyMinor style/performance: use
Path.mkdir(parents=True, exist_ok=True)overos.makedirs. Also, consider logging the exception vialoggingfor consistency with the rest of the script.- output_file = "test_reports/cloud_run_api_test_results.json" - try: - os.makedirs("test_reports", exist_ok=True) + output_file = "test_reports/cloud_run_api_test_results.json" + try: + from pathlib import Path + Path("test_reports").mkdir(parents=True, exist_ok=True)scripts/testing/test_model_status.py (2)
100-101: Use sys.exit instead of exit
exit()is intended for interactive shells. Usesys.exit()in scripts.- success = test_model_status(args.base_url) - exit(0 if success else 1) + success = test_model_status(args.base_url) + sys.exit(0 if success else 1)Add the import if missing:
import sys
84-91: Summary/return omits model_status_successYou compute and print
model_status_successbut the final return excludes it. If intentional, ignore. Otherwise, include it for stricter coverage.- return health_success and emotions_success and prediction_success + return health_success and emotions_success and model_status_success and prediction_successtests/unit/test_api_rate_limiter.py (1)
79-88: LGTM on middleware attachment testBasic middleware presence check is sufficient here. Optionally, asserting limiter presence on app state can strengthen the test:
hasattr(app.state, 'rate_limiter').scripts/testing/config.py (3)
30-32: Minor readability nit: align multiline env var lookupThe logic is fine; the indentation on Line 31 is over-indented relative to Lines 30 and 32. Consider aligning for readability.
- env_url = (os.environ.get("API_BASE_URL") or - os.environ.get("CLOUD_RUN_API_URL") or - os.environ.get("MODEL_API_BASE_URL")) + env_url = ( + os.environ.get("API_BASE_URL") + or os.environ.get("CLOUD_RUN_API_URL") + or os.environ.get("MODEL_API_BASE_URL") + )
117-123: Enable retries for POST requestsBy default, urllib3’s Retry does not retry POST. Since your client uses POST in
test_prediction/test_batch_prediction, include POST inallowed_methodsso 429/5xx transient failures are retried.- retry_strategy = Retry( - total=3, - backoff_factor=1, - status_forcelist=[429, 500, 502, 503, 504], - ) + retry_strategy = Retry( + total=3, + backoff_factor=1, + status_forcelist=[429, 500, 502, 503, 504], + allowed_methods=frozenset(["HEAD", "GET", "OPTIONS", "POST"]) + )
138-149: Make URL joining robust (avoid double/missing slashes)Current concatenation assumes
endpointstarts with “/”. Join defensively to handle both"/foo"and"foo".- url = f"{self.base_url}{endpoint}" + url = f"{self.base_url.rstrip('/')}/{endpoint.lstrip('/')}" @@ - url = f"{self.base_url}{endpoint}" + url = f"{self.base_url.rstrip('/')}/{endpoint.lstrip('/')}"scripts/training/fixed_focal_training.py (3)
320-324: Save/load the best model inside the created models/ directoryYou create a models/ directory but save/load from the project root. Store the checkpoint under models/ for consistency.
- torch.save(model.state_dict(), "best_focal_model.pth") + torch.save(model.state_dict(), "models/best_focal_model.pth") @@ - trained_model.load_state_dict(torch.load("best_focal_model.pth")) + trained_model.load_state_dict(torch.load("models/best_focal_model.pth"))Also applies to: 415-417
392-399: Optional: set seeds for reproducibilityResults will vary across runs due to shuffling and model initialization. Seed randomness if reproducible training is desired (especially for CI).
Add near device setup:
import os seed = 42 random.seed(seed) np.random.seed(seed) torch.manual_seed(seed) if torch.cuda.is_available(): torch.cuda.manual_seed_all(seed) os.environ["PYTHONHASHSEED"] = str(seed)
61-71: Dataset only covers 8/28 classesYou define 28 emotion slots but provide labeled examples for 8 classes. That’s fine for a minimal demo, but be aware the remaining 20 outputs will learn only negatives and can skew weighted metrics. Consider either:
- adding a few positives per missing class, or
- restricting num_classes and label space to the covered classes for this minimal example.
Also applies to: 188-228
scripts/training/setup_colab_environment.py (5)
138-145: Colab notebook is missing some dependencies you install via the scriptThe install cell omits pandas, numpy, scikit-learn, accelerate, python-dotenv, pytest-cov, pytest-asyncio which you install in
install_dependencies(). Colab may have some preinstalled, but versions can diverge; this can cause runtime mismatches.- "!pip install transformers>=4.30.0,<5.0.0 datasets>=2.10.0,<3.0.0 tokenizers>=0.13.0,<1.0.0\\n", + "!pip install transformers>=4.30.0,<5.0.0 datasets>=2.10.0,<3.0.0 tokenizers>=0.13.0,<1.0.0\\n", + "!pip install pandas>=2.0.0,<3.0.0 numpy>=1.24.0,<2.0.0 scikit-learn>=1.3.0,<2.0.0 accelerate>=0.20.0,<1.0.0 python-dotenv>=1.0.0,<2.0.0\\n", @@ - "!pip install pytest>=7.0.0,<8.0.0 black>=23.0.0,<24.0.0 ruff>=0.1.0,<1.0.0" + "!pip install pytest>=7.0.0,<8.0.0 pytest-cov>=4.0.0,<5.0.0 pytest-asyncio>=0.21.0,<1.0.0 black>=23.0.0,<24.0.0 ruff>=0.1.0,<1.0.0"
227-233: Address linter hint: add explicitchecktosubprocess.runRuff PLW1510: be explicit about
check. You already handle return codes, so make itcheck=False.- result = subprocess.run( + result = subprocess.run( [sys.executable, "scripts/ci/run_full_ci_pipeline.py"], capture_output=True, text=True, - timeout=600 # 10 minute timeout + timeout=600, # 10 minute timeout + check=False )
71-74: Improve pip failure diagnostics by logging stderr/stdoutOn install failure, log captured stdout/stderr to aid debugging rather than only the exception object.
- except subprocess.CalledProcessError as e: - logger.error(f"❌ Failed to install {package}: {e}") + except subprocess.CalledProcessError as e: + logger.error(f"❌ Failed to install {package}: returncode={e.returncode}") + if e.stdout: + logger.error(f"stdout:\\n{e.stdout}") + if e.stderr: + logger.error(f"stderr:\\n{e.stderr}") return False
239-242: On CI failure, also log stdout (not just stderr)This makes troubleshooting easier when tools write useful info to stdout.
- else: - logger.error("❌ CI pipeline verification failed") - logger.error(result.stderr) + else: + logger.error("❌ CI pipeline verification failed") + if result.stdout: + logger.error("stdout:") + logger.error(result.stdout) + if result.stderr: + logger.error("stderr:") + logger.error(result.stderr) return False
20-29: Optional: more robust Colab detection
COLAB_GPUcan be absent until a GPU is attached. A common pattern is to try importinggoogle.colab.def detect_colab_environment(): try: import google.colab # type: ignore logger.info("🎯 Detected Google Colab environment") logger.info(f"📊 Colab GPU: {os.environ.get('COLAB_GPU', 'unknown')}") return True except Exception: logger.info("💻 Running in local environment") return False
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (23)
CHANGELOG.md(1 hunks)scripts/testing/config.py(2 hunks)scripts/testing/test_api_startup.py(0 hunks)scripts/testing/test_cloud_run_api_endpoints.py(17 hunks)scripts/testing/test_e2e_simple.py(0 hunks)scripts/testing/test_model_status.py(2 hunks)scripts/testing/test_vertex_setup.py(2 hunks)scripts/training/fixed_focal_training.py(12 hunks)scripts/training/setup_colab_environment.py(6 hunks)tests/conftest.py(1 hunks)tests/e2e/test_complete_workflows.py(1 hunks)tests/integration/test_priority1_features.py(29 hunks)tests/unit/test_admin_endpoints.py(7 hunks)tests/unit/test_anomaly_detection.py(10 hunks)tests/unit/test_api_rate_limiter.py(2 hunks)tests/unit/test_api_security.py(11 hunks)tests/unit/test_csp_config.py(5 hunks)tests/unit/test_hash_security.py(3 hunks)tests/unit/test_http_exception_handler.py(0 hunks)tests/unit/test_jwt_manager_extra.py(0 hunks)tests/unit/test_sandbox_executor.py(2 hunks)tests/unit/test_secure_model_loader.py(22 hunks)tests/unit/test_validation_enhanced.py(13 hunks)
💤 Files with no reviewable changes (4)
- tests/unit/test_jwt_manager_extra.py
- tests/unit/test_http_exception_handler.py
- scripts/testing/test_api_startup.py
- scripts/testing/test_e2e_simple.py
🧰 Additional context used
🧬 Code Graph Analysis (10)
tests/conftest.py (2)
tests/integration/test_priority1_features.py (1)
reset_state(57-70)src/api_rate_limiter.py (1)
reset_state(436-444)
scripts/testing/config.py (1)
scripts/testing/test_config.py (1)
get(77-91)
tests/unit/test_anomaly_detection.py (2)
src/security_headers.py (3)
SecurityHeadersConfig(21-41)SecurityHeadersMiddleware(43-424)_analyze_user_agent_enhanced(236-335)src/api_rate_limiter.py (3)
_get_client_key(177-180)_analyze_request_patterns(253-287)_detect_abuse(289-333)
tests/unit/test_api_rate_limiter.py (1)
src/api_rate_limiter.py (1)
add_rate_limiting(447-477)
tests/unit/test_sandbox_executor.py (1)
src/models/secure_loader/sandbox_executor.py (4)
_get_safe_builtins(96-107)SandboxExecutor(38-283)execute_safely(164-183)sandbox_context(126-148)
tests/unit/test_api_security.py (2)
src/api_rate_limiter.py (8)
get_stats(396-410)allow_request(347-387)_get_client_key(177-180)_refill_bucket(335-345)add_to_blacklist(412-416)remove_from_blacklist(418-422)add_to_whitelist(424-428)remove_from_whitelist(430-434)src/input_sanitizer.py (5)
sanitize_text(91-134)validate_emotion_request(170-206)validate_batch_request(208-258)validate_content_type(260-277)detect_anomalies(307-339)
scripts/testing/test_cloud_run_api_endpoints.py (4)
tests/integration/test_summarizer_voice_endpoints.py (1)
client(26-28)scripts/testing/config.py (2)
get(138-142)post(144-148)deployment/cloud-run/secure_api_server.py (6)
get(259-280)get(394-405)get(415-424)get(433-447)post(293-329)post(342-387)scripts/testing/test_config.py (4)
get(77-91)post(93-107)create_test_config(110-112)get_rate_limit_requests(64-66)
tests/integration/test_priority1_features.py (3)
tests/integration/test_summarizer_voice_endpoints.py (1)
client(26-28)src/unified_ai_api.py (1)
refresh_token(933-965)src/security/jwt_manager.py (8)
create_access_token(57-67)create_refresh_token(69-80)create_token_pair(82-90)verify_token(92-108)blacklist_token(124-132)has_permission(143-146)is_token_blacklisted(134-136)cleanup_expired_tokens(148-161)
tests/unit/test_hash_security.py (2)
src/api_rate_limiter.py (2)
RateLimitConfig(22-43)_get_client_key(177-180)src/security_headers.py (1)
_before_request(79-92)
tests/unit/test_validation_enhanced.py (1)
src/data/validation.py (4)
check_data_types(52-98)check_text_quality(100-140)validate_journal_entries(142-209)validate_text_input(212-254)
🪛 Ruff (0.12.2)
tests/unit/test_sandbox_executor.py
124-124: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
tests/unit/test_secure_model_loader.py
63-63: os.path.join() should be replaced by Path with / operator
(PTH118)
263-263: os.path.join() should be replaced by Path with / operator
(PTH118)
378-378: os.path.join() should be replaced by Path with / operator
(PTH118)
448-448: os.path.join() should be replaced by Path with / operator
(PTH118)
486-486: os.path.join() should be replaced by Path with / operator
(PTH118)
487-487: os.path.exists() should be replaced by Path.exists()
(PTH110)
490-490: Unnecessary mode argument
Remove mode argument
(UP015)
tests/unit/test_api_security.py
86-86: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
103-103: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
122-122: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
136-136: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
444-444: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
scripts/testing/test_cloud_run_api_endpoints.py
260-260: Local variable rate_limited is assigned to but never used
Remove assignment to unused variable rate_limited
(F841)
297-297: Local variable data is assigned to but never used
Remove assignment to unused variable data
(F841)
439-439: os.makedirs() should be replaced by Path.mkdir(parents=True)
(PTH103)
tests/integration/test_priority1_features.py
90-90: Possible hardcoded password assigned to: "token_type"
(S105)
scripts/testing/test_model_status.py
101-101: Use sys.exit() instead of exit
Replace exit with sys.exit()
(PLR1722)
scripts/training/setup_colab_environment.py
227-227: subprocess.run without explicit check argument
Add explicit check=False
(PLW1510)
🔇 Additional comments (15)
tests/e2e/test_complete_workflows.py (1)
175-178: Whitespace-only tweak looks goodNo functional impact. Keeping the section tight after response_data assignment improves readability.
tests/unit/test_anomaly_detection.py (2)
47-61: UA scoring checks read clean and align with implementation behaviorThe expectations allow for both “legitimate_bot” and “normal,” which matches the documented categorization for low/negative scores.
123-130: Burst pattern detection assertions are reasonableThe test setup and the lower bound on score reflect the heuristic thresholds in _analyze_request_patterns.
tests/unit/test_hash_security.py (1)
150-159: LGTM: Hash algorithm verification is preciseComparing against hashlib.sha256 of the expected input string is correct and guards against regressions in _get_client_key.
tests/unit/test_api_security.py (1)
318-335: Good coverage on deeply nested JSON sanitizer behaviorThe test robustly asserts either a warning about depth or blocked content, which matches resilient expectations for sanitizer implementations.
tests/integration/test_priority1_features.py (1)
56-71: State-reset fixture is solidGood autouse fixture to reset rate limiter and JWT blacklist, and to inject test-only permissions. This reduces test flakiness and cross-test coupling.
tests/unit/test_api_rate_limiter.py (1)
56-64: LGTM on rate-limit exceeded testAssertions align with expected behavior; disabling abuse detection avoids flakiness. No changes needed.
tests/unit/test_validation_enhanced.py (3)
12-24: LGTM: robust fixture matching schemaThe fixture aligns with expected schema used by validation functions, keeping tests deterministic.
93-120: LGTM: comprehensive result structure assertionsGood coverage across
is_valid,validated_df,missing_values, and text quality columns.
198-207: LGTM: invalid type cases handledCovers None and non-string inputs; matches contract in
validate_text_input.tests/unit/test_admin_endpoints.py (3)
24-33: LGTM: conditional import with SkipTest fallbackGraceful handling when the model isn’t available prevents CI flakes.
41-48: LGTM: environment setup/teardown for admin API keyTests isolate side effects by setting and removing
ADMIN_API_KEY.
49-118: LGTM: auth and validation coverage across admin endpointsPositive and negative paths (missing/wrong key, missing IP) are covered well.
scripts/testing/config.py (1)
186-203: LGTM on formatting tweakTrimming the trailing space in the closing brace is harmless and consistent with the rest of the file.
scripts/testing/test_vertex_setup.py (1)
28-35: LGTM: formatting-only changesThe added blank lines improve readability around the logging checkpoints without impacting behavior.
Also applies to: 41-47
…ian bookworm versions; use requirements-api.txt; remove non-existent files
Co-authored-by: denizcan.uelker <denizcan.uelker@mercedes-benz.com>
…used vars and redundant flag; AST-only indentation detection; add backup option and efficient line-by-line whitespace fix; configurable excluded dirs; consistent 4-space indentation\n- check_pgvector: fix mis-indented comment to match PEP8
…to Added/Changed sections\n- testing/config: fix multiline continuation indent for env URL\n- debug script: dedent one-line docstring; suppress T201 on final print\n- rate_limiter: adopt built-in generics for allow_request return type\n- check_pgvector: log DB errors at error level with traceback
…lize docstring; adopt built-in generics; remove redundant 'r' mode for open; include file path in error messages; use extend() for file discovery\n- testing config: simplify env var resolution with walrus op
…erics everywhere; include file path in errors; extend() for discovery\n- check_pgvector: keep lines under limit if practical (no functional change)
…ops; minor IO tweaks\n\n- check_pgvector: use with-statement for conn/cursor; logging.exception; version-agnostic apt hint; configure logging in __main__\n- fix_linting_issues: pathlib/contextlib for temp cleanup; built-in generics already updated; cleaner backup and open usage
…een top-level defs
…analyze_user_agent
Co-authored-by: denizcan.uelker <denizcan.uelker@mercedes-benz.com>
…ants (minimal, unified, secure, production)
…, add environment variable documentation
- ✅ Replace 8 separate Dockerfiles with 1 consolidated Dockerfile.consolidated - ✅ Support for minimal, secure, and unified variants via build args - ✅ All variants tested and working: minimal (Flask), secure, unified (full ML) - ✅ Improved logging for HF model loading fallback (less alarming messages) - ✅ Fixed minimal variant requirements and entrypoint - ✅ Cleaned up old Dockerfiles to reduce maintenance overhead - ✅ Maintains all security features and DOK-DL3008 compliance
…esolve dependency conflicts Resolved issues in src/unified_ai_api.py with DeepSource Autofix
… improve build reliability
… improve logging, and add buildx docs
…tenv from secure requirements
…trailing whitespace
…ized logging for performance
…esolve dependency conflicts Resolved issues in the following files with DeepSource Autofix: 1. src/input_sanitizer.py 2. src/models/secure_loader/__init__.py 3. src/models/secure_loader/integrity_checker.py 4. src/models/secure_loader/model_validator.py 5. src/models/secure_loader/sandbox_executor.py 6. src/models/secure_loader/secure_model_loader.py 7. src/monitoring/dashboard.py 8. src/security_headers.py 9. src/security/jwt_manager.py
…ersions for consistency
…SAMO--DL into fix/docker-build-issues
…ion in unified_ai_api.py
Fix Docker builds: remove architecture-specific package pinning and resolve dependency conflicts
… for edge cases and missing scenarios
There was a problem hiding this comment.
Pull Request Overview
This PR refreshes the core test suite to ensure consistent CI feedback and introduces minimal training helper scripts. The changes focus on improving test coverage, removing duplicate code, and adding basic training utilities for the project.
Key changes:
- Enhanced test coverage with new test cases for edge cases and boundary conditions
- Cleaned up whitespace and removed empty files in test directories
- Added a Tests & Training section to the changelog documenting these improvements
Reviewed Changes
Copilot reviewed 10 out of 23 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_validation_enhanced.py | Added comprehensive test for missing values validation across all columns |
| tests/unit/test_jwt_manager_extra.py | Removed trailing whitespace |
| tests/unit/test_http_exception_handler.py | Removed trailing whitespace |
| tests/unit/test_api_security.py | Added tests for multiple user agents and abuse detection cooldown scenarios |
| tests/unit/test_api_rate_limiter.py | Added edge case tests for zero and negative rate limits |
| tests/integration/test_priority1_features.py | Added tests for empty batch transcription and file size limit validation |
| scripts/testing/test_cloud_run_api_endpoints.py | Added large payload testing functionality |
| scripts/testing/test_e2e_simple.py | Removed empty file |
| scripts/testing/test_api_startup.py | Removed empty file |
| CHANGELOG.md | Added Tests & Training section documenting the refresh |
Comments suppressed due to low confidence (1)
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| response = client.post("/transcribe/voice", files=files, data=data, headers=headers) | ||
|
|
||
| # Should accept files at the exact limit | ||
| assert response.status_code in [200, 202], f"Unexpected status code: {response.status_code}" |
There was a problem hiding this comment.
Creating a 50MB byte string in memory could cause memory issues in test environments. Consider using a mock file or streaming approach instead of allocating large amounts of memory.
| assert response.status_code in [200, 202], f"Unexpected status code: {response.status_code}" | |
| # Create a dummy audio file exactly at the limit using a temporary file | |
| with tempfile.NamedTemporaryFile(suffix=".wav") as tmp_file: | |
| tmp_file.seek(FILE_SIZE_LIMIT - 1) | |
| tmp_file.write(b"\0") | |
| tmp_file.flush() | |
| tmp_file.seek(0) | |
| files = {"audio_file": ("test_limit.wav", tmp_file, "audio/wav")} | |
| headers = {"Authorization": f"Bearer {access_token}"} | |
| data = {"language": "en", "model_size": "base"} | |
| response = client.post("/transcribe/voice", files=files, data=data, headers=headers) | |
| # Should accept files at the exact limit | |
| assert response.status_code in [200, 202], f"Unexpected status code: {response.status_code}" |
|
|
||
| large_payload_tests = [ | ||
| {"text": large_text, "description": "1MB text payload"}, | ||
| {"text": extremely_large_text, "description": "10MB text payload"} |
There was a problem hiding this comment.
Creating large text strings in memory (1MB and 10MB) could cause memory pressure during testing. Consider using generators or mock data instead of allocating large strings.
| {"text": extremely_large_text, "description": "10MB text payload"} | |
| # Helper to generate large text payloads without allocating them in memory | |
| def large_text_generator(char, size): | |
| for _ in range(size): | |
| yield char | |
| def get_large_text_stream(char, size): | |
| # Returns a file-like object for streaming text | |
| return io.StringIO(''.join(large_text_generator(char, size))) | |
| large_payload_tests = [ | |
| {"text_stream": get_large_text_stream("A", 1024 * 1024), "description": "1MB text payload"}, | |
| {"text_stream": get_large_text_stream("B", 10 * 1024 * 1024), "description": "10MB text payload"} |
|
|
||
| # Test with very large text payload | ||
| large_text = "A" * (1024 * 1024) # 1MB text | ||
| extremely_large_text = "B" * (10 * 1024 * 1024) # 10MB text |
There was a problem hiding this comment.
Creating a 10MB text string in memory could cause significant memory usage. Consider using a more memory-efficient approach for testing large payloads.
| extremely_large_text = "B" * (10 * 1024 * 1024) # 10MB text | |
| # Use a temporary file to avoid creating a 10MB string in memory | |
| temp_10mb_file = tempfile.NamedTemporaryFile(mode='w+', delete=False) | |
| temp_10mb_file.write("B" * (10 * 1024 * 1024)) # Write 10MB of 'B' to file | |
| temp_10mb_file.flush() | |
| temp_10mb_file.seek(0) | |
| # Read the file in chunks to avoid loading all into memory | |
| def read_10mb_file_in_chunks(file_obj, chunk_size=1024*1024): | |
| file_obj.seek(0) | |
| chunks = [] | |
| while True: | |
| data = file_obj.read(chunk_size) | |
| if not data: | |
| break | |
| chunks.append(data) | |
| return ''.join(chunks) | |
| extremely_large_text = read_10mb_file_in_chunks(temp_10mb_file) | |
| temp_10mb_file.close() | |
| os.unlink(temp_10mb_file.name) |
…d resolve import issues
…g flags for optimal GPU performance
… for edge cases and missing scenarios
…d resolve import issues
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
tests/unit/test_validation_enhanced.py (1)
36-55: Good coverage for “all columns have missing values” (addresses prior feedback).This test solidly validates behavior when every column contains missing values and closes out the earlier request to add such coverage.
tests/unit/test_api_rate_limiter.py (1)
75-100: Edge-case coverage for zero/negative rate limits looks good.Both tests align with the current allow_request behavior (bucket remains at/below threshold; reason contains "Rate limit"). This addresses prior suggestions to cover these cases.
tests/integration/test_priority1_features.py (1)
237-260: Solid negative test for empty batch input (matches earlier request).Asserts both status and a meaningful error message. Nice use of a dedicated permission header for the batch endpoint.
🧹 Nitpick comments (5)
tests/unit/test_validation_enhanced.py (2)
56-61: Use tolerant float comparisons for missing-value percentages.Direct float equality can be brittle depending on how percentages are computed. Prefer pytest.approx or math.isclose with a small tolerance.
Apply this diff for clearer, robust comparisons:
- assert missing_stats['user_id'] == 0.4 # 2 out of 5 missing (40%) - assert missing_stats['title'] == 0.4 # 2 out of 5 missing (40%) - assert missing_stats['content'] == 0.6 # 3 out of 5 missing (60%) - assert missing_stats['created_at'] == 0.4 # 2 out of 5 missing (40%) - assert missing_stats['is_private'] == 0.4 # 2 out of 5 missing (40%) + import math + assert math.isclose(missing_stats['user_id'], 0.4, rel_tol=1e-9, abs_tol=1e-9) + assert math.isclose(missing_stats['title'], 0.4, rel_tol=1e-9, abs_tol=1e-9) + assert math.isclose(missing_stats['content'], 0.6, rel_tol=1e-9, abs_tol=1e-9) + assert math.isclose(missing_stats['created_at'], 0.4, rel_tol=1e-9, abs_tol=1e-9) + assert math.isclose(missing_stats['is_private'], 0.4, rel_tol=1e-9, abs_tol=1e-9)
137-139: Remove redundant local import of pandas inside the test.pd is already imported at module level; re-importing within the test is unnecessary.
Apply this diff:
- import pandas as pd assert isinstance(results['validated_df'], pd.DataFrame)tests/unit/test_api_rate_limiter.py (1)
75-100: Optional: Parametrize zero/negative rate limit tests to reduce duplication.Both tests follow the same structure and can be consolidated with parametrization for clarity.
Apply this refactor:
+import pytest @@ - def test_allow_request_zero_rate_limits(self): - """Test that allow_request returns False when rate limits are zero.""" - config = RateLimitConfig( - requests_per_minute=0, - burst_size=0, - enable_user_agent_analysis=False, - enable_request_pattern_analysis=False - ) - rate_limiter = TokenBucketRateLimiter(config) - allowed, reason, _ = rate_limiter.allow_request("127.0.0.1") - assert allowed is False - assert "rate limit" in reason.lower() - - def test_allow_request_negative_rate_limits(self): - """Test that allow_request returns False when rate limits are negative.""" - config = RateLimitConfig( - requests_per_minute=-1, - burst_size=-5, - enable_user_agent_analysis=False, - enable_request_pattern_analysis=False - ) - rate_limiter = TokenBucketRateLimiter(config) - allowed, reason, _ = rate_limiter.allow_request("127.0.0.1") - assert allowed is False - assert "rate limit" in reason.lower() + @pytest.mark.parametrize( + "requests_per_minute,burst_size", + [(0, 0), (-1, -5)], + ) + def test_allow_request_invalid_rate_limits(self, requests_per_minute, burst_size): + """Test that allow_request returns False when rate limits are zero or negative.""" + config = RateLimitConfig( + requests_per_minute=requests_per_minute, + burst_size=burst_size, + enable_user_agent_analysis=False, + enable_request_pattern_analysis=False, + ) + rate_limiter = TokenBucketRateLimiter(config) + allowed, reason, _ = rate_limiter.allow_request("127.0.0.1") + assert allowed is False + assert "rate limit" in reason.lower()tests/unit/test_api_security.py (1)
168-170: Rename unused loop variable to underscore for clarity and lint compliance.Silences Ruff B007 and improves readability where the index isn’t used.
Apply this diff:
- for i in range(11): # More than 10 requests in 1 second + for _ in range(11): # More than 10 requests in 1 second @@ - for i in range(5): + for _ in range(5): @@ - for i in range(15): # Trigger abuse detection + for _ in range(15): # Trigger abuse detectionAlso applies to: 190-194, 498-500
scripts/testing/test_cloud_run_api_endpoints.py (1)
217-265: Large-payload test is valuable; align annotations and result capture.
- Prefer built-in generics: dict[str, Any] over Dict[str, Any].
- Your client returns parsed JSON (not a Response), so status_code will be "N/A". If you need status codes, consider using the lower-level client from scripts/testing/config.py for this test only.
Apply this diff:
- def test_extremely_large_payloads(self) -> Dict[str, Any]: + def test_extremely_large_payloads(self) -> dict[str, Any]: @@ - response = self.client.post("/predict", {"text": test_case["text"]}) + response = self.client.post("/predict", {"text": test_case["text"]}) @@ - "status_code": response.status_code if hasattr(response, 'status_code') else 'N/A' + "status_code": response.status_code if hasattr(response, 'status_code') else "N/A"If you want to capture HTTP status codes, I can help refactor this method to temporarily use scripts/testing/config.py’s client for this test. Let me know.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
scripts/testing/test_cloud_run_api_endpoints.py(17 hunks)tests/integration/test_priority1_features.py(28 hunks)tests/unit/test_api_rate_limiter.py(2 hunks)tests/unit/test_api_security.py(11 hunks)tests/unit/test_validation_enhanced.py(12 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
tests/integration/test_priority1_features.py (1)
tests/integration/test_summarizer_voice_endpoints.py (1)
client(26-28)
scripts/testing/test_cloud_run_api_endpoints.py (3)
scripts/testing/config.py (2)
get(138-142)post(144-148)deployment/cloud-run/secure_api_server.py (6)
get(259-280)get(394-405)get(415-424)get(433-447)post(293-329)post(342-387)scripts/testing/test_config.py (4)
get(77-91)post(93-107)create_test_config(110-112)get_rate_limit_requests(64-66)
tests/unit/test_api_security.py (4)
src/api_rate_limiter.py (9)
get_stats(396-410)allow_request(347-387)release_request(389-394)_get_client_key(177-180)_refill_bucket(335-345)add_to_blacklist(412-416)remove_from_blacklist(418-422)add_to_whitelist(424-428)remove_from_whitelist(430-434)deployment/secure_api_server.py (2)
add_to_blacklist(604-617)add_to_whitelist(621-634)src/input_sanitizer.py (5)
sanitize_text(91-134)validate_emotion_request(170-206)validate_batch_request(208-258)validate_content_type(260-277)detect_anomalies(307-339)src/security_headers.py (1)
_build_permissions_policy(169-200)
🪛 Ruff (0.12.2)
tests/integration/test_priority1_features.py
90-90: Possible hardcoded password assigned to: "token_type"
(S105)
508-508: Do not assert blind exception: Exception
(B017)
scripts/testing/test_cloud_run_api_endpoints.py
217-217: Use dict instead of Dict for type annotation
Replace with dict
(UP006)
267-267: Use dict instead of Dict for type annotation
Replace with dict
(UP006)
310-310: Local variable rate_limited is assigned to but never used
Remove assignment to unused variable rate_limited
(F841)
347-347: Local variable data is assigned to but never used
Remove assignment to unused variable data
(F841)
489-489: os.makedirs() should be replaced by Path.mkdir(parents=True)
(PTH103)
tests/unit/test_api_security.py
118-118: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
135-135: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
154-154: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
168-168: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
190-190: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
498-498: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (python)
🔇 Additional comments (1)
tests/unit/test_api_security.py (1)
64-95: Per-UA bucketing verification is on-point.Validates separate client keys per (IP, UA) and mirrors _get_client_key logic. Cleanly releases concurrency after each request.
| def test_websocket_authentication_required(self): | ||
| """Test that WebSocket requires authentication.""" | ||
| # This would require a WebSocket client test | ||
| # For now, we'll test the authentication logic | ||
| pass | ||
|
|
||
| # Test that WebSocket endpoint requires authentication | ||
| with pytest.raises(Exception): # WebSocket connection should fail without auth | ||
| # This simulates the authentication requirement | ||
| pass | ||
| # Mark as implemented but requires WebSocket client for full testing | ||
| assert True | ||
|
|
There was a problem hiding this comment.
Placeholder WebSocket test fails: using bare Exception with no raise causes DID NOT RAISE.
This test will fail at collection/execution and break CI. Either implement a proper WebSocket test or skip/xfail it until a client is available.
Apply one of these diffs:
Option A — skip with reason:
def test_websocket_authentication_required(self):
"""Test that WebSocket requires authentication."""
- # Test that WebSocket endpoint requires authentication
- with pytest.raises(Exception): # WebSocket connection should fail without auth
- # This simulates the authentication requirement
- pass
- # Mark as implemented but requires WebSocket client for full testing
- assert True
+ pytest.skip("WebSocket auth test requires a WebSocket client; skipping for now.")Option B — mark xfail:
+ @pytest.mark.xfail(reason="WebSocket client not available; implement when ready")
def test_websocket_authentication_required(self):
"""Test that WebSocket requires authentication."""
- with pytest.raises(Exception):
- pass
- assert True
+ # Placeholder: will be implemented with a WebSocket client
+ assert True📝 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.
| def test_websocket_authentication_required(self): | |
| """Test that WebSocket requires authentication.""" | |
| # This would require a WebSocket client test | |
| # For now, we'll test the authentication logic | |
| pass | |
| # Test that WebSocket endpoint requires authentication | |
| with pytest.raises(Exception): # WebSocket connection should fail without auth | |
| # This simulates the authentication requirement | |
| pass | |
| # Mark as implemented but requires WebSocket client for full testing | |
| assert True | |
| def test_websocket_authentication_required(self): | |
| """Test that WebSocket requires authentication.""" | |
| pytest.skip("WebSocket auth test requires a WebSocket client; skipping for now.") |
| def test_websocket_authentication_required(self): | |
| """Test that WebSocket requires authentication.""" | |
| # This would require a WebSocket client test | |
| # For now, we'll test the authentication logic | |
| pass | |
| # Test that WebSocket endpoint requires authentication | |
| with pytest.raises(Exception): # WebSocket connection should fail without auth | |
| # This simulates the authentication requirement | |
| pass | |
| # Mark as implemented but requires WebSocket client for full testing | |
| assert True | |
| @pytest.mark.xfail(reason="WebSocket client not available; implement when ready") | |
| def test_websocket_authentication_required(self): | |
| """Test that WebSocket requires authentication.""" | |
| # Placeholder: will be implemented with a WebSocket client | |
| assert True |
🧰 Tools
🪛 Ruff (0.12.2)
508-508: Do not assert blind exception: Exception
(B017)
🤖 Prompt for AI Agents
In tests/integration/test_priority1_features.py around lines 505 to 513 the
test_websocket_authentication_required() currently uses a with
pytest.raises(Exception): block that contains no raise, causing a "DID NOT
RAISE" failure; fix by either skipping or xfail-ing the placeholder test: Option
A - replace the body with pytest.skip("WebSocket client not available; skipping
until a real client is provided") so the test is skipped at runtime, or Option B
- mark the test with @pytest.mark.xfail(reason="WebSocket client not available",
strict=False) and simplify the body to a noop/assert True so it is reported as
an expected failure rather than breaking CI.
| def test_voice_transcription_file_size_at_limit(self): | ||
| """Test file size exactly at the limit for voice transcription.""" | ||
| # Login to get token | ||
| login_data = { | ||
| "username": "testuser@example.com", | ||
| "password": "testpassword123" | ||
| } | ||
| login_response = client.post("/auth/login", json=login_data) | ||
| access_token = login_response.json()["access_token"] | ||
|
|
||
| # Assume the file size limit is 50MB (adjust if different) | ||
| FILE_SIZE_LIMIT = 50 * 1024 * 1024 # 50MB in bytes | ||
|
|
||
| # Create a dummy audio file exactly at the limit | ||
| audio_content = b"\0" * FILE_SIZE_LIMIT | ||
| files = {"audio_file": ("test_limit.wav", audio_content, "audio/wav")} | ||
| headers = {"Authorization": f"Bearer {access_token}"} | ||
| data = {"language": "en", "model_size": "base"} | ||
|
|
||
| response = client.post("/transcribe/voice", files=files, data=data, headers=headers) | ||
|
|
||
| # Should accept files at the exact limit | ||
| assert response.status_code in [200, 202], f"Unexpected status code: {response.status_code}" | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm the file-size limit matches the implementation before locking the assertion.
The test assumes a 50MB limit. If the actual limit differs, this can cause flakes. Verify the enforced limit in the API and align the test accordingly.
Run this script to locate the effective file-size limit and the /transcribe/voice handler:
🏁 Script executed:
#!/bin/bash
# Find constants or checks that imply upload/file size limits
rg -n -C2 -i 'file.?size|upload|limit|max[_- ]?size|50.?mb|50\s*\*\s*1024\s*\*\s*1024|content-length' --type py
# Inspect the voice transcription endpoint(s)
rg -n -C3 -i 'transcribe.*voice|/transcribe/voice' --type pyLength of output: 8524
🏁 Script executed:
#!/bin/bash
# Locate how large_content is defined in the integration tests
rg -n 'large_content' tests/integration/test_priority1_features.py
# Show the context around the test that references large_content and the size limit
sed -n '530,590p' tests/integration/test_priority1_features.pyLength of output: 3013
#!/bin/bash
# Extract the transcribe_voice handler implementation
sed -n '1350,1450p' src/unified_ai_api.py🏁 Script executed:
#!/bin/bash
# Extract the voice transcription endpoint implementation to find file size validation
sed -n '1350,1450p' src/unified_ai_api.py
# Search for the "too large" error to locate the limit enforcement
rg -n -C2 -i "too large" src/unified_ai_api.pyLength of output: 5105
Align test’s file-size assumption with the implementation’s 45 MB limit
The voice‐transcription handler enforces
MAX_AUDIO_BYTES = 45 * 1024 * 1024 # 45 MBbut the test hard‐codes a 50 MB limit. Update the integration test to match the real threshold, for example:
-
In tests/integration/test_priority1_features.py (around line 545), replace
# Assume the file size limit is 50MB (adjust if different) FILE_SIZE_LIMIT = 50 * 1024 * 1024 # 50MB in bytes
with either:
from src.unified_ai_api import MAX_AUDIO_BYTES FILE_SIZE_LIMIT = MAX_AUDIO_BYTES
or
FILE_SIZE_LIMIT = 45 * 1024 * 1024 # 45MB in bytes
-
Remove or update the accompanying comment.
This ensures the “exact‐limit” test actually passes against the 45 MB enforcement in the code.
🤖 Prompt for AI Agents
In tests/integration/test_priority1_features.py around lines 545 to 568, the
test assumes a 50 MB audio limit but the application enforces MAX_AUDIO_BYTES =
45 * 1024 * 1024; update the test to match the implementation by replacing the
hard-coded 50MB value and comment with either an import of the canonical
constant (e.g., from src.unified_ai_api import MAX_AUDIO_BYTES and set
FILE_SIZE_LIMIT = MAX_AUDIO_BYTES) or by changing it to FILE_SIZE_LIMIT = 45 *
1024 * 1024 and update the comment accordingly so the “exact-limit” test aligns
with the real 45 MB threshold.
| def test_abuse_detection_reset_after_cooldown(self): | ||
| """Test that abuse detection resets after cooldown period.""" | ||
| client_ip = "192.168.1.6" | ||
| user_agent = "test-agent" | ||
|
|
||
| # Simulate rapid-fire requests to trigger abuse detection | ||
| for i in range(11): # More than 10 requests in 1 second | ||
| self.rate_limiter.request_history[self.rate_limiter._get_client_key(client_ip, user_agent)].append(time.time()) | ||
|
|
||
| # Verify abuse detection is triggered | ||
| allowed, reason, meta = self.rate_limiter.allow_request(client_ip, user_agent) | ||
| self.assertFalse(allowed) | ||
| self.assertEqual(reason, "Abuse detected") | ||
|
|
||
| # Simulate cooldown period (e.g., 60 seconds) by clearing request history | ||
| client_key = self.rate_limiter._get_client_key(client_ip, user_agent) | ||
| self.rate_limiter.request_history[client_key] = [] | ||
|
|
||
| # After cooldown, requests should be allowed again | ||
| allowed, reason, meta = self.rate_limiter.allow_request(client_ip, user_agent) | ||
| self.assertTrue(allowed, f"Request should be allowed after cooldown, but got: {reason}") | ||
|
|
There was a problem hiding this comment.
Test erroneously allows requests while the client remains blocked (cooldown not simulated).
After "Abuse detected", the client is added to blocked_clients for block_duration_seconds. Clearing request_history alone does not unblock the client; allow_request checks _is_client_blocked first and will keep rejecting during the block window.
Fix by expiring or removing the block before re-trying:
def test_abuse_detection_reset_after_cooldown(self):
"""Test that abuse detection resets after cooldown period."""
client_ip = "192.168.1.6"
user_agent = "test-agent"
@@
- # Simulate cooldown period (e.g., 60 seconds) by clearing request history
- client_key = self.rate_limiter._get_client_key(client_ip, user_agent)
- self.rate_limiter.request_history[client_key] = []
-
- # After cooldown, requests should be allowed again
- allowed, reason, meta = self.rate_limiter.allow_request(client_ip, user_agent)
+ # Simulate cooldown by clearing history AND removing/expiring the block
+ client_key = self.rate_limiter._get_client_key(client_ip, user_agent)
+ self.rate_limiter.request_history[client_key] = []
+ # Unblock the client explicitly for the test (no waiting)
+ self.rate_limiter.blocked_clients.pop(client_key, None)
+
+ # After cooldown/unblock, requests should be allowed again
+ allowed, reason, meta = self.rate_limiter.allow_request(client_ip, user_agent)
self.assertTrue(allowed, f"Request should be allowed after cooldown, but got: {reason}")Alternative: Patch time to simulate the block_duration elapsing:
from unittest.mock import patch
now = time.time()
with patch('api_rate_limiter.time.time', return_value=now + self.config.block_duration_seconds + 1):
allowed, reason, meta = self.rate_limiter.allow_request(client_ip, user_agent)
self.assertTrue(allowed)🧰 Tools
🪛 Ruff (0.12.2)
168-168: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
🤖 Prompt for AI Agents
In tests/unit/test_api_security.py around lines 162-183, the test clears
request_history but does not remove/expire the client from
rate_limiter.blocked_clients so allow_request still rejects; fix by either
removing the client key from rate_limiter.blocked_clients (e.g., pop the client
key) before the second allow_request, or simulate the cooldown elapsing by
patching time.time to return now + self.config.block_duration_seconds + 1 when
calling allow_request so the block is considered expired.
Summary by Sourcery
Refresh core tests for consistent CI feedback, introduce minimal training helper scripts and Colab setup, and update the changelog accordingly
New Features:
Enhancements:
Documentation:
Summary by CodeRabbit
New Features
Documentation
Tests