Skip to content

Wait for exemplars dialog in metrics component test - #21394

Closed
James Newton-King (JamesNK) wants to merge 1 commit into
mainfrom
jamesnk/fix-metrics-dialog-test
Closed

James Newton-King (JamesNK) wants to merge 1 commit into
mainfrom
jamesnk/fix-metrics-dialog-test

Conversation

@JamesNK

Copy link
Copy Markdown
Member

Description

Fix a timing race in ChartContainer_UnavailableHistogramPercentiles_TablePreservesExemplars, observed in the Windows Dashboard.Components job for #21393.

The test clicks the exemplars button and immediately checks the dialog queue. bUnit's synchronous click helper dispatches the event without awaiting its completion, so the queue can still be empty when the assertion runs.

Wait for the dialog assertions with cut.WaitForAssertion, preserving all existing checks of the instrument, exemplar values, and timestamps. This is a test-only change; dashboard behavior is unchanged.

Validation

Run on Windows from the fix worktree with DOTNET_ROOT=C:\Development\Source\aspire\.dotnet and MSBUILDTERMINALLOGGER=false.

  • Built and ran the focused theory: 4/4 passed.
    & "$env:DOTNET_ROOT\dotnet.exe" test --project tests\Aspire.Dashboard.Components.Tests\Aspire.Dashboard.Components.Tests.csproj --no-launch-profile -- --filter-method '*.ChartContainer_UnavailableHistogramPercentiles_TablePreservesExemplars' --filter-not-trait 'quarantined=true' --filter-not-trait 'outerloop=true'
  • Repeated the focused theory 20 times: 80/80 cases passed.
    & "$env:DOTNET_ROOT\dotnet.exe" test --project tests\Aspire.Dashboard.Components.Tests\Aspire.Dashboard.Components.Tests.csproj --no-build --no-restore --no-launch-profile -- --filter-method '*.ChartContainer_UnavailableHistogramPercentiles_TablePreservesExemplars' --filter-not-trait 'quarantined=true' --filter-not-trait 'outerloop=true'
  • Ran the entire metrics test class: 24/24 passed.
    & "$env:DOTNET_ROOT\dotnet.exe" test --project tests\Aspire.Dashboard.Components.Tests\Aspire.Dashboard.Components.Tests.csproj --no-build --no-restore --no-launch-profile -- --filter-class '*.MetricsTests' --filter-not-trait 'quarantined=true' --filter-not-trait 'outerloop=true'
  • git diff --check: passed.

The original race did not reproduce locally before the fix (84/84 cases passed); diagnosis is based on the failing CI stack trace and bUnit's event-dispatch implementation. Local repetitions are a pre-check, not proof of CI stability. Regular PR CI validation is pending; the dedicated reproduce workflow has not been run.

Prepared using the fix-flaky-test skill.

Fixes # (issue)

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
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown
Contributor

🚀 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/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 21394

Or

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

@github-actions

Copy link
Copy Markdown
Contributor

Tests selector

1 / 100 PR test projects · 0 PR jobs, from 1 changed file.

Selected PR test projects (1 / 100)

Aspire.Dashboard.Components.Tests

Selected PR jobs (0)

none


How these were chosen — grouped by what changed

🧪 tests/Aspire.Dashboard.Components.Tests/Pages/MetricsTests.cs (changed test)
→ 1 directly: Aspire.Dashboard.Components.Tests

Job reasons

none


Selection computed for commit 1f66149.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

No active review issues remain for the test-only change that waits for asynchronous exemplar dialog assertions.

No open issues 🎉

What changed in this PR

Addresses a timing race in the dashboard metrics component test by waiting for exemplar dialog assertions after the button click. This is a test-only change; dashboard behavior is unchanged.

Changes:

  • Wraps dialog assertions in cut.WaitForAssertion.
  • Preserves checks for a single dialog, instrument identity, exemplar values, and timestamps.
File Description
tests/​Aspire.Dashboard.Components.Tests/​Pages/​MetricsTests.cs Waits for exemplar dialog assertions to accommodate asynchronous event completion.

Review effort: Balanced

Copy link
Copy Markdown
Collaborator

Supporting evidence for this fix. It also appears to address #21126, the same test and the same Assert.Single() Failure: The collection was empty on windows-latest.

Mechanism: bUnit's Click() starts the event dispatch through renderer.Dispatcher.InvokeAsync and returns without waiting for it. If another thread is still running work on the renderer dispatcher when the button is clicked, the handler is queued and dialogs is still empty when the assertion runs.

Controlled native check (windows-latest, unchanged run-tests.yml, 0 warnings / 0 errors): a qualification-only test held the renderer dispatcher on another thread, then clicked the same exemplar button.

Run Source Queued Click() Awaited dispatch Original test, 4 rows
41007744819 main 63c361dd Failed, The collection was empty Passed 4/4 passed
41007747664 ClickAsync variant Failed, The collection was empty Passed 4/4 passed

This reproduces the historical failure by forcing the dispatcher to be busy. It does not identify which background chart work held the dispatcher in the original CI runs, 39212142055 and 39216025977. Waiting with WaitForAssertion, as this PR does, covers that window. I am not opening a competing PR.

🤖 Posted by AspireAgent (AI Agent)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants