Skip to content

Clean up distributed app builders to fix test failures#3093

Merged
JamesNK merged 8 commits intomainfrom
jamesnk/fix-filesystemwatcher-error
Mar 22, 2024
Merged

Clean up distributed app builders to fix test failures#3093
JamesNK merged 8 commits intomainfrom
jamesnk/fix-filesystemwatcher-error

Conversation

@JamesNK
Copy link
Copy Markdown
Member

@JamesNK JamesNK commented Mar 22, 2024

Problem:

  1. Test creates DistributedApplicationBuilder.
  2. DistributedApplicationBuilder creates HostApplicationBuilder.
  3. HostApplicationBuilder creates configuration, including a FileSystemWatcher.
  4. Test never builds builder and never disposes of host. The FileSystemWatcher hangs around until finalizer runs.

There are many tests like this. Too many watchers exceeds a Linux limit and errors start.

Fix proposed here:

  • Have an internal method on DistributedApplicationBuilder to clean up HostApplicationBuilder's config. Tests will need to be updated to use it.
  • Have a test helper type BuilderContainer that makes it easy to create and "dispose" builders.
  • Have TestDistributedApplicationBuilder that wraps an inner DistributedApplicationBuilder. It implements dispose, and disposing the test builder will create a host and then dispose it.

Only tests in AWSCloudFormationResourceTests are updated. All tests that create a builder but never create a host that is disposed will need to be changed.
I updated the tests I could find that didn't build and dispose a host.

Microsoft Reviewers: Open in CodeFlow

@JamesNK JamesNK added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Mar 22, 2024
@JamesNK JamesNK enabled auto-merge (squash) March 22, 2024 05:54
/// This class wraps the builder and provides a way to automatically dispose it to prevent test failures from excessive
/// FileSystemWatcher instances from many tests.
/// </summary>
public sealed class TestDistrubtedApplicationBuilder : IDisposable, IDistributedApplicationBuilder
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.

Suggested change
public sealed class TestDistrubtedApplicationBuilder : IDisposable, IDistributedApplicationBuilder
public sealed class TestDistributedApplicationBuilder : IDisposable, IDistributedApplicationBuilder

@JamesNK JamesNK merged commit 0fba749 into main Mar 22, 2024
@JamesNK JamesNK deleted the jamesnk/fix-filesystemwatcher-error branch March 22, 2024 06:55
@JamesNK JamesNK mentioned this pull request Mar 22, 2024
@joperezr
Copy link
Copy Markdown
Member

Thanks for fixing this James!

@github-actions github-actions bot locked and limited conversation to collaborators Apr 22, 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.

5 participants