feat(hono): Support middleware spans defined in app groups#20465
feat(hono): Support middleware spans defined in app groups#20465
Conversation
There was a problem hiding this comment.
those are basically the tests like before, but split in two scenarios, with a different route prefix (/test-middleware and /test-subapp-middleware)
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 46f78a2. Configure here.
|
|
||
| expect(middlewareSpans).toHaveLength(2); | ||
| expect(middlewareSpans[0]?.description).toBe('middlewareA'); | ||
| expect(middlewareSpans[1]?.description).toBe('middlewareB'); |
There was a problem hiding this comment.
Missing .filter() makes span assertion too broad
Medium Severity
The original test filtered spans with .filter(span => span.op === 'middleware.hono' && span.origin === 'auto.middleware.hono') before sorting. The refactored version drops the .filter() entirely, so middlewareSpans now contains all transaction spans, not just middleware spans. The subsequent toHaveLength(2) and description checks operate on the wrong set, making the test fragile and semantically incorrect.
Reviewed by Cursor Bugbot for commit 46f78a2. Configure here.
There was a problem hiding this comment.
it's not filtered on purpose - makes the test too weak. We want to specifically check for all spans that are here
s1gr1d
changed the title
2 participants
Patches
app.routeto also support middleware defined in Hono route groups.The Node instrumentation still produces too many spans, this will be fixed in another PR - one test is skipped for now.
Closes #20449