Skip to content

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Oct 19, 2024

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change.

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
  • Does the change require an update in our Aspire docs?
    • Yes
      • Link to aspire-docs issue:
    • No
Microsoft Reviewers: Open in CodeFlow

@JamesNK JamesNK force-pushed the jamesnk/hosting-timeout branch from 66e3b6f to 48a7682 Compare October 20, 2024 23:30
@JamesNK JamesNK marked this pull request as ready for review October 20, 2024 23:47
@mitchdenny
Copy link
Member

I think merging this is a good idea. The only question is about timing (in terms of when to merge AND individual timeouts).

Some of our test cases need to be modified to use time provider but that will take some time to do that so some tests might become flakey initially whilst that gets worked through. So should we wait until 9.0 has a stable build.

(acknowledging that we are currently being impacted by flakey tests).

@@ -34,7 +34,7 @@ public async Task DisposeAsync()
public static async Task<RedisContainer> CreateContainerAsync()
{
var container = new RedisBuilder()
.WithImage($"{TestConstants.AspireTestContainerRegistry}/{RedisContainerImageTags.Image}:{RedisContainerImageTags.Tag}")
.WithImage($"{ComponentTestConstants.AspireTestContainerRegistry}/{RedisContainerImageTags.Image}:{RedisContainerImageTags.Tag}")
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a TestConstants type and renamed this one to avoid conflict.

@JamesNK
Copy link
Member Author

JamesNK commented Oct 21, 2024

Some of our test cases need to be modified to use time provider but that will take some time to do that so some tests might become flakey initially whilst that gets worked through. So should we wait until 9.0 has a stable build.

Do you want this to be in the 9.0 branch? While these are just test changes, there are a lot of test changes. There are benefits: it would make backporting test changes easier. Could make 9.0 branch less flaky.

I was imagining these changes would be merged on main and not be backported.

@JamesNK JamesNK added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Oct 21, 2024
@JamesNK JamesNK requested a review from davidfowl October 22, 2024 00:31
@JamesNK
Copy link
Member Author

JamesNK commented Oct 22, 2024

I don't want this PR to rot. It makes changes across hosting tests project and it will gather conflicts easily.

What feedback do you have? What changes are required before it can be merged?

@mitchdenny
Copy link
Member

If you are suggesting backporting this to release/9.0 I am OK with it ... but we should make sure we can get all the way to release/9.0 for sure before merging this to main otherwise every other backport will become painful because of the number of source files this change touches.

Fact that this is already starting to accumulate merge conflicts kind of makes my point. I'm 100% supportive of this change, it's just about timing (I like how this gives us localized control over wait behavior vs. some global approach).

@JamesNK
Copy link
Member Author

JamesNK commented Oct 24, 2024

If you are suggesting backporting this to release/9.0

No

@mitchdenny
Copy link
Member

I think this is a good change. Its heavy handed but I think test reliability will improve if we can more easily identify where hangs are occuring.

@JamesNK JamesNK force-pushed the jamesnk/hosting-timeout branch from 88f6140 to e90c817 Compare October 29, 2024 05:35
@JamesNK JamesNK merged commit 96ede03 into main Oct 29, 2024
9 checks passed
@JamesNK JamesNK deleted the jamesnk/hosting-timeout branch October 29, 2024 06:57
@github-actions github-actions bot locked and limited conversation to collaborators Nov 28, 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.

3 participants