Skip to content

Fix flaky test and add more test logging#6366

Merged
JamesNK merged 4 commits intomainfrom
jamesnk/hosting-test-logging
Oct 18, 2024
Merged

Fix flaky test and add more test logging#6366
JamesNK merged 4 commits intomainfrom
jamesnk/hosting-test-logging

Conversation

@JamesNK
Copy link
Copy Markdown
Member

@JamesNK JamesNK commented Oct 18, 2024

Description

Adds more logging to tests.
Also improve flaky test to avoid potential race condition.

Fixes #6354

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?
    • Yes
      • Link to aspire-docs issue:
    • No
Microsoft Reviewers: Open in CodeFlow

@JamesNK JamesNK added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Oct 18, 2024
@JamesNK JamesNK requested review from adamint and drewnoakes October 18, 2024 01:42
@JamesNK JamesNK requested a review from mitchdenny as a code owner October 18, 2024 01:42
@JamesNK JamesNK changed the title Add test logging to some hosting tests Fix flaky test and add more test logging Oct 18, 2024
@JamesNK JamesNK enabled auto-merge (squash) October 18, 2024 05:38
@adamint
Copy link
Copy Markdown
Member

adamint commented Oct 18, 2024

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@adamint
Copy link
Copy Markdown
Member

adamint commented Oct 18, 2024

Test failure seemed unrelated, re-running.

@JamesNK JamesNK force-pushed the jamesnk/hosting-test-logging branch from e68aa4e to 52f1a31 Compare October 18, 2024 08:43
@JamesNK JamesNK merged commit 4636116 into main Oct 18, 2024
@JamesNK JamesNK deleted the jamesnk/hosting-test-logging branch October 18, 2024 10:01
return task.AsTask().TimeoutAfter(timeout, filePath, lineNumber);
}

[SuppressMessage("ApiDesign", "RS0026:Do not add multiple public overloads with optional parameters", Justification = "Required to maintain compatibility")]
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.

  1. Do we have public analyzers turned on for our tests? That seems wrong.
  2. What does "Required to maintain compatibility" mean for our tests? I don't think we need to worry about compatibility.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It’s copied from dotnet/aspnetcore

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.

It would be good to add that in a comment on the file

using System.Diagnostics.CodeAnalysis;
using System.Runtime.CompilerServices;

namespace Microsoft.AspNetCore.InternalTesting;
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.

Is this the right namespace to use?

@github-actions github-actions bot locked and limited conversation to collaborators Nov 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flaky DashboardServiceTests.WatchResources_ResourceHasCommands_CommandsSentWithResponse

4 participants