dotnet / aspire

Tools, templates, and packages to accelerate building observable, production-ready apps
https://learn.microsoft.com/dotnet/aspire
MIT License
3.94k stars 482 forks source link

Allow mock AddProject calls in external Hosting extensions tests #4877

Open eerhardt opened 4 months ago

eerhardt commented 4 months ago

When creating an external Hosting extension library, and trying to write tests for it, it is useful to be able to add a mock builder.AddProject<MockProject>("mockProject") so you can test that the connection string of your Resource gets hooked up to the project correctly. We do this today in a couple of tests, Qdrant and Milvus.

https://github.com/dotnet/aspire/blob/3ddc4cacc5b23f68acd09307b5741e6760bd5fc7/tests/Aspire.Hosting.Tests/Qdrant/AddQdrantTests.cs#L176-L184

https://github.com/dotnet/aspire/blob/3ddc4cacc5b23f68acd09307b5741e6760bd5fc7/tests/Aspire.Hosting.Tests/Qdrant/AddQdrantTests.cs#L314-L319

The issue is that while IProjectMetadata is public, it contains internal DIM'd properties

https://github.com/dotnet/aspire/blob/3ddc4cacc5b23f68acd09307b5741e6760bd5fc7/src/Aspire.Hosting/IProjectMetadata.cs#L13-L23

This means that unless Aspire.Hosting has InternalsVisibleTo the test (which it won't if these extensions are outside this repo), the test can't fill in these "for testing" properties, and an exception occurs because we try to read the ProjectPath, which doesn't exist.

To workaround this, when adding the project the test can set o => o.ExcludeLaunchProfile = true which tells the hosting code to not try to load the launch profile. But we should have a better way for extension test authors to enable this scenario.

Options I can think of:

cc @mitchdenny @davidfowl @sebastienros @radical @ReubenBond

mitchdenny commented 4 months ago

For these scenarios couldn't we just use the pathy version of AddProject since it doesn't check if the files actually exist at this stage of processing?

captainsafia commented 2 months ago

Bumping this issue up since some of the recent Azure Functions work has sparked new interest in this area.

We have a scenario in Azure Functions that requires us to do some special processing on the launch profile associated with the functions app to pull out the user-defined --port property and wire it up as an HTTP endpoint on the Aspire resource.

Currently, a lot of our launch profile-related infrastructure is internal to the AppHost. For the Functions scenario, we'd like to be able to query the launch profile using the pre-existing LaunchProfileExtensions, extract the LaunchProfile.CommandLineArgs property, and process it for a --port value that we can use to configure the AzureFunctionsProjectResource.

Right now, I've taken the approach @eerhardt outlined above:

We could make these properties public, which would mean making LaunchSettings public

The total set of new public APIs is:

Aspire.Hosting.IProjectMetadata.LaunchSettings.get -> Aspire.Hosting.LaunchSettings?
Aspire.Hosting.LaunchProfile
Aspire.Hosting.LaunchProfile.ApplicationUrl.get -> string?
Aspire.Hosting.LaunchProfile.ApplicationUrl.set -> void
Aspire.Hosting.LaunchProfile.CommandLineArgs.get -> string?
Aspire.Hosting.LaunchProfile.CommandLineArgs.set -> void
Aspire.Hosting.LaunchProfile.CommandName.get -> string?
Aspire.Hosting.LaunchProfile.CommandName.set -> void
Aspire.Hosting.LaunchProfile.DotnetRunMessages.get -> bool?
Aspire.Hosting.LaunchProfile.DotnetRunMessages.set -> void
Aspire.Hosting.LaunchProfile.EnvironmentVariables.get -> System.Collections.Generic.Dictionary<string!, string!>!
Aspire.Hosting.LaunchProfile.EnvironmentVariables.set -> void
Aspire.Hosting.LaunchProfile.LaunchBrowser.get -> bool?
Aspire.Hosting.LaunchProfile.LaunchBrowser.set -> void
Aspire.Hosting.LaunchProfile.LaunchProfile() -> void
Aspire.Hosting.LaunchProfile.LaunchUrl.get -> string?
Aspire.Hosting.LaunchProfile.LaunchUrl.set -> void
Aspire.Hosting.LaunchSettings
Aspire.Hosting.LaunchSettings.LaunchSettings() -> void
Aspire.Hosting.LaunchSettings.Profiles.get -> System.Collections.Generic.Dictionary<string!, Aspire.Hosting.LaunchProfile!>!
Aspire.Hosting.LaunchSettings.Profiles.set -> void

Thoughts on this approach given the requirements of both the testing and Functions scenarios?

mitchdenny commented 2 months ago

The only thing that gives me pause on this is that we are exposing a C# representation of a JSON format that we don't really own, and it would be nicer if these types actually came from the .NET SDK. Right now we have a rigid connection between Aspire and the format of launch settings - by exposing these types we potentially make our developers' code rigid too.

If we are pretty confident that launchSettings.json formats aren't going to change then I guess this is OK and we just have to keep up with the format.

captainsafia commented 2 months ago

@mitchdenny Good point! Another related point is the fact that our LaunchProfile type is only a subset of all the properties that are supported by the launchSettings.json schema.

I see a couple of ways to approach this:

Any other ideas or thoughts on the above?

mitchdenny commented 2 months ago

I think we forge ahead and expose the types. If the SDK ends up exposing its own types for this we could look to take a breaking change at some point in the future to harmonise if it becomes useful.

captainsafia commented 2 months ago

A proposed API shape following some recent discussions:

@eerhardt mentioned that we can be more conservative about the properties in the launch profile type that we make public and expose only the CommandLineArgs property that we need for Functions for now. This would mean the API shape would be:

Aspire.Hosting.LaunchProfile
Aspire.Hosting.LaunchProfile.CommandLineArgs.get -> string?
Aspire.Hosting.LaunchProfile.CommandLineArgs.set -> void
Aspire.Hosting.LaunchProfile.LaunchProfile() -> void
Aspire.Hosting.LaunchSettings
Aspire.Hosting.LaunchSettings.LaunchSettings() -> void
Aspire.Hosting.LaunchSettings.Profiles.get -> System.Collections.Generic.Dictionary<string!, Aspire.Hosting.LaunchProfile!>!
Aspire.Hosting.LaunchSettings.Profiles.set -> void

UPDATE: JK, we can't do this. The properties need to be public to support (de)serialization.

@davidfowl mentioned that it's a bit ick that all these new APIs are at the top-level Aspire.Hosting namespace. Creating a separate namespace for this stuff would also provide a nice landing space for other related APIs we'd consider making public, like the LaunchProfileAnnotation type @mitchdenny brought up in this comment.

We could consider moving these to a new Aspire.Hosting.Launch namespace so the updated API proposal would be:

Aspire.Hosting.IProjectMetadata.LaunchSettings.get -> Aspire.Hosting.Launch.LaunchSettings?
Aspire.Hosting.Launch.LaunchProfile
Aspire.Hosting.Launch.LaunchProfile.CommandLineArgs.get -> string?
Aspire.Hosting.Launch.LaunchProfile.CommandLineArgs.set -> void
Aspire.Hosting.Launch.LaunchProfile.LaunchProfile() -> void
Aspire.Hosting.Launch.LaunchSettings
Aspire.Hosting.Launch.LaunchSettings.LaunchSettings() -> void
Aspire.Hosting.Launch.LaunchSettings.Profiles.get -> System.Collections.Generic.Dictionary<string!, Aspire.Hosting.Launch.LaunchProfile!>!
Aspire.Hosting.Launch.LaunchSettings.Profiles.set -> void