Add real app tests and standardize test discovery#95
Conversation
Reviewer's GuideStandardizes Python unittest discovery pattern in the Makefile and replaces a placeholder test with real application-level tests for the Flask app routes and allowed_file helper. Sequence diagram for standardized unittest discovery via make testsequenceDiagram
actor Developer
participant Makefile
participant Python
participant unittest
participant tests_test_app
Developer->>Makefile: invoke make test
Makefile->>Python: run python -m unittest discover -s tests -p test*.py -v
Python->>unittest: initialize discovery
unittest->>unittest: discover tests matching pattern test*.py in tests
unittest-->>tests_test_app: load test cases
unittest->>tests_test_app: execute tests
tests_test_app-->>unittest: report results
unittest-->>Python: aggregate exit code
Python-->>Makefile: return exit code
Makefile-->>Developer: display test results
Sequence diagram for Flask app route and helper testing in test_appsequenceDiagram
participant unittest
participant TestAppRoutes
participant TestAllowedFile
participant FlaskApp
participant allowed_file
unittest->>TestAppRoutes: run setUp
TestAppRoutes->>FlaskApp: create test_client
unittest->>TestAppRoutes: run route tests
TestAppRoutes->>FlaskApp: client.get or client.post to routes
FlaskApp-->>TestAppRoutes: return response
TestAppRoutes-->>unittest: assert response status and body
unittest->>TestAllowedFile: run helper tests
TestAllowedFile->>allowed_file: call with sample filenames
allowed_file-->>TestAllowedFile: return boolean result
TestAllowedFile-->>unittest: assert expected boolean values
unittest-->>unittest: aggregate test results and exit code
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary of ChangesHello, 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 significantly enhances the project's test coverage by introducing dedicated tests for the Flask application's routes and a critical file utility function. Concurrently, it refines the test execution process by standardizing the test discovery pattern in the 'Makefile', ensuring that only relevant test files are identified and run. These changes improve the reliability of the application and streamline the testing workflow. Highlights
Changelog
Activity
Using Gemini Code AssistThe 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 by creating a comment using either
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 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
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tests/test_app.py" line_range="15-17" />
<code_context>
+ def setUp(self):
+ self.client = flask_app.test_client()
+
+ def test_root_endpoint_returns_success(self):
+ response = self.client.get('/')
+ self.assertEqual(response.status_code, 200)
+
+ def test_health_endpoint_contract(self):
</code_context>
<issue_to_address>
**suggestion (testing):** Consider asserting on the response payload for `/` to prove the route behavior, not just the status code.
This test only verifies that `/` returns HTTP 200. To fully validate the route, also assert on the response body (e.g., via `response.get_data(as_text=True)` or `response.get_json()`) to confirm it returns the expected content or structure, not just a success code.
```suggestion
def test_root_endpoint_returns_success(self):
response = self.client.get('/')
self.assertEqual(response.status_code, 200)
body = response.get_data(as_text=True)
self.assertIsInstance(body, str)
self.assertNotEqual(body.strip(), "")
```
</issue_to_address>
### Comment 2
<location path="tests/test_app.py" line_range="19-28" />
<code_context>
+ response = self.client.get('/')
+ self.assertEqual(response.status_code, 200)
+
+ def test_health_endpoint_contract(self):
+ response = self.client.get('/health')
+ self.assertIn(response.status_code, (200, 503))
+
+ payload = response.get_json()
+ self.assertIsInstance(payload, dict)
+ self.assertEqual(payload.get('service'), 'smart-google')
+ self.assertIsInstance(payload.get('mqtt_connected'), bool)
+ self.assertIn(payload.get('status'), ('ok', 'degraded'))
+
+ if response.status_code == 200:
+ self.assertEqual(payload.get('status'), 'ok')
+ else:
+ self.assertEqual(payload.get('status'), 'degraded')
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen the `/health` test by validating response headers and JSON shape more explicitly.
To further lock down the contract and detect regressions earlier, you could also assert that:
- The response has a JSON content type (e.g. `self.assertEqual(response.content_type, 'application/json')` or starts with `application/json`).
- Field types are as expected beyond `mqtt_connected` (e.g. `service` is a non-empty string).
These additions make the health probe more robust for external systems and tooling.
Suggested implementation:
```python
def test_root_endpoint_returns_success(self):
response = self.client.get('/')
self.assertEqual(response.status_code, 200)
def test_health_endpoint_contract(self):
response = self.client.get('/health')
self.assertIn(response.status_code, (200, 503))
# Content type / headers
self.assertTrue(
response.content_type.startswith("application/json"),
msg=f"Unexpected content type: {response.content_type}",
)
payload = response.get_json()
self.assertIsInstance(payload, dict)
# service field: non-empty string
self.assertIn("service", payload)
self.assertIsInstance(payload["service"], str)
self.assertTrue(payload["service"])
self.assertEqual(payload["service"], "smart-google")
# mqtt_connected field: boolean
self.assertIn("mqtt_connected", payload)
self.assertIsInstance(payload["mqtt_connected"], bool)
# status field: constrained string
self.assertIn("status", payload)
self.assertIsInstance(payload["status"], str)
self.assertIn(payload["status"], ("ok", "degraded"))
if response.status_code == 200:
self.assertEqual(payload["status"], "ok")
else:
self.assertEqual(payload["status"], "degraded")
```
If the existing `test_health_endpoint_contract` method is already present in `tests/test_app.py`, replace its body with the updated version above rather than creating a duplicate method. Adjust the indentation to match the surrounding class definition if it differs from the snippet shown.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| def test_root_endpoint_returns_success(self): | ||
| response = self.client.get('/') | ||
| self.assertEqual(response.status_code, 200) |
There was a problem hiding this comment.
suggestion (testing): Consider asserting on the response payload for / to prove the route behavior, not just the status code.
This test only verifies that / returns HTTP 200. To fully validate the route, also assert on the response body (e.g., via response.get_data(as_text=True) or response.get_json()) to confirm it returns the expected content or structure, not just a success code.
| def test_root_endpoint_returns_success(self): | |
| response = self.client.get('/') | |
| self.assertEqual(response.status_code, 200) | |
| def test_root_endpoint_returns_success(self): | |
| response = self.client.get('/') | |
| self.assertEqual(response.status_code, 200) | |
| body = response.get_data(as_text=True) | |
| self.assertIsInstance(body, str) | |
| self.assertNotEqual(body.strip(), "") |
| def test_health_endpoint_contract(self): | ||
| response = self.client.get('/health') | ||
| self.assertIn(response.status_code, (200, 503)) | ||
|
|
||
| payload = response.get_json() | ||
| self.assertIsInstance(payload, dict) | ||
| self.assertEqual(payload.get('service'), 'smart-google') | ||
| self.assertIsInstance(payload.get('mqtt_connected'), bool) | ||
| self.assertIn(payload.get('status'), ('ok', 'degraded')) | ||
|
|
There was a problem hiding this comment.
suggestion (testing): Strengthen the /health test by validating response headers and JSON shape more explicitly.
To further lock down the contract and detect regressions earlier, you could also assert that:
- The response has a JSON content type (e.g.
self.assertEqual(response.content_type, 'application/json')or starts withapplication/json). - Field types are as expected beyond
mqtt_connected(e.g.serviceis a non-empty string).
These additions make the health probe more robust for external systems and tooling.
Suggested implementation:
def test_root_endpoint_returns_success(self):
response = self.client.get('/')
self.assertEqual(response.status_code, 200)
def test_health_endpoint_contract(self):
response = self.client.get('/health')
self.assertIn(response.status_code, (200, 503))
# Content type / headers
self.assertTrue(
response.content_type.startswith("application/json"),
msg=f"Unexpected content type: {response.content_type}",
)
payload = response.get_json()
self.assertIsInstance(payload, dict)
# service field: non-empty string
self.assertIn("service", payload)
self.assertIsInstance(payload["service"], str)
self.assertTrue(payload["service"])
self.assertEqual(payload["service"], "smart-google")
# mqtt_connected field: boolean
self.assertIn("mqtt_connected", payload)
self.assertIsInstance(payload["mqtt_connected"], bool)
# status field: constrained string
self.assertIn("status", payload)
self.assertIsInstance(payload["status"], str)
self.assertIn(payload["status"], ("ok", "degraded"))
if response.status_code == 200:
self.assertEqual(payload["status"], "ok")
else:
self.assertEqual(payload["status"], "degraded")If the existing test_health_endpoint_contract method is already present in tests/test_app.py, replace its body with the updated version above rather than creating a duplicate method. Adjust the indentation to match the surrounding class definition if it differs from the snippet shown.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces valuable application-level tests for the Flask app and standardizes the test discovery process, which are great improvements. The new tests for the allowed_file helper are comprehensive.
My review focuses on the new application route tests in tests/test_app.py. I've identified a couple of areas for improvement to make the tests more robust and reliable. Specifically, the tests for the / and /health endpoints don't account for the FULL_FEATURES flag in app.py, which can lead to tests failing in environments where optional dependencies are not installed. I've also suggested a way to make the assertions in the health check test clearer and safer.
Overall, this is a solid contribution, and with a few adjustments to the tests, it will be even better.
| def test_health_endpoint_contract(self): | ||
| response = self.client.get('/health') | ||
| self.assertIn(response.status_code, (200, 503)) | ||
|
|
||
| payload = response.get_json() | ||
| self.assertIsInstance(payload, dict) | ||
| self.assertEqual(payload.get('service'), 'smart-google') | ||
| self.assertIsInstance(payload.get('mqtt_connected'), bool) | ||
| self.assertIn(payload.get('status'), ('ok', 'degraded')) | ||
|
|
||
| if response.status_code == 200: | ||
| self.assertEqual(payload.get('status'), 'ok') | ||
| else: | ||
| self.assertEqual(payload.get('status'), 'degraded') |
There was a problem hiding this comment.
The test for the /health endpoint seems to assume that the application is running with FULL_FEATURES=True. However, if FULL_FEATURES is False (e.g., due to missing optional dependencies), a fallback /health endpoint is registered in app.py which returns a different payload ({'status': 'healthy', 'features': 'basic'}). This will cause this test to fail because it expects keys like service and mqtt_connected which are not present in the fallback response.
Tests should be reliable and produce consistent results. Please consider skipping this test if FULL_FEATURES is False. You can import FULL_FEATURES from app and use self.skipTest() if it's False at the beginning of the test method.
| def test_root_endpoint_returns_success(self): | ||
| response = self.client.get('/') | ||
| self.assertEqual(response.status_code, 200) |
There was a problem hiding this comment.
This test only verifies the status code. The response body of the root endpoint / differs depending on the FULL_FEATURES flag in app.py. To make this test more robust and ensure it's testing the intended behavior, consider adding an assertion to check the response content.
For example, if you intend to test the fallback route (when FULL_FEATURES is False), you could add:
data = response.get_json()
self.assertEqual(data['status'], 'Smart-Google is working!')This will make the test more explicit about what it's verifying.
| self.assertEqual(payload.get('service'), 'smart-google') | ||
| self.assertIsInstance(payload.get('mqtt_connected'), bool) | ||
| self.assertIn(payload.get('status'), ('ok', 'degraded')) |
There was a problem hiding this comment.
Using payload.get('key') in assertions can lead to less clear failure messages if the key is missing (e.g., AssertionError: None != 'smart-google'). It's better practice to first check for the key's existence or use dictionary access payload['key'] which would raise a KeyError with a clear message. This makes it explicit that the presence of these keys is part of the contract being tested.
self.assertIn('service', payload)
self.assertEqual(payload['service'], 'smart-google')
self.assertIn('mqtt_connected', payload)
self.assertIsInstance(payload['mqtt_connected'], bool)
self.assertIn('status', payload)
self.assertIn(payload['status'], ('ok', 'degraded'))
@copilot |
Co-authored-by: DaTiC0 <13198638+DaTiC0@users.noreply.github.com>
Strengthen health contract tests and standardize fallback health endpoint
…itional assertions based on FULL_FEATURES
|
@copilot code change failing two checks! Fix it! |
… E402 Co-authored-by: DaTiC0 <13198638+DaTiC0@users.noreply.github.com>
Fix CI: pin Flask 1.1.2 transitive deps and suppress noqa E402 in test imports
…ts in routes and app files
Summary
Validation
make test)make health)AI-Assisted Review (if applicable)
Risk & Rollback
Summary by Sourcery
Add application-level tests and align unittest discovery with a consistent filename pattern.
New Features:
Enhancements:
Tests: