Skip to content

fix: assert HED routes by reaching them, not by reading app.routes - #353

Open
neuromechanist wants to merge 1 commit into
developfrom
fix/route-introspection-starlette-1x
Open

fix: assert HED routes by reaching them, not by reading app.routes#353
neuromechanist wants to merge 1 commit into
developfrom
fix/route-introspection-starlette-1x

Conversation

@neuromechanist

Copy link
Copy Markdown
Member

develop and every open PR are red on one test, and it is not any of their
faults.

test_hed_routes_mounted does [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. CI installs unlocked (uv pip install --system -e ".[dev]"), so
it resolved starlette==1.6.0 / fastapi==0.141.1 while uv.lock pins
starlette==0.50.0 / fastapi==0.128.0. The break arrived from upstream with no
change in this repo.

The app is fine

Worth stating first, because an AttributeError in a route-mounting test looks
alarming. Verified directly under fastapi==0.141.1 / starlette==1.6.0:

introspection FAILS: '_IncludedRouter' object has no attribute 'path'
route types: ['APIRoute', 'Mount', 'Route', '_IncludedRouter']
  /hed/ask      -> HTTP 422   (route exists, rejected an empty body)
  /hed/sessions -> HTTP 200

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 APIRouter and is untouched: those
routes still carry .path under both versions, so it keeps its stronger
exact-path assertion.

Verified under both majors

  • Locked (starlette 0.50.0): 36 passed
  • CI's resolution (starlette 1.6.0): 36 passed

Not in scope

Two other CI jobs fail for unrelated reasons, and I left both alone:

  • Build and Test Docker Image -- the Dockerfile's apt-get step installing the
    GitHub CLI keyring exits 100. An apt/upstream-repo problem, not Python.
  • claude-review -- the action itself errors with "Internal error: directory
    mismatch for directory .../tsconfig.json".

Worth separate issues if they persist.

Related: #352 is blocked on this one.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant