test(fastmcp): Span streaming tests#6167
Conversation
Codecov Results 📊✅ 1917 passed | ⏭️ 318 skipped | Total: 2235 | Pass Rate: 85.77% | Execution Time: 3m 43s All tests are passing successfully. ✅ Patch coverage is 100.00%. Project has 12733 uncovered lines. Generated by Codecov Action |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 503a5ba. Configure here.
| # Check span was created | ||
| spans = [item.payload for item in items if item.type == "span"] | ||
| tool_spans = [ | ||
| s for s in spans if s["attributes"].get("sentry.op") == OP.MCP_SERVER |
There was a problem hiding this comment.
I had followed a somewhat similar pattern in another PR and had received feedback that we're trying to avoid this approach of selectively getting spans like this (filtering and matching on the op) because it removes some of the implicit assumptions that we're making about the amount of spans that we're emitting and their order.
Maybe something to sync about when we're all online so that we're all using the same approach going forward 😄
There was a problem hiding this comment.
In this case, the logic in the else: branch for the transaction-based variant has the same filter.
In general I'm trying to stay as close as possible to this philosophy (from the review comment you linked).
I'd advocate for following the logic in the old branch as close as possible, without any extra filtering or sorting of spans
If there are other improvements to the tests we can still discuss and make them in a follow-up but I'd consider them out of scope for porting the tests to span-first.
What do you think?
There was a problem hiding this comment.
If there are other improvements to the tests we can still discuss and make them in a follow-up but I'd consider them out of scope for porting the tests to span-first.
Yeah that's fair to leave these improvements out of scope for this PR, I wouldn't want this to be overloaded with a number of different changes.
My earlier comment was more to note the inconsistency we have if testing the implicit assumptions is important within this project, and wanting to ensure tests are ported over to the correct approach as part of the migration process if we can fit it in since the old version of the tests will be removed soon and we likely won't come back to this afterwards.
There was a problem hiding this comment.
Gotcha. I think there are different levels of implicit assumptions:
- Lines similar to
(span, ) = spanshave an implicit assumption that there is only one span, as otherwise the test fails with aValueError. - Lines like
(client_span, tool_span, ) = spansalso have an ordering assumption. There is an implicit assumption that exactly 2 spans have been captured, and that theclient_spanis captured before thetool_span.
As for preferring one approach over another, I think it depends if you only test positive features (the span that I intend to create is there), or also the absence of other spans. I'm not aware of either approach being prescribed over the other.
I also don't think we provide a guarantee on the ordering of spans, so lines like (client_span, tool_span, ) = spans probably just exist because it's the shortest syntax to extract the spans 🤷 .

Description
The following tests are not parametrized since they have no assertions on spans:
test_fastmcp_tool_with_none_return()test_fastmcp_tool_with_no_arguments()test_standalone_fastmcp_specific_features()test_mcp_fastmcp_specific_features()test_fastmcp_prompt_async()Adapting Tests
sedcommands used for converting transaction context managers:sedcommands used for converting event capture:sedcommands used for convertingop:sedcommands used for converting origin:sedcommands used for convertingdescription:sedcommands used for convertingdatatoattributes:sedcommands to remove transaction assertions:Issues
Reminders
tox -e linters.feat:,fix:,ref:,meta:)