Adding public API test coverage #5250

Zombach · 2024-08-09T23:48:03Z

Initial issues #2649
Issues #5047
message

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
- No
Does the change require an update in our Aspire docs?
- Yes
- No

Added public API test coverage for:

Microsoft Reviewers: Open in CodeFlow

Alirexaa · 2024-08-10T06:18:44Z

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 · 2024-08-10T08:24:51Z

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 · 2024-08-10T08:26:15Z

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 · 2024-08-10T09:04:30Z

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 #5047 (comment), 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 · 2024-08-10T09:13:51Z

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 · 2024-08-10T09:38:04Z

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 · 2024-08-10T09:39:27Z

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

Zombach · 2024-08-10T20:46:55Z

I'm working on Aspire.Keycloak.Authentication

Zombach · 2024-08-11T10:35:39Z

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

Zombach · 2024-08-11T13:21:27Z

Working on Aspire.Azure.AI.OpenAI

Zombach · 2024-08-11T19:05:18Z

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 · 2024-08-11T19:07:57Z

Working on Aspire.Azure.Data.Tables

Zombach · 2024-08-15T21:38:03Z

Aspire.Azure.Storage.Queues

Zombach · 2024-08-15T21:46:35Z

@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 · 2024-08-17T11:42:48Z

Aspire.Hosting.Dapr

Alirexaa

@Zombach, We should cover empty strings.

src/Components/Aspire.Keycloak.Authentication/AspireKeycloakExtensions.cs

Alirexaa · 2024-08-17T14:43:39Z

src/Components/Aspire.Keycloak.Authentication/AspireKeycloakExtensions.cs

-        => builder.AddKeycloakJwtBearer(serviceName, realm, authenticationScheme, null);
+    {
+        ArgumentNullException.ThrowIfNull(builder);
+


Suggested change

ArgumentException.ThrowIfNullOrEmpty(serviceName);

ArgumentException.ThrowIfNullOrEmpty(realm);

ArgumentException.ThrowIfNullOrEmpty(authenticationScheme);

There is no need to check here, since the check occurs in the called method further

I khow but i perfer to check these first of methods.

I'm not sure that makes sense. When it's all in one class, and all the methods, it's an overload of the main one.

src/Components/Aspire.Keycloak.Authentication/AspireKeycloakExtensions.cs

src/Components/Aspire.Microsoft.Azure.Cosmos/AspireMicrosoftAzureCosmosExtensions.cs

Alirexaa · 2024-08-17T14:55:14Z

@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 · 2024-08-17T14:57:50Z

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 · 2024-08-17T18:33:15Z

@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.

Co-authored-by: Alireza Baloochi <[email protected]>

Microsoft.Extensions.ServiceDiscovery

75bc84e

dotnet-issue-labeler bot added the area-service-discovery label Aug 9, 2024

Zombach marked this pull request as draft August 9, 2024 23:48

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Aug 9, 2024

Zombach mentioned this pull request Aug 9, 2024

Check List: Validate arguments of public methods #5047

Open

56 tasks

Microsoft.Extensions.ServiceDiscovery.Abstractions

0b237e5

Merge branch 'dotnet:main' into validate-arguments-of-public-methods

4a87f47

Aspire.Keycloak.Authentication

b08bb94

Aspire.Microsoft.Azure.Cosmos

162aff7

Zombach added 2 commits August 11, 2024 21:18

Aspire.Azure.AI.OpenAI

0086380

remove address for Aspire.Hosting.Redis.Tests

99b8bd6

Zombach and others added 5 commits August 11, 2024 23:22

Aspire.Azure.Data.Tables

9904f9a

United null and empty

9a403db

Merge branch 'dotnet:main' into validate-arguments-of-public-methods

33492d5

Merge branch 'dotnet:main' into validate-arguments-of-public-methods

79cea71

Merge branch 'dotnet:main' into validate-arguments-of-public-methods

bdf50bc

build-analysis bot mentioned this pull request Aug 14, 2024

Some tests timing out on helix #5247

Open

Merge branch 'dotnet:main' into validate-arguments-of-public-methods

03a919a

Zombach and others added 4 commits August 16, 2024 02:12

Merge branch 'dotnet:main' into validate-arguments-of-public-methods

1f8e67b

Merge branch 'dotnet:main' into validate-arguments-of-public-methods

7d316f0

Merge branch 'dotnet:main' into validate-arguments-of-public-methods

761d5e7

Aspire.Hosting.Dapr

c56eea3

build-analysis bot mentioned this pull request Aug 17, 2024

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Open

3 tasks

Aspire.Hosting.Dapr#2

2beb33c

Zombach marked this pull request as ready for review August 17, 2024 13:04

Zombach requested review from radical, eerhardt and sebastienros as code owners August 17, 2024 13:04

Zombach changed the title ~~Adding public API test coverage[Draft]~~ Adding public API test coverage Aug 17, 2024

Alirexaa reviewed Aug 17, 2024

View reviewed changes

Zombach and others added 12 commits August 18, 2024 12:20

Apply suggestions from code review

02251c7

Co-authored-by: Alireza Baloochi <[email protected]>

Fix NullOrEmpty

6d17cf5

Merge branch 'dotnet:main' into validate-arguments-of-public-methods

ee4c279

Merge branch 'dotnet:main' into validate-arguments-of-public-methods

b43022d

Fix test Empty

cc710b5

Edits to suit a single style

8f90367

Merge branch 'dotnet:main' into validate-arguments-of-public-methods

accacdf

Merge branch 'dotnet:main' into validate-arguments-of-public-methods

90a1fd1

Merge branch 'dotnet:main' into validate-arguments-of-public-methods

8541955

Merge branch 'dotnet:main' into validate-arguments-of-public-methods

77d41d5

Merge branch 'dotnet:main' into validate-arguments-of-public-methods

3bc23a4

Merge branch 'dotnet:main' into validate-arguments-of-public-methods

f43a0ae

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding public API test coverage #5250

Adding public API test coverage #5250

Zombach commented Aug 9, 2024 •

edited

Loading

Alirexaa commented Aug 10, 2024

Zombach commented Aug 10, 2024

Zombach commented Aug 10, 2024

Alirexaa commented Aug 10, 2024

Zombach commented Aug 10, 2024

Alirexaa commented Aug 10, 2024

Alirexaa commented Aug 10, 2024

Zombach commented Aug 10, 2024

Zombach commented Aug 11, 2024

Zombach commented Aug 11, 2024

Zombach commented Aug 11, 2024 •

edited

Loading

Zombach commented Aug 11, 2024

Zombach commented Aug 15, 2024

Zombach commented Aug 15, 2024

Zombach commented Aug 17, 2024

Alirexaa left a comment

Alirexaa Aug 17, 2024

Zombach Aug 18, 2024

Alirexaa Aug 18, 2024

Zombach Aug 19, 2024

Alirexaa commented Aug 17, 2024

sebastienros commented Aug 17, 2024

Zombach commented Aug 17, 2024

+        ArgumentException.ThrowIfNullOrEmpty(serviceName);
+        ArgumentException.ThrowIfNullOrEmpty(realm);
+        ArgumentException.ThrowIfNullOrEmpty(authenticationScheme);

Adding public API test coverage #5250

Are you sure you want to change the base?

Adding public API test coverage #5250

Conversation

Zombach commented Aug 9, 2024 • edited Loading

Checklist

Microsoft Reviewers: Open in CodeFlow

Alirexaa commented Aug 10, 2024

Zombach commented Aug 10, 2024

Zombach commented Aug 10, 2024

Alirexaa commented Aug 10, 2024

Zombach commented Aug 10, 2024

Alirexaa commented Aug 10, 2024

Alirexaa commented Aug 10, 2024

Zombach commented Aug 10, 2024

Zombach commented Aug 11, 2024

Zombach commented Aug 11, 2024

Zombach commented Aug 11, 2024 • edited Loading

Zombach commented Aug 11, 2024

Zombach commented Aug 15, 2024

Zombach commented Aug 15, 2024

Zombach commented Aug 17, 2024

Alirexaa left a comment

Choose a reason for hiding this comment

Alirexaa Aug 17, 2024

Choose a reason for hiding this comment

Zombach Aug 18, 2024

Choose a reason for hiding this comment

Alirexaa Aug 18, 2024

Choose a reason for hiding this comment

Zombach Aug 19, 2024

Choose a reason for hiding this comment

Alirexaa commented Aug 17, 2024

sebastienros commented Aug 17, 2024

Zombach commented Aug 17, 2024

Zombach commented Aug 9, 2024 •

edited

Loading

Zombach commented Aug 11, 2024 •

edited

Loading