Skip to content

Conversation

adamint
Copy link
Member

@adamint adamint commented May 14, 2025

Description

This test seems to take a looong time to complete even on hardware (~15-20 seconds). Based on the test failing on the test runner, I think we just need to increase the timeout. A lot.

Fixes #9152

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
  • Does the change require an update in our Aspire docs?

@Copilot Copilot AI review requested due to automatic review settings May 14, 2025 19:56
@adamint adamint requested review from radical and eerhardt as code owners May 14, 2025 19:56
@github-actions github-actions bot added the area-integrations Issues pertaining to Aspire Integrations packages label May 14, 2025
@adamint adamint requested a review from JamesNK May 14, 2025 19:57
Copy link
Contributor

@Copilot 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 increases the retry count in the asynchronous test helper to address the long timeout issues observed in theme change tests. Key changes include the addition of a customizable "retries" parameter in both overloads of AssertIsTrueRetryAsync and its use in AppBarTests to allow for longer timeouts.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
tests/Shared/AsyncTestHelpers.cs Added an optional "retries" parameter to improve timeout control.
tests/Aspire.Dashboard.Tests/Integration/Playwright/AppBarTests.cs Updated test call to use the increased retry count for better stability.

: $"The operation at {filePath}:{lineNumber} timed out after reaching the limit of {timeout.TotalMilliseconds}ms.";

public static Task AssertIsTrueRetryAsync(Func<bool> assert, string message, ILogger? logger = null)
public static Task AssertIsTrueRetryAsync(Func<bool> assert, string message, ILogger? logger = null, int retries = 10)
Copy link
Preview

Copilot AI May 14, 2025

Choose a reason for hiding this comment

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

Consider updating the method documentation for AssertIsTrueRetryAsync to include details about the new 'retries' parameter and its impact on the timeout behavior.

Copilot uses AI. Check for mistakes.

checkbox = page.GetByRole(AriaRole.Radio).And(page.GetByText(checkboxText)).First;

await AsyncTestHelpers.AssertIsTrueRetryAsync(
async () => await checkbox.IsCheckedAsync(),
Copy link
Preview

Copilot AI May 14, 2025

Choose a reason for hiding this comment

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

[nitpick] The lambda 'async () => await checkbox.IsCheckedAsync()' could be simplified to '() => checkbox.IsCheckedAsync()' since there's no additional async processing required, which can make the code more concise.

Suggested change
async () => await checkbox.IsCheckedAsync(),
() => checkbox.IsCheckedAsync(),

Copilot uses AI. Check for mistakes.

@radical
Copy link
Member

radical commented May 14, 2025

Would it be useful to get screenshots to see what might be going on? Is the webpage completely stuck, or not loading because of other errors?

@danmoseley danmoseley merged commit 5c81fe3 into dotnet:main May 15, 2025
255 checks passed
@danmoseley
Copy link
Member

don't we get screenshots (playwright?)

@github-actions github-actions bot locked and limited conversation to collaborators Jun 15, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-integrations Issues pertaining to Aspire Integrations packages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Failing test]: On macOS Aspire.Dashboard.Tests.Integration.Playwright.AppBarTests.AppBar_Change_Theme_ReloadPage
4 participants