Api tests#1362
Conversation
…Bruno for easy collaboration with the community.
|
|
WalkthroughThis PR adds a complete Bruno API test collection for the PictoPy backend, covering Albums, Face_Clusters, Folders, Health, Images, Models, Shutdown, User_Preferences, and memories endpoints, along with environment config and README documentation. It also adds a pytest suite for the models router and reformats the OpenAPI JSON documentation. ChangesBruno API Test Collection
Models Router pytest Suite and OpenAPI Docs
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/tests/Bruno/PictoPy/Albums/Delete Album.yml (1)
1-73: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftNo executable test assertions — file is a fixture/doc example only.
This Bruno definition (and the rest of this test collection) only contains
examplesrequest/response fixtures — there is notests/script:post-responseblock with actual assertions (status code checks, schema validation, etc.). Running this collection viabru runwould only fire the requests without verifying any response, so it does not provide automated regression coverage despite being framed as an "API test collection."Consider adding a
testsblock (Bruno supports JS assertions viachai/expect) to validate status codes and key response fields for each request.As per path instructions, "Ensure that test code is automated, comprehensive, and follows testing best practices" and "Verify that all critical functionality is covered by tests."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/Bruno/PictoPy/Albums/Delete` Album.yml around lines 1 - 73, The Delete Album Bruno fixture only defines example request/response payloads and has no executable assertions, so it does not actually test the API. Add a Bruno tests block (or script:post-response) to the Delete Album definition and include assertions for the DELETE request in the existing examples, using Bruno’s JS assertions (for example via expect/chai) to verify the status code and key response fields. Use the existing request name “Delete Album” and the example responses in this file to locate and wire in the automated checks.Source: Path instructions
🧹 Nitpick comments (7)
backend/tests/Bruno/PictoPy/Models/Get Hardware Recommendation.yml (1)
16-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLarge block of commented-out tests left in place.
Lines 17-61 contain a substantial block of disabled test assertions covering status, success flag, required fields, ram_gb, gpu_detected, gpu_names, available_providers, and recommended_tier validity, leaving only a single active test at lines 62-68. This dead code adds noise and hides most of the intended test coverage for this endpoint.
Consider either re-enabling these assertions (they align well with the handler's
{ "success": true, "data": hw_info }response shape) or removing the commented-out code if intentionally deferred.As per path instructions, "Point out redundant obvious comments that do not add clarity to the code" and "Ensure that all critical functionality is covered by tests".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/Bruno/PictoPy/Models/Get` Hardware Recommendation.yml around lines 16 - 68, The test file for Get Hardware Recommendation still contains a large commented-out block of core assertions, leaving only the gpu_detected/gpu_names check active. Update the existing test suite by either re-enabling the intended assertions around response status, success, required data fields, ram_gb, available_providers, and recommended_tier, or remove the dead commented code if it is no longer relevant. Keep the active coverage in the same test group so the endpoint behavior in this model response test remains fully validated.Source: Path instructions
backend/tests/test_models.py (4)
11-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
conftest.py/pytest config oversys.pathhack.Manually appending to
sys.pathis fragile — it depends on the test being invoked from a specific working directory and duplicates path-resolution logic that would normally live once inconftest.pyor apythonpathsetting inpytest.ini/pyproject.toml.♻️ Suggested approach
Move this to
backend/tests/conftest.py, or add topyproject.toml:[tool.pytest.ini_options] pythonpath = ["."]As per path instructions, "Check for PEP 8 violations and Python best practices."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/test_models.py` around lines 11 - 14, The test setup in test_models.py is using a manual sys.path.append hack to import app.routes.models, which should be moved into shared pytest configuration instead. Remove the path mutation from the test module and place the repository root import setup in backend/tests/conftest.py or configure pytest pythonpath in pyproject.toml/pytest.ini so imports like models_router and DownloadTaskEntry resolve consistently. Keep the test file focused on imports and test logic only.Source: Path instructions
217-323: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRepeated patch boilerplate across most tests.
The same 2-3 line
with patch(...)block is copy-pasted across roughly 30 test methods (e.g.,TestModelStatus,TestSetupModels,TestStartDownloadModel,TestDeleteModel). Consider consolidating into a fixture (e.g.,status_response_factoryor anautousefixture that patchesMODEL_REGISTRY/get_model_pathby default, similar to the pattern already used forhardware_response/download_facenet_response) to reduce duplication and ease future maintenance.♻️ Example consolidation for TestModelStatus
+@pytest.fixture +def status_response_factory(mock_model_registry): + def _make(file_exists: bool = False): + with patch("app.routes.models.MODEL_REGISTRY", mock_model_registry), \ + patch("app.routes.models.get_model_path", return_value="/fake/path.onnx"), \ + patch("app.routes.models.os.path.exists", return_value=file_exists): + return client.get("/models/status") + return _make + class TestModelStatus: - def test_returns_200(self, mock_model_registry): - with patch("app.routes.models.MODEL_REGISTRY", mock_model_registry), \ - patch("app.routes.models.get_model_path", return_value="/fake/path.onnx"), \ - patch("app.routes.models.os.path.exists", return_value=False): - response = client.get("/models/status") - assert response.status_code == 200 + def test_returns_200(self, status_response_factory): + assert status_response_factory().status_code == 200As per path instructions, "Look for code duplication."
Also applies to: 614-627, 696-756, 763-838
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/test_models.py` around lines 217 - 323, The repeated patch setup around the model status tests is duplicated across many methods, so consolidate it into a shared fixture or factory instead of copy-pasting the same `with patch(...)` block. Update the affected test classes such as `TestModelStatus`, `TestSetupModels`, `TestStartDownloadModel`, and `TestDeleteModel` to reuse a fixture like `status_response_factory` or an autouse patch fixture for `MODEL_REGISTRY`, `get_model_path`, and `os.path.exists`, matching the existing fixture pattern used elsewhere.Source: Path instructions
26-208: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNo type hints on fixtures or return values.
None of the fixture functions declare parameter or return type hints (e.g.,
-> dict,-> Response), which the path instructions call out explicitly for Python files.As per path instructions, "Ensure proper use of type hints."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/test_models.py` around lines 26 - 208, The fixture helpers in test_models.py are missing explicit type hints, which conflicts with the Python guidance to use proper typing. Add return annotations to each fixture such as the registry/config fixtures, the response fixtures, and the task-id fixtures, and annotate any fixture parameters they consume (for example, dependencies like mock_model_registry or mock_hardware_info) so the signatures of mock_model_registry, hardware_response, completed_task_response, and the other fixture functions are typed consistently.Source: Path instructions
583-586: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winWeak substring assertion for error status.
assert "error" in rawchecks the entire raw SSE payload for the substring "error", which could pass even if the parsed JSON'sstatusfield is something else (e.g., a message merely containing the word "error"). Prefer parsing the JSON payload and assertingparsed["status"] == "error", consistent with the more precise checks used elsewhere in this test class (e.g., Line 544-545).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/test_models.py` around lines 583 - 586, The test in test_error_stream_contains_error_status is using a weak raw substring check that can pass for the wrong reason. Update the assertion to parse the SSE payload as JSON, like the nearby tests in this class, and verify the parsed object's status field is exactly error instead of checking that the raw text merely contains the word. Use the existing error_task_response fixture and the same parsing approach already used in the other stream tests to keep the assertion precise.backend/tests/Bruno/PictoPy/User_Preferences/Get User Preferences.yml (1)
1-62: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftNo actual test assertions in this "API test" file.
This request only defines documentation-style
examples; there is no Brunotests {}orasserts {}block that programmatically validates the response (status code,successfield, schema, etc.). As currently written, running this file via the Bruno CLI would not fail even if the endpoint's contract regressed. This pattern repeats across the collection.Consider adding lightweight assertions, e.g.:
tests { test("status is 200", function () { expect(res.getStatus()).to.equal(200); }); test("has user_preferences", function () { expect(res.getBody()).to.have.property("user_preferences"); }); }As per path instructions, "Ensure that test code is automated, comprehensive, and follows testing best practices."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/Bruno/PictoPy/User_Preferences/Get` User Preferences.yml around lines 1 - 62, The Bruno request file only contains documentation examples and no executable validation, so it won’t catch regressions. Add a tests/asserts block to the Get User Preferences request with real checks against the response returned by the http GET on /user-preferences/; use the existing examples as a guide and verify status, success, and presence of user_preferences in the response body. Keep the checks lightweight but automated so the Bruno CLI fails when the endpoint contract changes.Source: Path instructions
backend/tests/Bruno/PictoPy/Folders/Add Folder.yml (1)
14-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a boolean placeholder for
taggingCompleted.backend/app/schemas/folders.pydefines this field asOptional[bool], so""in the Bruno request/example is misleading; usefalseto match the API shape.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/Bruno/PictoPy/Folders/Add` Folder.yml around lines 14 - 18, The Bruno request/example for the Add Folder flow uses an empty string for taggingCompleted, but the folders schema expects a boolean. Update the example payload in the Add Folder Bruno test so taggingCompleted uses a boolean placeholder like false, matching backend/app/schemas/folders.py and the API shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/tests/Bruno/PictoPy/Albums/Add` Images To Album.yml:
- Around line 15-20: The 200 example in the Add Images To Album Bruno test is
using an invalid empty image_ids payload, which conflicts with the
ImageIdsRequest validation. Update the request body in the success case to use a
non-empty image_ids list, and keep the empty array only in the 422 validation
example so the test matches the request schema and the behavior of
ImageIdsRequest.
In `@backend/tests/Bruno/PictoPy/Albums/Create` Album.yml:
- Around line 14-19: The 200 success example in the album create Bruno fixture
is invalid because CreateAlbumRequest requires a non-empty name, so this payload
would actually produce a 422. Update the example payload in the Create Album
YAML to use a realistic non-empty placeholder value for name, and make the same
correction in the other referenced success example so the fixture matches the
expected 200 response.
- Around line 1-87: The Bruno request definitions are missing automated
assertions, so they only document example requests/responses instead of
verifying behavior. Add a tests: block to the Create Album request in the Bruno
file using Bruno’s JS assertions, and reference the existing request/example
structure to check the response status, the success field, and the expected
response shape. Keep the assertions minimal but real so this file executes as an
automated test rather than a static fixture.
- Around line 43-55: The Bruno 200 response examples are inconsistent because
the success-case fixtures in the album request docs still return "success":
false. Update the example payloads in the Bruno YAML fixtures to reflect a true
successful response, and check the other newly added Bruno example files for the
same mismatch so the documented 200 cases match the actual success semantics.
In `@backend/tests/Bruno/PictoPy/Albums/Delete` Album.yml:
- Around line 39-45: The “Successful Response” example in the Delete Album Bruno
fixture is inconsistent because the sample body sets success to false. Update
the example payload in the Delete Album YAML so the success field matches a
successful 200 response, and keep the fixture body aligned with the documented
outcome for that request.
In `@backend/tests/Bruno/PictoPy/Albums/Remove` Image From Album.yml:
- Around line 26-81: The Remove Image From Album Bruno test only documents 200
and 422, but the remove_image_from_album route also returns 404 for a missing
album and 500 on delete failure. Update this example set to include 404 and 500
response examples alongside the existing SuccessResponse and validation case,
matching the style used in the Face_Clusters test files so the route’s critical
error paths are covered.
- Around line 45-51: The 200 response example in the Remove Image From Album
Bruno test is using an error-style payload with success set to false, which
conflicts with a successful response. Update the response body in this test to
reflect a true success case, keeping the structure consistent with the other
album-related examples and correcting the value in the JSON payload.
In `@backend/tests/Bruno/PictoPy/Albums/Remove` Images From Album.yml:
- Around line 51-57: The 200 response example in the album image removal request
is inconsistent because it shows a failed payload despite representing success.
Update the example response in the Remove Images From Album test to reflect a
successful outcome, matching the same pattern used by the related Remove Image
From Album request, and verify the response body fields in the documented JSON
example.
- Around line 29-90: The Bruno examples for the album bulk image removal route
only cover the happy path and a validation case that never reaches the handler
because `ImageIdsRequest` rejects an empty `image_ids` array first. Update
`Remove Images From Album.yml` to either replace the current 422 example with a
request payload that actually passes request validation for the `DELETE
/albums/:album_id/images` route, or add explicit examples for the handler’s 404
and 500 branches so `album_id` and the bulk removal flow are exercised.
In `@backend/tests/Bruno/PictoPy/Health/Root.yml`:
- Around line 25-33: The `/health` example response body in Root.yml is
incorrect because it uses an empty JSON payload instead of matching the actual
handler output. Update the example under the `response` section so it reflects
the JSON object returned by the backend `/health` handler, using the existing
health response example structure and the `message` field shape expected from
that endpoint.
In `@backend/tests/Bruno/PictoPy/Images/Get` All Images.yml:
- Around line 44-72: The 200/Successful Response example in the Get All Images
Bruno test is showing an incorrect success flag. Update the example payload in
the Get All Images response fixture so the documented success state matches the
actual successful response from the endpoint and response model, and make sure
the sample data under the JSON example remains aligned with that success case.
In `@backend/tests/Bruno/PictoPy/Images/Toggle` Favourite.yml:
- Around line 37-45: The 200 response example in the Toggle Favourite Bruno test
uses an empty JSON body, which does not match the route’s actual success
payload. Update the example under the response definition to reflect the real
shape returned by the toggle-favourite endpoint, and make sure the documented
JSON fields match the success response from the handler (including success,
image_id, and isFavourite).
In `@backend/tests/Bruno/PictoPy/Models/Download` Progress.yml:
- Around line 33-41: The 200 example for the GET
/models/download/{task_id}/progress SSE response is documented with the wrong
content type and body shape. Update the Bruno example in Download Progress.yml
to match the StreamingResponse behavior from the download progress handler,
using the SSE media type from the endpoint and an event-stream style body
instead of JSON so the collection reflects the actual response for task_id.
In `@backend/tests/Bruno/PictoPy/User_Preferences/Get` User Preferences.yml:
- Around line 19-41: The 200 Response example in the Get User Preferences Bruno
test is inconsistent because the response body under the successful GET example
sets success to false. Update the example payload in the examples block for the
GET request so the success field matches a successful 200 response, and keep the
user_preferences shape unchanged.
In `@backend/tests/Bruno/PictoPy/User_Preferences/Update` User Preferences.yml:
- Around line 11-17: The Update User Preferences Bruno example uses invalid
placeholder values and a contradictory success example. In the request body and
the 200 Response example, update the `GPU_Acceleration` field to a realistic
boolean value to match the schema, and make the example response consistent by
changing the `success` field in the 200 Response/Successful Response section to
`true`. Keep the request/response bodies in sync with the documented types in
the User Preferences example.
In `@backend/tests/Bruno/README.md`:
- Line 133: The instruction text in the Bruno README uses “Right click” and
should be updated to the hyphenated imperative form “Right-click” to match the
rest of the instructions and satisfy the language style check. Edit the affected
sentence in the README content and keep the rest of the step wording unchanged.
- Around line 111-117: The README table formatting in the Bruno tests docs still
uses compact pipe spacing that markdownlint rejects. Reformat the variables
table in the README section to match the repository’s preferred markdown table
style, keeping the same entries but adjusting the spacing/alignment so it passes
lint.
- Around line 97-101: The README example in the Bruno section is using a fenced
raw URL instead of a Markdown link, which triggers the lint warning. Update the
link text in the affected content to use standard Markdown link syntax for the
fetch.usebruno.com URL so it renders cleanly. Keep the surrounding wording the
same and make the change near the Bruno README example block.
- Around line 137-146: Tone down the pytest coverage statement in the Bruno
README so it only reflects what this PR actually adds. Update the “Pytest Tests”
section in README to avoid saying “every endpoint” and instead describe the
models pytest suite or general automated coverage in backend/tests, keeping the
rest of the guidance about pytest as the CI source of truth and Bruno for manual
exploration.
- Around line 72-89: Fix the Quick Start Markdown so the explanatory sentence is
no longer inside the fenced bash block and the block only contains actual shell
commands; move that sentence into normal paragraph text near the “Start the
PictoPy backend” step. Also convert the numbered step labels in this README
section into proper Markdown headings instead of bold emphasis, keeping the
existing “Start the PictoPy backend”, “Open the collection in Bruno”, “Activate
the environment”, and “Verify the connection” structure consistent and
lint-friendly.
---
Outside diff comments:
In `@backend/tests/Bruno/PictoPy/Albums/Delete` Album.yml:
- Around line 1-73: The Delete Album Bruno fixture only defines example
request/response payloads and has no executable assertions, so it does not
actually test the API. Add a Bruno tests block (or script:post-response) to the
Delete Album definition and include assertions for the DELETE request in the
existing examples, using Bruno’s JS assertions (for example via expect/chai) to
verify the status code and key response fields. Use the existing request name
“Delete Album” and the example responses in this file to locate and wire in the
automated checks.
---
Nitpick comments:
In `@backend/tests/Bruno/PictoPy/Folders/Add` Folder.yml:
- Around line 14-18: The Bruno request/example for the Add Folder flow uses an
empty string for taggingCompleted, but the folders schema expects a boolean.
Update the example payload in the Add Folder Bruno test so taggingCompleted uses
a boolean placeholder like false, matching backend/app/schemas/folders.py and
the API shape.
In `@backend/tests/Bruno/PictoPy/Models/Get` Hardware Recommendation.yml:
- Around line 16-68: The test file for Get Hardware Recommendation still
contains a large commented-out block of core assertions, leaving only the
gpu_detected/gpu_names check active. Update the existing test suite by either
re-enabling the intended assertions around response status, success, required
data fields, ram_gb, available_providers, and recommended_tier, or remove the
dead commented code if it is no longer relevant. Keep the active coverage in the
same test group so the endpoint behavior in this model response test remains
fully validated.
In `@backend/tests/Bruno/PictoPy/User_Preferences/Get` User Preferences.yml:
- Around line 1-62: The Bruno request file only contains documentation examples
and no executable validation, so it won’t catch regressions. Add a tests/asserts
block to the Get User Preferences request with real checks against the response
returned by the http GET on /user-preferences/; use the existing examples as a
guide and verify status, success, and presence of user_preferences in the
response body. Keep the checks lightweight but automated so the Bruno CLI fails
when the endpoint contract changes.
In `@backend/tests/test_models.py`:
- Around line 11-14: The test setup in test_models.py is using a manual
sys.path.append hack to import app.routes.models, which should be moved into
shared pytest configuration instead. Remove the path mutation from the test
module and place the repository root import setup in backend/tests/conftest.py
or configure pytest pythonpath in pyproject.toml/pytest.ini so imports like
models_router and DownloadTaskEntry resolve consistently. Keep the test file
focused on imports and test logic only.
- Around line 217-323: The repeated patch setup around the model status tests is
duplicated across many methods, so consolidate it into a shared fixture or
factory instead of copy-pasting the same `with patch(...)` block. Update the
affected test classes such as `TestModelStatus`, `TestSetupModels`,
`TestStartDownloadModel`, and `TestDeleteModel` to reuse a fixture like
`status_response_factory` or an autouse patch fixture for `MODEL_REGISTRY`,
`get_model_path`, and `os.path.exists`, matching the existing fixture pattern
used elsewhere.
- Around line 26-208: The fixture helpers in test_models.py are missing explicit
type hints, which conflicts with the Python guidance to use proper typing. Add
return annotations to each fixture such as the registry/config fixtures, the
response fixtures, and the task-id fixtures, and annotate any fixture parameters
they consume (for example, dependencies like mock_model_registry or
mock_hardware_info) so the signatures of mock_model_registry, hardware_response,
completed_task_response, and the other fixture functions are typed consistently.
- Around line 583-586: The test in test_error_stream_contains_error_status is
using a weak raw substring check that can pass for the wrong reason. Update the
assertion to parse the SSE payload as JSON, like the nearby tests in this class,
and verify the parsed object's status field is exactly error instead of checking
that the raw text merely contains the word. Use the existing error_task_response
fixture and the same parsing approach already used in the other stream tests to
keep the assertion precise.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 741f389b-fc85-4ac2-8e00-ebfbddb1f73b
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (50)
backend/tests/Bruno/PictoPy/Albums/Add Images To Album.ymlbackend/tests/Bruno/PictoPy/Albums/Create Album.ymlbackend/tests/Bruno/PictoPy/Albums/Delete Album.ymlbackend/tests/Bruno/PictoPy/Albums/Get Album Images.ymlbackend/tests/Bruno/PictoPy/Albums/Get Album.ymlbackend/tests/Bruno/PictoPy/Albums/Get Albums.ymlbackend/tests/Bruno/PictoPy/Albums/Remove Image From Album.ymlbackend/tests/Bruno/PictoPy/Albums/Remove Images From Album.ymlbackend/tests/Bruno/PictoPy/Albums/Update Album.ymlbackend/tests/Bruno/PictoPy/Albums/folder.ymlbackend/tests/Bruno/PictoPy/Face_Clusters/Face Tagging.ymlbackend/tests/Bruno/PictoPy/Face_Clusters/Get All Clusters.ymlbackend/tests/Bruno/PictoPy/Face_Clusters/Get Cluster Images.ymlbackend/tests/Bruno/PictoPy/Face_Clusters/Rename Cluster.ymlbackend/tests/Bruno/PictoPy/Face_Clusters/Trigger Global Reclustering.ymlbackend/tests/Bruno/PictoPy/Face_Clusters/folder.ymlbackend/tests/Bruno/PictoPy/Folders/Add Folder.ymlbackend/tests/Bruno/PictoPy/Folders/Delete Folders.ymlbackend/tests/Bruno/PictoPy/Folders/Disable Ai Tagging.ymlbackend/tests/Bruno/PictoPy/Folders/Enable Ai Tagging.ymlbackend/tests/Bruno/PictoPy/Folders/Get All Folders.ymlbackend/tests/Bruno/PictoPy/Folders/Sync Folder.ymlbackend/tests/Bruno/PictoPy/Folders/folder.ymlbackend/tests/Bruno/PictoPy/Health/Root.ymlbackend/tests/Bruno/PictoPy/Health/folder.ymlbackend/tests/Bruno/PictoPy/Images/Get All Images.ymlbackend/tests/Bruno/PictoPy/Images/Toggle Favourite.ymlbackend/tests/Bruno/PictoPy/Images/folder.ymlbackend/tests/Bruno/PictoPy/Models/Delete Model.ymlbackend/tests/Bruno/PictoPy/Models/Download Progress.ymlbackend/tests/Bruno/PictoPy/Models/Get Hardware Recommendation.ymlbackend/tests/Bruno/PictoPy/Models/Get Model Status.ymlbackend/tests/Bruno/PictoPy/Models/Setup Models.ymlbackend/tests/Bruno/PictoPy/Models/Start Download Model.ymlbackend/tests/Bruno/PictoPy/Models/folder.ymlbackend/tests/Bruno/PictoPy/Shutdown/Shutdown.ymlbackend/tests/Bruno/PictoPy/Shutdown/folder.ymlbackend/tests/Bruno/PictoPy/User_Preferences/Get User Preferences.ymlbackend/tests/Bruno/PictoPy/User_Preferences/Update User Preferences.ymlbackend/tests/Bruno/PictoPy/User_Preferences/folder.ymlbackend/tests/Bruno/PictoPy/environments/Local Development server.ymlbackend/tests/Bruno/PictoPy/memories/Generate Memories.ymlbackend/tests/Bruno/PictoPy/memories/Get Locations.ymlbackend/tests/Bruno/PictoPy/memories/Get On This Day.ymlbackend/tests/Bruno/PictoPy/memories/Get Timeline.ymlbackend/tests/Bruno/PictoPy/memories/folder.ymlbackend/tests/Bruno/PictoPy/opencollection.ymlbackend/tests/Bruno/README.mdbackend/tests/test_models.pydocs/backend/backend_python/openapi.json
| body: | ||
| type: json | ||
| data: |- | ||
| { | ||
| "image_ids": [] | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,120p' backend/tests/Bruno/PictoPy/Albums/Add\ Images\ To\ Album.yml
printf '\n--- schema ---\n'
sed -n '1,220p' backend/app/schemas/album.pyRepository: AOSSIE-Org/PictoPy
Length of output: 4125
Use a valid payload in the 200 example The success example still posts {"image_ids": []}, but ImageIdsRequest rejects empty lists with a 422. Keep the empty array for the 422 example and switch the 200 request to a non-empty list.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/Bruno/PictoPy/Albums/Add` Images To Album.yml around lines 15 -
20, The 200 example in the Add Images To Album Bruno test is using an invalid
empty image_ids payload, which conflicts with the ImageIdsRequest validation.
Update the request body in the success case to use a non-empty image_ids list,
and keep the empty array only in the 422 validation example so the test matches
the request schema and the behavior of ImageIdsRequest.
| info: | ||
| name: Create Album | ||
| type: http | ||
| seq: 2 | ||
| tags: | ||
| - Albums | ||
|
|
||
| http: | ||
| method: POST | ||
| url: "{{baseUrl}}/albums/" | ||
| body: | ||
| type: json | ||
| data: |- | ||
| { | ||
| "name": "", | ||
| "description": "", | ||
| "is_hidden": false, | ||
| "password": "" | ||
| } | ||
| auth: inherit | ||
|
|
||
| settings: | ||
| encodeUrl: true | ||
| timeout: 0 | ||
| followRedirects: true | ||
| maxRedirects: 5 | ||
|
|
||
| examples: | ||
| - name: 200 Response | ||
| description: Successful Response | ||
| request: | ||
| url: "{{baseUrl}}/albums/" | ||
| method: POST | ||
| body: | ||
| type: json | ||
| data: |- | ||
| { | ||
| "name": "", | ||
| "description": "", | ||
| "is_hidden": false, | ||
| "password": "" | ||
| } | ||
| response: | ||
| status: 200 | ||
| statusText: OK | ||
| headers: | ||
| - name: Content-Type | ||
| value: application/json | ||
| body: | ||
| type: json | ||
| data: |- | ||
| { | ||
| "success": false, | ||
| "album_id": "" | ||
| } | ||
| - name: 422 Response | ||
| description: Validation Error | ||
| request: | ||
| url: "{{baseUrl}}/albums/" | ||
| method: POST | ||
| body: | ||
| type: json | ||
| data: |- | ||
| { | ||
| "name": "", | ||
| "description": "", | ||
| "is_hidden": false, | ||
| "password": "" | ||
| } | ||
| response: | ||
| status: 422 | ||
| statusText: Unprocessable Entity | ||
| headers: | ||
| - name: Content-Type | ||
| value: application/json | ||
| body: | ||
| type: json | ||
| data: |- | ||
| { | ||
| "detail": [ | ||
| { | ||
| "loc": [], | ||
| "msg": "", | ||
| "type": "" | ||
| } | ||
| ] | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
No tests: assertion blocks in the Bruno request definitions.
None of the new Bruno files include an actual tests: script (Bruno supports JS-based assertions on response status/body). As written, these are static request/response examples rather than automated tests that verify behavior on run. Consider adding minimal assertions (status code, success field, response schema) to make this a genuine automated test suite rather than documentation-only fixtures. This applies to all files in this cohort.
As per path instructions, "Ensure that test code is automated, comprehensive, and follows testing best practices" and "Verify that all critical functionality is covered by tests".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/Bruno/PictoPy/Albums/Create` Album.yml around lines 1 - 87, The
Bruno request definitions are missing automated assertions, so they only
document example requests/responses instead of verifying behavior. Add a tests:
block to the Create Album request in the Bruno file using Bruno’s JS assertions,
and reference the existing request/example structure to check the response
status, the success field, and the expected response shape. Keep the assertions
minimal but real so this file executes as an automated test rather than a static
fixture.
Source: Path instructions
| { | ||
| "name": "", | ||
| "description": "", | ||
| "is_hidden": false, | ||
| "password": "" | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
200 example payload would actually trigger a 422, not success.
name: "" violates "name: str = Field(..., min_length=1)" in CreateAlbumRequest, so this exact request body would return a validation error rather than the 200 shown here. Consider using a non-empty placeholder name in the success example to keep the fixture realistic.
Also applies to: 37-42
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/Bruno/PictoPy/Albums/Create` Album.yml around lines 14 - 19,
The 200 success example in the album create Bruno fixture is invalid because
CreateAlbumRequest requires a non-empty name, so this payload would actually
produce a 422. Update the example payload in the Create Album YAML to use a
realistic non-empty placeholder value for name, and make the same correction in
the other referenced success example so the fixture matches the expected 200
response.
| response: | ||
| status: 200 | ||
| statusText: OK | ||
| headers: | ||
| - name: Content-Type | ||
| value: application/json | ||
| body: | ||
| type: json | ||
| data: |- | ||
| { | ||
| "success": false, | ||
| "album_id": "" | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
200 example marks "success": false for a "Successful Response".
This example is labeled as the 200 success case but the fixture body reports "success": false", which is misleading for anyone using this collection as documentation/reference. This same pattern recurs in every other Bruno example file added in this PR.
📝 Proposed fix
body:
type: json
data: |-
{
- "success": false,
+ "success": true,
"album_id": ""
}📝 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.
| response: | |
| status: 200 | |
| statusText: OK | |
| headers: | |
| - name: Content-Type | |
| value: application/json | |
| body: | |
| type: json | |
| data: |- | |
| { | |
| "success": false, | |
| "album_id": "" | |
| } | |
| response: | |
| status: 200 | |
| statusText: OK | |
| headers: | |
| - name: Content-Type | |
| value: application/json | |
| body: | |
| type: json | |
| data: |- | |
| { | |
| "success": true, | |
| "album_id": "" | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/Bruno/PictoPy/Albums/Create` Album.yml around lines 43 - 55,
The Bruno 200 response examples are inconsistent because the success-case
fixtures in the album request docs still return "success": false. Update the
example payloads in the Bruno YAML fixtures to reflect a true successful
response, and check the other newly added Bruno example files for the same
mismatch so the documented 200 cases match the actual success semantics.
| body: | ||
| type: json | ||
| data: |- | ||
| { | ||
| "success": false, | ||
| "msg": "" | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
"Successful Response" example body has "success": false.
The 200 example is labeled "Successful Response" but the sample body sets "success": false. This is misleading for anyone using these fixtures as API documentation.
Suggested fix
body:
type: json
data: |-
{
- "success": false,
+ "success": true,
"msg": ""
}📝 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.
| body: | |
| type: json | |
| data: |- | |
| { | |
| "success": false, | |
| "msg": "" | |
| } | |
| body: | |
| type: json | |
| data: |- | |
| { | |
| "success": true, | |
| "msg": "" | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/Bruno/PictoPy/Albums/Delete` Album.yml around lines 39 - 45,
The “Successful Response” example in the Delete Album Bruno fixture is
inconsistent because the sample body sets success to false. Update the example
payload in the Delete Album YAML so the success field matches a successful 200
response, and keep the fixture body aligned with the documented outcome for that
request.
| **1. Start the PictoPy backend** | ||
|
|
||
| ```bash | ||
| For detailed setup instructions, coding guidelines, and on how to start the backend, please check out our [CONTRIBUTING.md](./CONTRIBUTING.md) file. | ||
| ``` | ||
|
|
||
| **2. Open the collection in Bruno** | ||
|
|
||
| - Launch Bruno | ||
| - Click **Open Collection** | ||
| - Navigate to `backend/tests/Bruno/` inside the cloned repo | ||
| - Click **Open** | ||
|
|
||
| **3. Activate the environment** | ||
|
|
||
| Click the environment dropdown (top right) → select **Local Development server** | ||
|
|
||
| **4. Verify the connection** |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the Quick Start markdown structure.
The explanatory sentence is inside a fenced bash block, and the numbered step labels are styled as emphasis instead of headings. That makes the rendered README harder to scan and keeps tripping the markdownlint rules.
🧰 Tools
🪛 GitHub Check: Linting
[failure] 89-89: Emphasis used instead of a heading
backend/tests/Bruno/README.md:89 MD036/no-emphasis-as-heading Emphasis used instead of a heading [Context: "4. Verify the connection"] https://github.com/DavidAnson/markdownlint/blob/v0.40.0/doc/md036.md
[failure] 85-85: Emphasis used instead of a heading
backend/tests/Bruno/README.md:85 MD036/no-emphasis-as-heading Emphasis used instead of a heading [Context: "3. Activate the environment"] https://github.com/DavidAnson/markdownlint/blob/v0.40.0/doc/md036.md
[failure] 78-78: Emphasis used instead of a heading
backend/tests/Bruno/README.md:78 MD036/no-emphasis-as-heading Emphasis used instead of a heading [Context: "2. Open the collection in Brun..."] https://github.com/DavidAnson/markdownlint/blob/v0.40.0/doc/md036.md
[failure] 72-72: Emphasis used instead of a heading
backend/tests/Bruno/README.md:72 MD036/no-emphasis-as-heading Emphasis used instead of a heading [Context: "1. Start the PictoPy backend"] https://github.com/DavidAnson/markdownlint/blob/v0.40.0/doc/md036.md
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/Bruno/README.md` around lines 72 - 89, Fix the Quick Start
Markdown so the explanatory sentence is no longer inside the fenced bash block
and the block only contains actual shell commands; move that sentence into
normal paragraph text near the “Start the PictoPy backend” step. Also convert
the numbered step labels in this README section into proper Markdown headings
instead of bold emphasis, keeping the existing “Start the PictoPy backend”,
“Open the collection in Bruno”, “Activate the environment”, and “Verify the
connection” structure consistent and lint-friendly.
Source: Linters/SAST tools
| Click the button at the top of this page, or use this link directly: | ||
|
|
||
| ``` | ||
| https://fetch.usebruno.com?url=git@github.com:AOSSIE-Org/PictoPy.git | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use a Markdown link instead of a fenced URL.
The fetch URL doesn't need a code block; linkifying it will render more cleanly and remove the markdownlint warning.
🧰 Tools
🪛 GitHub Check: Linting
[failure] 99-99: Fenced code blocks should have a language specified
backend/tests/Bruno/README.md:99 MD040/fenced-code-language Fenced code blocks should have a language specified [Context: "```"] https://github.com/DavidAnson/markdownlint/blob/v0.40.0/doc/md040.md
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/Bruno/README.md` around lines 97 - 101, The README example in
the Bruno section is using a fenced raw URL instead of a Markdown link, which
triggers the lint warning. Update the link text in the affected content to use
standard Markdown link syntax for the fetch.usebruno.com URL so it renders
cleanly. Keep the surrounding wording the same and make the change near the
Bruno README example block.
Source: Linters/SAST tools
| | Variable | Default Value | Description | | ||
| |---|---|---| | ||
| | `baseUrl` | `http://localhost:52123` | Backend base URL | | ||
| | `taskId` | *(set after download/setup)* | Task ID returned by download or setup endpoints | | ||
| | `albumId` | *(set after create album)* | Album ID returned by create album | | ||
| | `folderId` | *(set after add folder)* | Folder ID returned by add folder | | ||
| | `clusterId` | *(set after clustering)* | Cluster ID returned by face cluster endpoints | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Normalize the table spacing.
This table still uses the compact pipe style that markdownlint rejects here. Reformatting it will keep CI green.
🧰 Tools
🪛 GitHub Check: Linting
[failure] 112-112: Table column style
backend/tests/Bruno/README.md:112:9 MD060/table-column-style Table column style [Table pipe is missing space to the right for style "compact"] https://github.com/DavidAnson/markdownlint/blob/v0.40.0/doc/md060.md
[failure] 112-112: Table column style
backend/tests/Bruno/README.md:112:9 MD060/table-column-style Table column style [Table pipe is missing space to the left for style "compact"] https://github.com/DavidAnson/markdownlint/blob/v0.40.0/doc/md060.md
[failure] 112-112: Table column style
backend/tests/Bruno/README.md:112:5 MD060/table-column-style Table column style [Table pipe is missing space to the right for style "compact"] https://github.com/DavidAnson/markdownlint/blob/v0.40.0/doc/md060.md
[failure] 112-112: Table column style
backend/tests/Bruno/README.md:112:5 MD060/table-column-style Table column style [Table pipe is missing space to the left for style "compact"] https://github.com/DavidAnson/markdownlint/blob/v0.40.0/doc/md060.md
[failure] 112-112: Table column style
backend/tests/Bruno/README.md:112:1 MD060/table-column-style Table column style [Table pipe is missing space to the right for style "compact"] https://github.com/DavidAnson/markdownlint/blob/v0.40.0/doc/md060.md
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/Bruno/README.md` around lines 111 - 117, The README table
formatting in the Bruno tests docs still uses compact pipe spacing that
markdownlint rejects. Reformat the variables table in the README section to
match the repository’s preferred markdown table style, keeping the same entries
but adjusting the spacing/alignment so it passes lint.
Source: Linters/SAST tools
|
|
||
| ### Entire collection | ||
|
|
||
| Right click the **PictoPy** collection in the sidebar → **Run Collection** → Bruno runs every request in order and shows a full summary. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Hyphenate “Right-click.”
This matches the rest of the imperative instructions and clears the language warning.
🧰 Tools
🪛 LanguageTool
[grammar] ~133-~133: Use a hyphen to join words.
Context: ... for fail. ### Entire collection Right click the PictoPy collection in the ...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/Bruno/README.md` at line 133, The instruction text in the Bruno
README uses “Right click” and should be updated to the hyphenated imperative
form “Right-click” to match the rest of the instructions and satisfy the
language style check. Edit the affected sentence in the README content and keep
the rest of the step wording unchanged.
Source: Linters/SAST tools
| ## Pytest Tests | ||
|
|
||
| Every endpoint also has automated pytest coverage inside `backend/tests/`. These run automatically on every push via GitHub Actions and are the source of truth for CI. | ||
|
|
||
| ```bash | ||
| cd backend/tests | ||
| pytest -v | ||
| ``` | ||
|
|
||
| Bruno is for manual exploration and quick verification during development. Pytest is for automation. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Tone down the pytest coverage claim.
The current PR stack only adds the models pytest suite, so “every endpoint” is broader than what’s actually included and may mislead readers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/Bruno/README.md` around lines 137 - 146, Tone down the pytest
coverage statement in the Bruno README so it only reflects what this PR actually
adds. Update the “Pytest Tests” section in README to avoid saying “every
endpoint” and instead describe the models pytest suite or general automated
coverage in backend/tests, keeping the rest of the guidance about pytest as the
CI source of truth and Bruno for manual exploration.
|
Please resolve the merge conflicts before review. Your PR will only be reviewed by a maintainer after all conflicts have been resolved. 📺 Watch this video to understand why conflicts occur and how to resolve them: |
1 participant
Addressed Issues:
Fixes #(TODO:issue number)
Screenshots/Recordings:
TODO: If applicable, add screenshots or recordings that demonstrate the interface before and after the changes.
Additional Notes:
AI Usage Disclosure:
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact. AI slop is strongly discouraged and may lead to banning and blocking. Do not spam our repos with AI slop.
Check one of the checkboxes below:
I have used the following AI models and tools: TODO
Checklist
Summary by CodeRabbit
New Features
Tests
Documentation