Add relationship tests #13
Conversation
WalkthroughCentralizes and adds test helper fixtures in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T as Test (pytest)
participant F as Fixture helper (_create)
participant A as API server (POST/PUT endpoints)
participant D as Database
Note over T,F: Test calls shared fixture helper to create resources
T->>F: call _create(name, relations?, headers?)
F->>A: POST /{category,subcategory,product}/create (payload with relations)
A->>D: persist entity and relation links (many-to-many)
D-->>A: created resource + related IDs
A-->>F: HTTP 201 (resource body)
F-->>T: return response
opt update flow
T->>A: PUT /{resource}/<id>/update (modify relations)
A->>D: update relation tables
D-->>A: updated resource
A-->>T: HTTP 200
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 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 (
|
![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
🧹 Nitpick comments (15)
tests/conftest.py (3)
53-63: Solid, reusable category creation fixture; consider DRYing similar helpers.The fixture is clear and flexible (optional headers + subcategories). Given the nearly identical patterns across create_category/create_subcategory/create_product, consider extracting a small helper (e.g., _post_json(path, base_payload, headers)) to reduce duplication and centralize auth header fallback.
If you want, I can propose a small helper in this file and update all three fixtures.
66-79: Good coverage for cross-entity payloads; verify API behavior for invalid IDs.This fixture supports both categories and products in one call—nice. As a follow-up, tests that pass non-existent IDs (or duplicate IDs) would harden behavior around validation and idempotency (e.g., no duplicate association rows).
Would you like me to add negative-path tests that send invalid/duplicate IDs and assert error codes and messages?
81-93: Looks good; optional: avoid repeated register/login calls for perf.Repeated calls to create_authenticated_headers() trigger register/login each time. That’s fine for correctness, but if test runtime becomes a concern, a session-scoped “auth_headers” fixture could register once and then just login or reuse tokens.
tests/test_relationships.py (12)
7-15: Autouse setup is fine; optional: rely on the DB lifecycle in client() and drop the extra asserts.The asserts ensure clean tables and are harmless, but they duplicate the guarantees already provided by the client fixture’s create_all/drop_all lifecycle. If flakiness crops up with the in-memory SQLite setup, consider switching tests to a file-backed sqlite DB for robustness across connections.
15-20: Be explicit with dynamic relationships: call .all() before iterating.Category.subcategories is configured with lazy="dynamic", which returns a query. Iteration works today, but being explicit avoids surprises and clarifies intent.
- return sorted([subcategory.id for subcategory in category.subcategories]) + return sorted([subcategory.id for subcategory in category.subcategories.all()])
21-26: Same here: prefer .all() for clarity on dynamic relationship.- return sorted([category.id for category in subcategory.categories]) + return sorted([category.id for category in subcategory.categories.all()])
27-32: Same here: explicit .all() on dynamic products relation.- return sorted([product.id for product in subcategory.products]) + return sorted([product.id for product in subcategory.products.all()])
33-38: Same here: explicit .all() on dynamic subcategories for product.- return sorted([subcategory.id for subcategory in product.subcategories]) + return sorted([subcategory.id for subcategory in product.subcategories.all()])
39-44: Nit: also call .all() here for consistency.- return sorted({product.id for subcategory in category.subcategories for product in subcategory.products.all()}) + return sorted({product.id for subcategory in category.subcategories.all() for product in subcategory.products.all()})
87-97: Updates: assert 2xx (not strictly 201) unless the contract mandates 201 for updates.If the API guarantees 201 on update, keep as is. Otherwise, assert any 2xx to decouple the tests from implementation details.
- assert update_response.status_code == 201 + assert 200 <= update_response.status_code < 300
105-112: Same note for subcategory update: accept any 2xx unless 201 is part of the public contract.- assert update_response.status_code == 201 + assert 200 <= update_response.status_code < 300
121-125: Same note for product update endpoint.- assert update_response.status_code == 201 + assert 200 <= update_response.status_code < 300
145-161: Pagination assertions are brittle to default page size; parameterize or assert totals.The test assumes a default page size of 10. If that ever changes (config/env-driven), this will fail despite correct behavior. Prefer specifying an explicit page size if supported (e.g., per_page/page_size), or assert total items across pages and basic page-size constraints.
Option A (if API supports):
- page1 = self.client.get(f"/category/{category['id']}/products?page=1").get_json() - page2 = self.client.get(f"/category/{category['id']}/products?page=2").get_json() + page1 = self.client.get(f"/category/{category['id']}/products?page=1&page_size=10").get_json() + page2 = self.client.get(f"/category/{category['id']}/products?page=2&page_size=10").get_json()Option B (API doesn’t support page_size): keep size checks loose and assert totals.
- assert len(page1["products"]) == 10 - assert len(page2["products"]) == 2 + assert 1 <= len(page1["products"]) <= 12 + assert 0 < len(page2["products"]) <= 12And keep the total-ID set equality assertion you already have.
209-221: Nit: avoid hard-coding a large ID; derive a definitely-missing ID.Hard-coding 999999 works but can be made future-proof by computing max(id)+1 within each scope.
I can add a small helper to fetch max existing ID for each model and parametrize with that.
116-206: Great coverage for updates and getters; consider adding unlink/replace behavior tests.Right now tests cover additive updates. It’d be valuable to also verify:
- Replacing relationships (e.g., PUT with a different set removes previous links, if that’s the intended contract).
- Duplicate IDs in payload don’t create duplicate association rows.
- Unauthorized access (missing/invalid token) returns 401/403.
I can draft these tests if helpful.
📜 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)
tests/conftest.py(1 hunks)tests/test_category.py(0 hunks)tests/test_product.py(0 hunks)tests/test_relationships.py(1 hunks)tests/test_subcategory.py(0 hunks)
💤 Files with no reviewable changes (3)
- tests/test_category.py
- tests/test_product.py
- tests/test_subcategory.py
🧰 Additional context used
🧬 Code graph analysis (2)
tests/test_relationships.py (2)
app/models.py (4)
Category(64-76)Subcategory(79-92)Product(95-109)get(30-35)tests/conftest.py (5)
client(14-21)create_category(54-63)create_subcategory(67-78)create_product(82-93)create_authenticated_headers(43-50)
tests/conftest.py (1)
tests/test_subcategory.py (1)
_create(18-28)
🪛 Ruff (0.12.2)
tests/test_relationships.py
45-45: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
🔇 Additional comments (1)
tests/test_relationships.py (1)
52-86: Nicely thorough relationship creation tests.The bidirectional assertions and cross-graph checks (product visible via category through subcategories) are on point and catch many real-world regressions.
Summary by CodeRabbit