Guard _assert_error_span_extras against duplicate span names#231
Merged
Conversation
Address CoPilot review on #230 (the guard landed after the squash-merge). _assert_error_span_extras picked candidates[0] for a span name; with a single-attempt fixture (124) names are unique, but a future fixture with multiple same-named spans (e.g. call-level retry) could silently assert the wrong span. Assert exactly one candidate so that case fails loudly here instead, rather than duplicating _assert_span_tree_matches's full name+attribute disambiguation. Test-only; no behavior change.
There was a problem hiding this comment.
Pull request overview
This PR hardens the conformance test helper _assert_error_span_extras (used by observability fixture 124) to fail loudly when a span name matches more than one finished span, avoiding accidental assertions against the wrong span if future fixtures produce repeated same-named spans (for example, due to retries).
Changes:
- Add an explicit
len(candidates) == 1assertion before selectingcandidates[0]in_assert_error_span_extras.
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.
Follow-up to #230. The CoPilot review thread on #230 was resolved with this guard, but it landed after the squash-merge, so main didn't carry it yet.
_assert_error_span_extras(the fixture-124 span-tree extras checker) pickedcandidates[0]for a given span name. Fixture 124 is single-attempt, so each span name is unique and this is correct today; but a future fixture with multiple same-named spans (e.g. call-level retry producing severalopenarmature.llm.completespans) could silently assert against the wrong one. This asserts exactly one candidate so that case fails loudly here, rather than duplicating_assert_span_tree_matches's full name+attribute disambiguation (the helper runs after that structural walker anyway).Test-only, no behavior change. Fixture 124 stays green.