Repository navigation
refactor: DRY consolidation, test coverage, and doc consistency - #1
Conversation
NON_TERMINAL_BUILD_STATES listed ADMITTED/RUNNING, which the backend never emits for build runs; a healthy SOLVING build rendered as 'failed' in the stepper. Use the real transient set (QUEUED/RESOLVING_DEPENDENCIES/SOLVING) emitted by curve_lab_lifecycle, pin all transient/terminal states in the unit tests, and fix a workspace test fixture that used the impossible RUNNING state.
- README: document the Python >=3.13,<3.14 cap (dal-python ships cp313-only wheels), name the real frontend CI job instead of a nonexistent 'web-quality' one, and fix the directory tree (migrations/ lives next to app/, risk lives in curve_risk.py, frontend gains src/curves/ and the full page list). - valuation.py: drop the nonexistent value_trade from the module docstring. - system.py: the router serves health, not market endpoints. - dependencies.py: annotate store_dependency as StoreProtocol, which is what get_store() actually returns.
…l rules - Register app-level exception handlers for CurveLabLifecycleError, QuoteCanonicalizationError, NotFoundError and ConflictError, replacing 18 copy-pasted try/except blocks in routers/curve_lab.py and 16 in the legacy routers. Trade creation keeps its distinct NotFoundError -> 422 mapping (a missing product/model is a validation failure, not a missing trade). - Make canonical_json.py the single canonical-byte implementation, unified on UTF-8 (matching the persisted canonical_error_utf8 contract); calibrations.py delegates and both stores import from the shared module. Pure-ASCII payloads hash identically under either encoding. - Extract instrument_order.py: one native-name map and one canonical (maturity, start, native_name) ordering used by both admission and the gateway, so the persisted calibration_index mapping can no longer drift between calibrations.py and dal_gateway.py (five former copies). - store.py: widen the seam docstring to cover calibrations and Curve Lab.
- Extract api_helpers.py (valuation polling, product/model/trade/portfolio factories) and curve_lab_helpers.py (DISCOUNT_KEY, job polling, completed build/import/risk runners, parametrized single-OIS document), removing triplicated helpers across six test files. - Fix the compiled-fixing subprocess to put the backend root on sys.path so the tests.* package imports resolve when run as a script. - New test_store_contract.py runs the same behavioral suite against both Store and DbStore (delete guards, trade cascade, CAS publish idempotency, restart reconciliation to SERVER_RESTARTED/TIMED_OUT) -- the in-memory store's first behavioral coverage. - New test_dal_gateway_native_paths.py drives the single-curve spec builder, all six instrument kinds, and the price_curve_lab_trades error branches through a dedicated recording fake dal, covering native request-building paths the shared fake never reaches. - Pin previously untested boundaries: MATRIX_DIMENSION_EXCEEDED rejection, malformed If-Match / runtime-manifest 422s, build/import 429 queue exhaustion, and portfolio 404s.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 101 |
| Duplication | -7 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
CI enforces ruff format --check; reformat the five files touched by this PR. Codacy flags every pytest assert as Bandit B101 (high), which makes any test-adding PR fail the gate -- exclude backend/tests from the Bandit engine in .codacy.yml.
There was a problem hiding this comment.
Pull request overview
This PR follows up on a codebase-wide DRY / test-coverage / documentation-consistency review by aligning frontend Curve Lab stepper state handling with backend-emitted states, consolidating backend canonicalization + instrument-order rules into shared utilities, centralizing backend exception handling, and expanding/refactoring test coverage via shared helpers and new contract/path suites.
Changes:
- Frontend: fix Curve Lab build stepper transient-state handling (and update unit tests) to match backend state machine.
- Backend: centralize exception mapping in app-level handlers; unify canonical JSON bytes/hash logic; consolidate calibration instrument ordering into a shared module.
- Tests: add Store vs DbStore behavioral contract suite, add native DAL gateway path coverage, and DRY common API/Curve Lab helpers.
Reviewed changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Updates runtime/docs details (Python cap, CI naming, tree layout). |
| frontend/tests/unit/curve_lab_workspace.test.tsx | Aligns workspace test fixtures with new transient build state. |
| frontend/tests/unit/curve_builder_utils.test.ts | Adds coverage for additional transient/terminal build states. |
| frontend/src/curves/curveBuilderUtils.ts | Updates NON_TERMINAL_BUILD_STATES to backend-emitted transient states. |
| backend/tests/test_valuation_failures.py | DRYs API test setup/polling via shared helpers. |
| backend/tests/test_store_contract.py | New behavioral contract suite covering Store + DbStore semantics. |
| backend/tests/test_eval_date.py | DRYs API setup/polling via shared helpers; removes local duplicates. |
| backend/tests/test_dal_gateway_native_paths.py | New tests covering native DalGateway request-building/error branches. |
| backend/tests/test_curve_lab_risk_api.py | Replaces duplicated helpers/constants with shared Curve Lab helpers. |
| backend/tests/test_curve_lab_lifecycle_api.py | Replaces duplicated helpers with shared Curve Lab helpers; adds new boundary tests. |
| backend/tests/test_curve_lab_build_modes.py | Imports shared DISCOUNT_KEY constant to avoid drift. |
| backend/tests/test_curve_lab_api.py | Updates canonical JSON expectation to UTF-8 (non-ASCII preserved). |
| backend/tests/test_calibration_acceptance_boundaries.py | Adds boundary test for matrix dimension pre-admission rejection. |
| backend/tests/test_api.py | DRYs valuation polling and entity creation with shared helpers. |
| backend/tests/curve_lab_helpers.py | New shared helpers/constants for Curve Lab API job polling/completion. |
| backend/tests/curve_lab_compiled_fixing_admission_subprocess.py | Adjusts sys.path/imports to use shared job polling helper. |
| backend/tests/api_helpers.py | New shared helpers for product/model/trade/portfolio valuation API tests. |
| backend/app/services/valuation.py | Docstring consistency update for blocking valuation paths. |
| backend/app/services/store.py | Uses canonical JSON utility as single source of truth. |
| backend/app/services/instrument_order.py | New shared canonical instrument ordering + native-name mapping. |
| backend/app/services/db/store_db.py | Switches to canonical JSON utility implementation. |
| backend/app/services/dal_gateway.py | Uses shared instrument-ordering utilities; removes duplicated mapping/sorting. |
| backend/app/services/canonical_json.py | Canonical JSON bytes now UTF-8, non-ASCII preserved; shared hashing utility. |
| backend/app/services/calibrations.py | Reuses canonical JSON + shared instrument ordering; removes duplicated logic. |
| backend/app/routers/trades.py | Removes per-route NotFound try/except in favor of app-level handlers (except trade create mapping). |
| backend/app/routers/system.py | Removes per-route NotFound try/except in favor of app-level handlers. |
| backend/app/routers/products.py | Removes per-route NotFound/Conflict try/except in favor of app-level handlers. |
| backend/app/routers/portfolios.py | Removes per-route NotFound try/except in favor of app-level handlers. |
| backend/app/routers/models.py | Removes per-route NotFound/Conflict try/except in favor of app-level handlers. |
| backend/app/routers/curve_lab.py | Removes per-route lifecycle/quote error wrapping; relies on app-level handlers. |
| backend/app/main.py | Adds centralized exception handlers for lifecycle/canonicalization/not-found/conflict. |
| backend/app/dependencies.py | Updates store dependency type to StoreProtocol for interface-based routing. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Summary
Follow-up to a full-codebase review along three axes (DRY, unit-test coverage, documentation consistency). Four logical commits:
fix(frontend): Curve Lab build-run stepper states —
NON_TERMINAL_BUILD_STATESlistedADMITTED/RUNNING, which the backend never emits; a healthySOLVINGbuild rendered as failed in the stepper. Now uses the real transient set (QUEUED/RESOLVING_DEPENDENCIES/SOLVING).docs: README + docstrings — document the
>=3.13,<3.14Python cap (cp313-only dal-python wheels), fix the nonexistent "web-quality CI job" reference, fix the directory tree (migrations/,curve_risk.py,src/curves/), and correct stale docstrings (value_trade, "market endpoints") plus thestore_dependencyreturn annotation.refactor(backend): error handling + canonical rules —
CurveLabLifecycleError/QuoteCanonicalizationError/NotFoundError/ConflictErrorreplace 34 copy-pasted try/except blocks across the routers. Trade creation keeps its distinctNotFoundError → 422mapping.canonical_json.pyis now the single canonical-byte implementation, unified on UTF-8 (matching the persistedcanonical_error_utf8contract); the calibration-side duplicate is gone. Pure-ASCII payloads hash identically either way; non-ASCII Curve Lab fingerprints computed before this change will not match recomputation.instrument_order.py: one native-name map and one canonical(maturity, start, native_name)ordering shared by admission and the gateway (previously five copies), so the persistedcalibration_indexmapping cannot drift.test(backend): shared helpers + coverage —
api_helpers.py/curve_lab_helpers.pyremove triplicated helpers across six test files.test_store_contract.py: the same behavioral suite runs against bothStoreandDbStore(delete guards, cascade, CAS publish idempotency, restart reconciliation) — first behavioral coverage for the in-memory store.test_dal_gateway_native_paths.py: a dedicated recording fake dal covers the single-curve spec builder, all six instrument kinds, and theprice_curve_lab_tradeserror branches — paths the shared fake never reaches.MATRIX_DIMENSION_EXCEEDEDrejection, malformedIf-Match/runtime-manifest 422s, build/import 429 queue exhaustion, portfolio 404s.Verification
ruff check app/ tests/clean.npm run build(type-check) clean.Known follow-ups (deliberately out of scope)
_declaration_countsvs_fallback_storage_nodes) is not byte-equivalent today (LOG_DISCOUNT anchor semantics differ); merging needs a contract decision first.pytest-cov,@vitest/coverage-v8).