Adding public API test coverage[Draft]

[Zombach]

Initial issues #2649 Issues #5047 message

Checklist

• Is this feature complete?
  • [ ] Yes. Ready to ship.
  • [x] No. Follow-up changes expected.
• Are you including unit tests for the changes and scenario tests if relevant?
  • [x] Yes
  • [ ] No
• Did you add public API?
  • [ ] Yes
  • [x] No
• Does the change require an update in our Aspire docs?
  • [ ] Yes
  • [x] No

Added public API test coverage for:

• [x] Microsoft.Extensions.ServiceDiscovery.Abstractions
• [x] Microsoft.Extensions.ServiceDiscovery
• [x] Aspire.Keycloak.Authentication
• [x] Aspire.Microsoft.Azure.Cosmos
• [x] Aspire.Azure.AI.OpenAI
• [x] Aspire.Azure.Data.Tables

Microsoft Reviewers: Open in CodeFlow

[Alirexaa]

hi @Zombach, I recommend changing the PR title to "[Draft] Adding public API test coverage for Microsoft.Extensions.ServiceDiscovery.* "

and I will send PR for "Adding public API test coverage for components ". also, the remaining hosting resources should be "Adding public API test coverage for Aspire.Hosting.* ".

by using this approach we separate all remaining test coverage into three PRs.

[Zombach]

hi @Zombach, I recommend changing the PR title to "[Draft] Adding public API test coverage for Microsoft.Extensions.ServiceDiscovery.* "

and I will send PR for "Adding public API test coverage for components ". also, the remaining hosting resources should be "Adding public API test coverage for Aspire.Hosting.* ".

by using this approach we separate all remaining test coverage into three PRs.

I thought about this. But the difference in PR volumes confuses me. For Microsoft.Extensions.ServiceDiscovery.* these are two packages. And for Aspire.Hosting.* 10+, for components 20+.

I think it's better to do something... something like "Adding public API test coverage for components#1" | "Adding public API test coverage for components#2" | "Adding public API test coverage for components#3" And throw +- 10 packets into each of them. Then PR will be more convenient for review

[Zombach]

hi @Zombach, I recommend changing the PR title to "[Draft] Adding public API test coverage for Microsoft.Extensions.ServiceDiscovery.* " and I will send PR for "Adding public API test coverage for components ". also, the remaining hosting resources should be "Adding public API test coverage for Aspire.Hosting.* ". by using this approach we separate all remaining test coverage into three PRs.

I thought about this. But the difference in PR volumes confuses me. For Microsoft.Extensions.ServiceDiscovery.* these are two packages. And for Aspire.Hosting.* 10+, for components 20+.

I think it's better to do something... something like "Adding public API test coverage for components#1" | "Adding public API test coverage for components#2" | "Adding public API test coverage for components#3" And throw +- 10 packets into each of them. Then PR will be more convenient for review

And in commits indicate the packages on which work was done. For example Microsoft.Extensions.ServiceDiscovery

[Alirexaa]

My goal is to decrease PR counts to a minimum of 3 PRs.

I think it's better to do something... something like "Adding public API test coverage for components#1" | "Adding public API test coverage for components#2" | "Adding public API test coverage for components#3" And throw +- 10 packets into each of them. Then PR will be more convenient for review

We did this until now, we opened new PR per project but based on https://github.com/dotnet/aspire/issues/5047#issuecomment-2278838761, we should stop doing that.

And in commits indicate the packages on which work was done. For example Microsoft.Extensions.ServiceDiscovery

We could do this in these 3 PRs too.

[Zombach]

We did this until now, we opened new PR per project but based on #5047 (comment), we should stop doing that.

I mean that the composition will be like this: Adding public API test coverage for components#1 Aspire.Azure.AI.OpenAI Aspire.Azure.Data.Tables Aspire.Azure.Messaging.EventHubs Aspire.Azure.Messaging.ServiceBus Aspire.Azure.Messaging.WebPubSub Aspire.Azure.Search.Documents Aspire.Azure.Security.KeyVault Aspire.Azure.Storage.Blobs Aspire.Azure. Storage.Queues Aspire.Confluent.Kafka

And then we would have only 3 PRs

[Alirexaa]

We did this until now, we opened new PR per project but based on #5047 (comment), we should stop doing that.

I mean that the composition will be like this: Adding public API test coverage for components#1 Aspire.Azure.AI.OpenAI Aspire.Azure.Data.Tables Aspire.Azure.Messaging.EventHubs Aspire.Azure.Messaging.ServiceBus Aspire.Azure.Messaging.WebPubSub Aspire.Azure.Search.Documents Aspire.Azure.Security.KeyVault Aspire.Azure.Storage.Blobs Aspire.Azure. Storage.Queues Aspire.Confluent.Kafka

And then we would have only 3 PRs

hmm, every PR has 10 projects, right? seem unrelated projects in the same PR but it's not important.

[Alirexaa]

@Zombach, Could you give me access to your branch?

[Zombach]

I'm working on Aspire.Keycloak.Authentication

[Zombach]

I'm working on Aspire.Microsoft.Azure.Cosmos

[Zombach]

Working on Aspire.Azure.AI.OpenAI

[Zombach]

Hi @sebastienros Could you give me a hint?

//There is a method
public static void AddKeyedAzureOpenAIClient(
    this IHostApplicationBuilder builder,
    string name,
    Action<AzureOpenAISettings>? configureSettings = null,
    Action<IAzureClientBuilder<AzureOpenAIClient, AzureOpenAIClientOptions>>? configureClientBuilder = null)
{
    ArgumentNullException.ThrowIfNull(builder);
    ArgumentException.ThrowIfNullOrEmpty(name);

    var configurationSectionName = OpenAIComponent.GetKeyedConfigurationSectionName(name, efaultConfigSectionName);
    new OpenAIComponent().AddClient(builder, configurationSectionName, configureSettings, configureClientBuilder, connectionName: name, serviceKey: name);
    builder.Services.TryAddKeyedSingleton(typeof(OpenAIClient), serviceKey: name, static (provider, key) => provider.GetRequiredKeyedService<AzureOpenAIClient>(key));
}

[Theory]
[InlineData(true)]
[InlineData(false)]
public void AddAzureTableClientShouldThrowWhenConnectionNameIsNullOrEmpty(bool isNull)
{
    var builder = new HostApplicationBuilder();
    var connectionName = isNull ? null! : string.Empty;
    Action<AzureDataTablesSettings>? configureSettings = null;
    Action<IAzureClientBuilder<TableServiceClient, TableClientOptions>>? configureClientBuilder = null;

    var action = () => builder.AddAzureTableClient(connectionName, configureSettings, configureClientBuilder);

    var exception = isNull ? Assert.Throws<ArgumentNullException>(action) : Assert.Throws<ArgumentException>(action);
    Assert.Equal(nameof(connectionName), exception.ParamName);
}

Is it worth checking via 'ArgumentException.ThrowIfNullOrEmpty'? So that you can write a check for 'null' and 'empty'? Or is checking for 'null' more than enough?

[Zombach]

Working on Aspire.Azure.Data.Tables

[Zombach]

Working on Aspire.Hosting.AWS

[Zombach]

Aspire.Confluent.Kafka

[Zombach]

Aspire.Azure.Security.KeyVault

[Zombach]

Aspire.Azure.Storage.Queues

[Zombach]

@Alirexaa Hello, are you going to add or write anything? Could you please review? So that I could send this PR for a full review?

[Zombach]

Aspire.Hosting.Dapr

[Alirexaa]

@Alirexaa Hello, are you going to add or write anything? Could you please review? So that I could send this PR for a full review?

can I add test coverage for the remaining projects or can we do it in another PR?

[sebastienros]

Thanks @Zombach / @Alirexaa for keeping up with the task. I will review soon but busy on E2E testing right now which is our current priority.

[Zombach]

@Alirexaa Hello, are you going to add or write anything? Could you please review? So that I could send this PR for a full review?

can I add test coverage for the remaining projects or can we do it in another PR?

I think it’s better to do it the next PR. This one is already big.

[danmoseley]

@Zombach going through old PR's. Is this still in progress -- perhaps was waiting for us (sorry). If so could you please reopen and resolve conflicts so we can review?

I had a look at resolving but I don't have enough context so I'm closing-as is. Thanks for the PR so far