fix(ci): the test suite ran against a resolution nothing else used - #216
Conversation
pyproject.toml + uv.lock are the source of truth and the Dockerfile installs with `uv sync --frozen`, so what SHIPS is pinned. But test.yml and collector-live-check.yml installed from requirements.txt — unpinned lower bounds — and then pip-installed pytest and friends by hand on top. A green local suite was not evidence about what CI would do, nor about what a user installing today would get. Both workflows now sync from the lockfile and run pytest inside that environment, so the suite exercises exactly what the image contains. requirements.txt is a pinned export of the lock rather than a second, looser opinion, and a test asserts the two still agree. docker and tzdata move into the dev extra; they were imported by the suite and installed ad hoc. This also CORRECTS the diagnosis recorded in six files. The under-enumeration of app.routes was attributed to Starlette 1.3. It is not: holding Starlette at 1.3.1 and moving FastAPI alone gives 76 routes with /login on 0.136.1 and 62 without it on 0.141.1. The lock pins 0.136.1, so once CI installs from it the exposure is gone — the shared helper stays as protection for the next bump. One test encoded the wrong diagnosis directly: it asserted the union finds MORE routes than app.routes, which is false on the version that ships, where the two are equal. It now asserts the union is a SUPERSET — never sees less — which holds everywhere. Negative control: pointing test.yml back at requirements.txt fails 4; drifting one pin away from the lock fails 1; dropping docker/tzdata from the dev extra fails 2. Each revert prints its byte count. Closes CashPilot-de1
|
Warning Review limit reached
Next review available in: 42 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
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 |
1 participant
The defect
pyproject.toml+uv.lockare the source of truth, and the Dockerfile installs withuv sync --frozen— so what ships is pinned. Buttest.ymlandcollector-live-check.ymlinstalled fromrequirements.txt(unpinned lower bounds) and then pip-installed pytest and friends by hand on top.A green local suite was not evidence about what CI would do, nor about what a user installing today would get.
The fix
Both workflows sync from the lockfile and run pytest inside that environment, so the suite exercises exactly what the image contains.
requirements.txtbecomes a pinned export of the lock rather than a second, looser opinion — with a test asserting the two still agree.dockerandtzdatamove into the dev extra; they're imported by the suite and were installed ad hoc.This also corrects a diagnosis I got wrong in six files
I attributed the
app.routesunder-enumeration to Starlette 1.3. That's wrong. Holding Starlette at 1.3.1 and moving FastAPI alone:app.routes/loginpresentIt's FastAPI 0.141.1. And since the lock pins 0.136.1, once CI installs from the lock the exposure disappears — the shared enumeration helper from #215 stays as protection for the next bump, not as a workaround for today.
One test encoded the wrong diagnosis directly: it asserted the union finds more routes than
app.routes, which is false on the version that ships, where the two are equal. It now asserts the union is a superset — never sees less — which holds everywhere.Evidence
ruff check+ruff format --checkclean, both CI harnesses clean, 95.26% coverage, 2886 passed — run against the locked versions (uv sync --frozen --extra dev), which is what CI will now use.Negative control:
Closes CashPilot-de1