fix: assert HED routes by reaching them, not by reading app.routes - #353
Open
neuromechanist wants to merge 1 commit into
Open
fix: assert HED routes by reaching them, not by reading app.routes#353neuromechanist wants to merge 1 commit into
neuromechanist wants to merge 1 commit into
Conversation
test_hed_routes_mounted did `[r.path for r in app.routes]`. Starlette 1.x puts _IncludedRouter objects in app.routes for anything added via include_router, and those have no .path, so the comprehension raises AttributeError and every PR in the repo goes red at once. CI installs unlocked (`uv pip install -e ".[dev]"`), so it resolved starlette 1.6.0 while uv.lock pins 0.50.0, and the break arrived without any change here. The app was never broken. Verified directly under fastapi 0.141.1 / starlette 1.6.0: /hed/ask answers 422 and /hed/sessions answers 200, so the routes are mounted and serving. Only the test was wrong, because it asserted a framework internal rather than the property its own docstring names. Now it reaches the three routes and requires that none answers 404, which is the only status that means "not mounted" -- a 422 is the route rejecting an empty body and a 405 would be a method mismatch, both of which are mounted. That cannot break on the next internals rename. Verified under both majors: 36 passed on the locked starlette 0.50.0 and 36 on CI's starlette 1.6.0.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
developand every open PR are red on one test, and it is not any of theirfaults.
test_hed_routes_mounteddoes[r.path for r in app.routes]. Starlette 1.xputs
_IncludedRouterobjects inapp.routesfor anything added viainclude_router, and those have no.path, so the comprehension raisesAttributeError. CI installs unlocked (uv pip install --system -e ".[dev]"), soit resolved
starlette==1.6.0/fastapi==0.141.1whileuv.lockpinsstarlette==0.50.0/fastapi==0.128.0. The break arrived from upstream with nochange in this repo.
The app is fine
Worth stating first, because an
AttributeErrorin a route-mounting test looksalarming. Verified directly under
fastapi==0.141.1/starlette==1.6.0:The routes are mounted and serving. Only the test was wrong: it asserted a
framework internal rather than the property its own docstring names.
The fix
Reach the three routes and require that none answers 404, which is the only
status that means "not mounted" -- a 422 is the route rejecting an empty body, and
a 405 would be a method mismatch; both are mounted. That assertion cannot break on
the next internals rename, which is the actual lesson here.
The sibling test at line 55 introspects a raw
APIRouterand is untouched: thoseroutes still carry
.pathunder both versions, so it keeps its strongerexact-path assertion.
Verified under both majors
starlette 0.50.0): 36 passedstarlette 1.6.0): 36 passedNot in scope
Two other CI jobs fail for unrelated reasons, and I left both alone:
apt-getstep installing theGitHub CLI keyring exits 100. An apt/upstream-repo problem, not Python.
mismatch for directory .../tsconfig.json".
Worth separate issues if they persist.
Related: #352 is blocked on this one.