Skip to content

Improve CallService_McpTool_TelemetryRecorded test reliability#13374

Merged
JamesNK merged 4 commits intomainfrom
copilot/improve-telemetry-recorded-test
Dec 8, 2025
Merged

Improve CallService_McpTool_TelemetryRecorded test reliability#13374
JamesNK merged 4 commits intomainfrom
copilot/improve-telemetry-recorded-test

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Dec 8, 2025

Description

Refactored the CallService_McpTool_TelemetryRecorded test to be more robust and removed it from quarantine. The test was flaky due to strict assumptions about telemetry ordering and initialization state.

Changes

  • Removed quarantine - Test now runs in regular CI pipeline
  • Loop-based telemetry reading - Replaces direct channel reads with loops that search for specific events, tolerating initialization and other intermediate telemetry
  • Focused assertions - Only validates presence of McpToolCall and EndOperation events, ignoring telemetry count and ordering
  • Simplified cleanup - Removed sender disposal and empty-channel verification that assumed no other telemetry would be present

The new pattern uses break for cleaner loop control:

// Read until we find the event we care about
bool foundMcpToolCall = false;
while (await channel.Reader.WaitToReadAsync().DefaultTimeout())
{
    var context = await channel.Reader.ReadAsync().DefaultTimeout();
    if (context.Name.Contains(TelemetryEventKeys.McpToolCall))
    {
        foundMcpToolCall = true;
        break;
    }
}
Assert.True(foundMcpToolCall, "Expected to find McpToolCall telemetry event");

Validation

The test has been validated by running it 50 consecutive times with a 100% pass rate, confirming the reliability improvements have successfully eliminated the flakiness.

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
    • No
  • Does the change require an update in our Aspire docs?
    • Yes
    • No
Original prompt

Improve CallService_McpTool_TelemetryRecorded test:

  • Remove QuarantinedTest attribute
  • Remove code to drain initialization telemetry
  • Change assert logic to read telemetry items in a loop.
  • Read until TelemetryEventKeys.McpToolCall is matched.
  • Then read until TelemetryEndpoints.TelemetryEndOperation is matched
  • Remove disposing the sender and verifying there is no other telemetry

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

- Remove QuarantinedTest attribute
- Remove initialization telemetry drain code
- Use loop pattern to read telemetry until McpToolCall is found
- Continue reading until EndOperation is found
- Remove sender disposal and verification code

Co-authored-by: JamesNK <303201+JamesNK@users.noreply.github.com>
Copilot AI changed the title [WIP] Improve CallService_McpTool_TelemetryRecorded test Improve CallService_McpTool_TelemetryRecorded test reliability Dec 8, 2025
Copilot AI requested a review from JamesNK December 8, 2025 01:20
await testTelemetrySender.DisposeAsync();
// Read telemetry items until we find the McpToolCall event
bool foundMcpToolCall = false;
while (!foundMcpToolCall && await testTelemetrySender.ContextChannel.Reader.WaitToReadAsync().DefaultTimeout())
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot Remove foundMcpToolCall from while test. Instead, break after foundMcpToolCall is set to true.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in e527a40. Removed foundMcpToolCall from the while condition and added break after setting the flag to true.

Assert.False(hasMore);
// Then read until we find the EndOperation event
bool foundEndOperation = false;
while (!foundEndOperation && await testTelemetrySender.ContextChannel.Reader.WaitToReadAsync().DefaultTimeout())
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot Remove foundEndOperation from while test. Instead, break after foundEndOperation is set to true.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in e527a40. Removed foundEndOperation from the while condition and added break after setting the flag to true.

- Remove foundMcpToolCall from while condition, use break after setting flag
- Remove foundEndOperation from while condition, use break after setting flag

Co-authored-by: JamesNK <303201+JamesNK@users.noreply.github.com>
Copilot AI requested a review from JamesNK December 8, 2025 01:31
@JamesNK
Copy link
Copy Markdown
Member

JamesNK commented Dec 8, 2025

@copilot Run CallService_McpTool_TelemetryRecorded test 50 times. It should pass consistently.

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Dec 8, 2025

@copilot Run CallService_McpTool_TelemetryRecorded test 50 times. It should pass consistently.

✅ Test passed all 50 runs consistently. Duration ranged from 1.066s to 1.149s per run. No failures observed.

@JamesNK JamesNK marked this pull request as ready for review December 8, 2025 01:41
Copilot AI review requested due to automatic review settings December 8, 2025 01:41
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Dec 8, 2025

🚀 Dogfood this PR with:

⚠️ WARNING: Do not do this without first carefully reviewing the code of this PR to satisfy yourself it is safe.

curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 13374

Or

  • Run remotely in PowerShell:
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 13374"

1 similar comment
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Dec 8, 2025

🚀 Dogfood this PR with:

⚠️ WARNING: Do not do this without first carefully reviewing the code of this PR to satisfy yourself it is safe.

curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 13374

Or

  • Run remotely in PowerShell:
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 13374"

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR successfully removes the CallService_McpTool_TelemetryRecorded test from quarantine by refactoring it to use a more robust loop-based telemetry reading pattern. The changes eliminate flakiness caused by strict assumptions about telemetry ordering and initialization state.

Key Changes:

  • Removed QuarantinedTest attribute to return test to regular CI pipeline
  • Replaced direct channel reads with loop-based pattern that searches for specific telemetry events
  • Simplified assertions to validate presence of key events rather than exact ordering and counts

@JamesNK JamesNK enabled auto-merge (squash) December 8, 2025 02:01
@JamesNK JamesNK merged commit e82eaab into main Dec 8, 2025
285 of 286 checks passed
@JamesNK JamesNK deleted the copilot/improve-telemetry-recorded-test branch December 8, 2025 02:11
@dotnet-policy-service dotnet-policy-service bot added this to the 13.1 milestone Dec 8, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Jan 7, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants