[tests] Refactor SqlServer tests to use testcontainers #3034

radical · 2024-03-20T03:37:56Z

Also add tracing tests for Aspire.Microsoft.Data.SqlClient.Tests

Microsoft Reviewers: Open in CodeFlow

radical · 2024-03-21T01:55:15Z

@eerhardt Are there any tests that I missed here?

…tcontainers

src/Aspire.Hosting.SqlServer/Aspire.Hosting.SqlServer.csproj

tests/Aspire.Microsoft.EntityFrameworkCore.SqlServer.Tests/SqlServerContainerFixture.cs

eerhardt · 2024-03-21T20:50:58Z

tests/Aspire.Microsoft.Data.SqlClient.Tests/ConformanceTests.cs

-    // TODO
-    protected override string[] RequiredLogCategories => Array.Empty<string>();
+    protected override string[] RequiredLogCategories => [
+        "Microsoft.Extensions.Hosting.Internal.ApplicationLifetime",


Were tests failing if we don't do this? We don't have these categories elsewhere.

The issue is that Microsoft.Data.SqlClient doesn't support logging. See Aspire.Microsoft.Data.SqlClient: in https://github.com/dotnet/aspire/blob/main/src/Components/Telemetry.md.

No, the tests weren't failing because they didn't check for any required categories.

From your link:

Aspire.Microsoft.EntityFrameworkCore.SqlServer: Log categories: "Microsoft.EntityFrameworkCore.ChangeTracking" "Microsoft.EntityFrameworkCore.Database.Command" "Microsoft.EntityFrameworkCore.Database.Connection" "Microsoft.EntityFrameworkCore.Database.Transaction" "Microsoft.EntityFrameworkCore.Infrastructure" "Microsoft.EntityFrameworkCore.Migrations" "Microsoft.EntityFrameworkCore.Model" "Microsoft.EntityFrameworkCore.Model.Validation" "Microsoft.EntityFrameworkCore.Query" "Microsoft.EntityFrameworkCore.Update"

Shouldn't we check for these?

oops, you were talking about SqlClient:

Aspire.Microsoft.Data.SqlClient: Log categories: none (the client does not provide an easy way to integrate it with logger factory)

- move SqlServerContainerFixture to `Aspire.Microsoft.Data.SqlClient.Tests` - Instead of using `InternalsVisibleTo`, compile the relevant file with the project.

…rumentation.SqlClient to Aspire.Microsoft.EntityFrameworkCore.SqlServer

… src/Components/Telemetry.md

…y.md

radical · 2024-03-22T19:36:48Z

I made the suggested changes. And also had to update ActivitySourceName:

-    protected override string ActivitySourceName => "OpenTelemetry.Instrumentation.SqlClient";
+    protected override string ActivitySourceName => "Aspire.Microsoft.Data.SqlClient";

-    protected override string ActivitySourceName => "OpenTelemetry.Instrumentation.SqlClient";
+    protected override string ActivitySourceName => "Aspire.Microsoft.EntityFrameworkCore.SqlServer";

does that change look correct?

tests/Aspire.Microsoft.Data.SqlClient.Tests/ConformanceTests.cs

src/Components/Telemetry.md

eerhardt · 2024-03-25T15:56:43Z

tests/Aspire.Microsoft.Data.SqlClient.Tests/ConformanceTests.cs

@@ -71,5 +81,26 @@ protected override void SetMetrics(MicrosoftDataSqlClientSettings options, bool
        => options.Metrics = enabled;

    protected override void TriggerActivity(SqlConnection service)
-        => service.Open();


Was just opening a connection not enough to trigger an activity?

Yeah, nothing was emitted with just that, and the tests would fail:

[xUnit.net 00:00:23.52] Aspire.Microsoft.Data.SqlClient.Tests.ConformanceTests.TracingEnablesTheRightActivitySource [FAIL] Failed Aspire.Microsoft.Data.SqlClient.Tests.ConformanceTests.TracingEnablesTheRightActivitySource [7 s] Error Message: Microsoft.DotNet.RemoteExecutor.RemoteExecutionException : Remote process failed with an unhandled exception. Stack Trace: Child exception: Xunit.Sdk.NotEmptyException: Assert.NotEmpty() Failure at Aspire.Components.ConformanceTests.ConformanceTests`2.ActivitySourceTest(String key) in /Users/ankj/dev/a3/tests/Aspire.Components.Common.Tests/ConformanceTests.cs:line 405 at Aspire.Microsoft.Data.SqlClient.Tests.ConformanceTests.<>c.<TracingEnablesTheRightActivitySource>b__24_1(ConformanceTests obj) in /Users/ankj/dev/a3/tests/Aspire.Microsoft.Data.SqlClient.Tests/ConformanceTests.cs:line 94 at Aspire.Microsoft.Data.SqlClient.Tests.ConformanceTests.RunWithFixtureAsync(Action`1 test) in /Users/ankj/dev/a3/tests/Aspire.Microsoft.Data.SqlClient.Tests/ConformanceTests.cs:line 104 at Aspire.Microsoft.Data.SqlClient.Tests.ConformanceTests.RunWithFixtureAsync(Action`1 test) in /Users/ankj/dev/a3/tests/Aspire.Microsoft.Data.SqlClient.Tests/ConformanceTests.cs:line 104 at Microsoft.DotNet.RemoteExecutor.Program.Main(String[] args) in /_/src/Microsoft.DotNet.RemoteExecutor/src/Program.cs:line 65 Child process: Aspire.Microsoft.Data.SqlClient.Tests, Version=42.42.42.42, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51 Aspire.Microsoft.Data.SqlClient.Tests.ConformanceTests+<>c System.Threading.Tasks.Task <TracingEnablesTheRightActivitySource>b__24_0()

* Change how resources expose urls to the dashboard - The resource server used to have a services and endpoints split, that is now a single object called a url which has a name, a full url (uri format) and a bool for if that endpoint is internal (or an implementation detail). - Clean up the way we extract urls from the dcp information by mapping the app model resource and endpoint within each service resource in DCP. - Mark fields as deprecated in the proto file - Moved from Tuples to records for CustomResourceSnapshot

Fix dotnet#3128

* Enable basic certificate auth for resource service This adds the ability to configured the gRPC connection from the dashboard to a resource service to use certificates for authentication. Such auth is not used in the local dev scenario, so the app host is changed to suppress auth via the `DOTNET_RESOURCE_SERVICE_DISABLE_AUTH` environment variable. The dashboard will now require certificates unless this variable is set to `1`. This discourages self-hosting the dashboard in an insecure manner. * Support loading client cert from keystore * Use enum for resource service auth mode * Dashboard web app supports OpenID Connect * Use a single authentication call for both the web app and OTLP Use explicit policies and configure those policies with the authentication schemes required. Also rejig some config settings for consistency. * Remove redundant configuration key * Rename "web app" to "frontend" * Reorder readme sections and adjust headings and their levels * Seal class * Simplify frontend auth implementation * Add docs * More thorough enum validation * Remove unused config key * Change exception type

…et#3140) [main] Update dependencies from dotnet/arcade

…et#3148) [main] Update dependencies from dotnet/arcade

This was missed in dotnet#3033.

* Fix SqlClient component activity name * Update instructions

Follow up to dotnet#3120

# Conflicts: # eng/Versions.props # src/Aspire.Hosting/Dcp/ApplicationExecutor.cs

eerhardt

LGTM. Thanks!

radical added testing ☑️ area-integrations Issues pertaining to Aspire Integrations packages labels Mar 20, 2024

radical requested a review from eerhardt March 21, 2024 01:54

radical marked this pull request as ready for review March 21, 2024 01:54

radical mentioned this pull request Mar 21, 2024

[tests] Move component tests to use testcontainers #3063

Open

9 tasks

radical force-pushed the sqlserver-testcontainers branch from ce9227e to b88d79a Compare March 21, 2024 05:28

radical added 3 commits March 21, 2024 02:06

[tests] Refactor SqlServer tests to use testcontainers

13689fb

Add SqlServerContainerImageTags

f42b611

Aspire.Microsoft.Data.SqlClient.Tests: add tracing tests, and use tes…

26d02c8

…tcontainers

radical force-pushed the sqlserver-testcontainers branch from b88d79a to 26d02c8 Compare March 21, 2024 06:07

Run sqlclient tests on helix

ef5177e

eerhardt reviewed Mar 21, 2024

View reviewed changes

radical added 5 commits March 22, 2024 14:59

Merge remote-tracking branch 'origin/main' into sqlserver-testcontainers

2621ae0

Address review feedback from @ eerhardt

03b6eb3

- move SqlServerContainerFixture to `Aspire.Microsoft.Data.SqlClient.Tests` - Instead of using `InternalsVisibleTo`, compile the relevant file with the project.

Update SqlServer/SqlClient ActivitySourceName from OpenTelemetry.Inst…

d80a12d

…rumentation.SqlClient to Aspire.Microsoft.EntityFrameworkCore.SqlServer

Remove RequiredLogCategories for SqlClient as none are supported, see…

968ab3d

… src/Components/Telemetry.md

Update Activity source names for SqlClient, and SqlServer in Telemetr…

651cd99

…y.md

radical requested review from sebastienros and eerhardt March 22, 2024 19:37

Merge branch 'main' into sqlserver-testcontainers

063a4cf

eerhardt mentioned this pull request Mar 25, 2024

Fix SqlClient component activity name #3118

Merged

eerhardt reviewed Mar 25, 2024

View reviewed changes

radical requested a review from eerhardt March 25, 2024 19:21

normj and others added 5 commits March 25, 2024 16:06

Fix typo in method name "Forwaded" -> "Forwarded" (dotnet#3123)

65c105c

Set the value to be the Text on Endpoints (dotnet#3130)

3528a3b

Fix dotnet#3128

Update dependencies from https://github.com/dotnet/arcade build (dotn…

519e461

…et#3140) [main] Update dependencies from dotnet/arcade

mitchdenny and others added 8 commits March 25, 2024 16:06

Fix PrincipalId/PrincipalType excess parameters. (dotnet#3135)

a756291

Update dependencies from https://github.com/dotnet/arcade build (dotn…

3ca1603

…et#3148) [main] Update dependencies from dotnet/arcade

Fix key store config binding (dotnet#3139)

2722266

This was missed in dotnet#3033.

Fix SqlClient component activity name (dotnet#3118)

c2b2ec6

* Fix SqlClient component activity name * Update instructions

Fix KeyVault README (dotnet#3151)

b0ddb23

Follow up to dotnet#3120

address review feedback

1b37dd6

Merge remote-tracking branch 'origin/main' into sqlserver-testcontainers

b18e683

# Conflicts: # eng/Versions.props # src/Aspire.Hosting/Dcp/ApplicationExecutor.cs

Fix merge

099a75c

eerhardt approved these changes Mar 26, 2024

View reviewed changes

eerhardt merged commit c96e8e0 into dotnet:main Mar 26, 2024
8 checks passed

radical deleted the sqlserver-testcontainers branch March 26, 2024 21:33

github-actions bot locked and limited conversation to collaborators Apr 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[tests] Refactor SqlServer tests to use testcontainers #3034

[tests] Refactor SqlServer tests to use testcontainers #3034

radical commented Mar 20, 2024 •

edited

Loading

radical commented Mar 21, 2024

eerhardt Mar 21, 2024

radical Mar 22, 2024

radical Mar 22, 2024

radical commented Mar 22, 2024

eerhardt Mar 25, 2024

radical Mar 25, 2024

eerhardt left a comment

[tests] Refactor SqlServer tests to use testcontainers #3034

[tests] Refactor SqlServer tests to use testcontainers #3034

Conversation

radical commented Mar 20, 2024 • edited Loading

Microsoft Reviewers: Open in CodeFlow

radical commented Mar 21, 2024

eerhardt Mar 21, 2024

Choose a reason for hiding this comment

radical Mar 22, 2024

Choose a reason for hiding this comment

radical Mar 22, 2024

Choose a reason for hiding this comment

radical commented Mar 22, 2024

eerhardt Mar 25, 2024

Choose a reason for hiding this comment

radical Mar 25, 2024

Choose a reason for hiding this comment

eerhardt left a comment

Choose a reason for hiding this comment

radical commented Mar 20, 2024 •

edited

Loading