feat: Enable SuccessfulCommandTest by leveraging Testcontainers

[HofmeisterAn]

Relates #4193.

Changes

I am submitting this pull request to enable SuccessfulCommandTest by leveraging Testcontainers. The changes in this PR spin up instances of Microsoft SQL Server that the test can run against. By doing so, we can achieve a broader coverage, reliable and reproducible test results.

With the use of Testcontainers, we can be sure that the test environment is always in the same state, regardless of the underlying host system. This is achieved by running the test against a container, which is created on-demand and destroyed after the test has completed. In addition, it allows the test to run in isolation, reducing the risk of interference from other processes or applications.

To run the tests with Testcontainers, a Moby compatible engine such as Docker is required.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: HofmeisterAn / name: Andre Hofmeister (ee58108)

[cijothomas]

@HofmeisterAn Thanks. Could you sign the CLA agreement, which is a pre-req before we can consider any contributions. The link from the CLA bot comment has instructions. Your employer might need to be consulted for any company specific instructions.

[HofmeisterAn]

I was able to integrate Testcontainers, enable and run the tests within just a few minutes. However, I was not aware of the current pipeline configuration, which adds unnecessary overhead and requires additional configuration when using it with Testcontainers. I'm not sure how we should proceed with the tests. I think the best approach is to move away from the Docker Compose setup and simply run the tests.

Regarding the issue with strong-named assemblies, I am aware of it. The issue has been addressed, and I am waiting for a release of Docker.DotNet to resolve it. Although it may not be critical for tests, it's good to know that it has been taken care of.

---

To run the tests in a Docker Compose configuration you need to provide a Docker endpoint. That is usually done by mounting the Docker socket. Its called Docker Wormhole pattern. The following diff runs fine, but I changed the build configuration to Debug to ignore the strong-named assemblies and missing documentation warnings.

Diff

diff --git a/build/docker-compose.net6.0.yml b/build/docker-compose.net6.0.yml
index 0d592597..07c5608d 100644
--- a/build/docker-compose.net6.0.yml
+++ b/build/docker-compose.net6.0.yml
@@ -5,5 +5,5 @@ services:
     build:
       args:
         PUBLISH_FRAMEWORK: net6.0
-        TEST_SDK_VERSION: 6.0
-        BUILD_SDK_VERSION: 7.0
+        TEST_SDK_VERSION: '6.0'
+        BUILD_SDK_VERSION: '7.0'
diff --git a/test/OpenTelemetry.Instrumentation.SqlClient.Tests/Dockerfile b/test/OpenTelemetry.Instrumentation.SqlClient.Tests/Dockerfile
index 1083201d..24144181 100644
--- a/test/OpenTelemetry.Instrumentation.SqlClient.Tests/Dockerfile
+++ b/test/OpenTelemetry.Instrumentation.SqlClient.Tests/Dockerfile
@@ -6,7 +6,7 @@ ARG BUILD_SDK_VERSION=7.0
 ARG TEST_SDK_VERSION=7.0
 
 FROM mcr.microsoft.com/dotnet/sdk:${BUILD_SDK_VERSION} AS build
-ARG PUBLISH_CONFIGURATION=Release
+ARG PUBLISH_CONFIGURATION=Debug
 ARG PUBLISH_FRAMEWORK=net7.0
 WORKDIR /repo
 COPY . ./
diff --git a/test/OpenTelemetry.Instrumentation.SqlClient.Tests/docker-compose.yml b/test/OpenTelemetry.Instrumentation.SqlClient.Tests/docker-compose.yml
index 2c4b2b27..fdb0a6c7 100644
--- a/test/OpenTelemetry.Instrumentation.SqlClient.Tests/docker-compose.yml
+++ b/test/OpenTelemetry.Instrumentation.SqlClient.Tests/docker-compose.yml
@@ -19,7 +19,10 @@ services:
       dockerfile: ./test/OpenTelemetry.Instrumentation.SqlClient.Tests/Dockerfile
     entrypoint: ["bash", "-c", "/wait && dotnet vstest OpenTelemetry.Instrumentation.SqlClient.Tests.dll --TestCaseFilter:CategoryName=SqlIntegrationTests"]
     environment:
+      - TESTCONTAINERS_HOST_OVERRIDE=host.docker.internal
       - OTEL_SQLCONNECTIONSTRING=Data Source=sql; User ID=sa; Password=Pass@word18
       - WAIT_HOSTS=sql:1433
     depends_on:
       - sql
+    volumes:
+      - /var/run/docker.sock:/var/run/docker.sock

When running Compose with the aforementioned diff, the tests run successfully.

docker-compose --file=test/OpenTelemetry.Instrumentation.SqlClient.Tests/docker-compose.yml --file=build/docker-compose.net6.0.yml --project-directory=. up --exit-code-from=tests --build

Successful test run

opentelemetry-dotnet-tests-1  | --------------------------------------------------------
opentelemetry-dotnet-tests-1  | docker-compose-wait - Everything's fine, the application can now start!
opentelemetry-dotnet-tests-1  | --------------------------------------------------------
opentelemetry-dotnet-tests-1  | Microsoft (R) Test Execution Command Line Tool Version 17.3.1 (x64)
opentelemetry-dotnet-tests-1  | Copyright (c) Microsoft Corporation.  All rights reserved.
opentelemetry-dotnet-tests-1  | 
opentelemetry-dotnet-tests-1  | Starting test execution, please wait...
opentelemetry-dotnet-tests-1  | A total of 1 test files matched the specified pattern.
opentelemetry-dotnet-tests-1  | [testcontainers.org 00:00:00.04] Connected to Docker:
opentelemetry-dotnet-tests-1  |   Host: unix:/var/run/docker.sock
opentelemetry-dotnet-tests-1  |   Server Version: 20.10.22
opentelemetry-dotnet-tests-1  |   Kernel Version: 5.10.102.1-microsoft-standard-WSL2
opentelemetry-dotnet-tests-1  |   API Version: 1.41
opentelemetry-dotnet-tests-1  |   Operating System: Docker Desktop
opentelemetry-dotnet-tests-1  |   Total Memory: 24.87 GB
opentelemetry-dotnet-tests-1  | [testcontainers.org 00:00:00.14] Docker container 0aa4a7a111763705c9018dcf49bc6c89ea6e56599c4424378a826ff8903b2050 created
opentelemetry-dotnet-tests-1  | [testcontainers.org 00:00:00.18] Start Docker container 0aa4a7a111763705c9018dcf49bc6c89ea6e56599c4424378a826ff8903b2050
opentelemetry-dotnet-tests-1  | [testcontainers.org 00:00:00.57] Docker container 4f557a612da8b84e739dce6af9b65f4f84f81743716d9bc78f4b54d1b4ec311b created
opentelemetry-dotnet-tests-1  | [testcontainers.org 00:00:00.58] Start Docker container 4f557a612da8b84e739dce6af9b65f4f84f81743716d9bc78f4b54d1b4ec311b
opentelemetry-dotnet-tests-1  | [testcontainers.org 00:00:00.92] Execute "/opt/mssql-tools/bin/sqlcmd -Q SELECT 1;" at Docker container 4f557a612da8b84e739dce6af9b65f4f84f81743716d9bc78f4b54d1b4ec311b
opentelemetry-dotnet-tests-1  | [testcontainers.org 00:00:03.76] Delete Docker container 4f557a612da8b84e739dce6af9b65f4f84f81743716d9bc78f4b54d1b4ec311b
opentelemetry-dotnet-tests-1  | [testcontainers.org 00:00:04.27] Docker container b8ffe923455eaa28fe72f9c0488ad3be5055b1a346fed2eefc8e8e1278a9a03c created
opentelemetry-dotnet-tests-1  | [testcontainers.org 00:00:04.27] Start Docker container b8ffe923455eaa28fe72f9c0488ad3be5055b1a346fed2eefc8e8e1278a9a03c
opentelemetry-dotnet-tests-1  | [testcontainers.org 00:00:04.75] Execute "/opt/mssql-tools/bin/sqlcmd -Q SELECT 1;" at Docker container b8ffe923455eaa28fe72f9c0488ad3be5055b1a346fed2eefc8e8e1278a9a03c
opentelemetry-dotnet-tests-1  | [testcontainers.org 00:00:07.34] Delete Docker container b8ffe923455eaa28fe72f9c0488ad3be5055b1a346fed2eefc8e8e1278a9a03c
opentelemetry-dotnet-tests-1  | [testcontainers.org 00:00:07.82] Docker container 46d3620e064a857cc27fac060ce822151bafb407c87598712fff1769a9b508be created
opentelemetry-dotnet-tests-1  | [testcontainers.org 00:00:07.82] Start Docker container 46d3620e064a857cc27fac060ce822151bafb407c87598712fff1769a9b508be
opentelemetry-dotnet-tests-1  | [testcontainers.org 00:00:08.12] Execute "/opt/mssql-tools/bin/sqlcmd -Q SELECT 1;" at Docker container 46d3620e064a857cc27fac060ce822151bafb407c87598712fff1769a9b508be
opentelemetry-dotnet-tests-1  | [testcontainers.org 00:00:10.77] Delete Docker container 46d3620e064a857cc27fac060ce822151bafb407c87598712fff1769a9b508be
opentelemetry-dotnet-tests-1  | [testcontainers.org 00:00:11.33] Docker container d0627e3b19bd887d520e2c0d7463186413f1a6562a5e8076291fa07aef9ad52f created
opentelemetry-dotnet-tests-1  | [testcontainers.org 00:00:11.34] Start Docker container d0627e3b19bd887d520e2c0d7463186413f1a6562a5e8076291fa07aef9ad52f
opentelemetry-dotnet-tests-1  | [testcontainers.org 00:00:11.68] Execute "/opt/mssql-tools/bin/sqlcmd -Q SELECT 1;" at Docker container d0627e3b19bd887d520e2c0d7463186413f1a6562a5e8076291fa07aef9ad52f
opentelemetry-dotnet-tests-1  | [testcontainers.org 00:00:14.32] Delete Docker container d0627e3b19bd887d520e2c0d7463186413f1a6562a5e8076291fa07aef9ad52f
opentelemetry-dotnet-tests-1  | [testcontainers.org 00:00:14.79] Docker container 2cdcd685820ce9e52dd6ef104ef16234485d6e2b1eada1351e81d282f2894f55 created
opentelemetry-dotnet-tests-1  | [testcontainers.org 00:00:14.80] Start Docker container 2cdcd685820ce9e52dd6ef104ef16234485d6e2b1eada1351e81d282f2894f55
opentelemetry-dotnet-tests-1  | [testcontainers.org 00:00:15.16] Execute "/opt/mssql-tools/bin/sqlcmd -Q SELECT 1;" at Docker container 2cdcd685820ce9e52dd6ef104ef16234485d6e2b1eada1351e81d282f2894f55
opentelemetry-dotnet-tests-1  | [testcontainers.org 00:00:17.78] Delete Docker container 2cdcd685820ce9e52dd6ef104ef16234485d6e2b1eada1351e81d282f2894f55
opentelemetry-dotnet-tests-1  | [testcontainers.org 00:00:18.23] Docker container f4b6a4641ff94b4e2498e29ec3d48b2a4060e8f1225c9780dc21e58a26ef0e55 created
opentelemetry-dotnet-tests-1  | [testcontainers.org 00:00:18.24] Start Docker container f4b6a4641ff94b4e2498e29ec3d48b2a4060e8f1225c9780dc21e58a26ef0e55
opentelemetry-dotnet-tests-1  | [testcontainers.org 00:00:18.57] Execute "/opt/mssql-tools/bin/sqlcmd -Q SELECT 1;" at Docker container f4b6a4641ff94b4e2498e29ec3d48b2a4060e8f1225c9780dc21e58a26ef0e55
opentelemetry-dotnet-tests-1  | [testcontainers.org 00:00:21.19] Delete Docker container f4b6a4641ff94b4e2498e29ec3d48b2a4060e8f1225c9780dc21e58a26ef0e55
opentelemetry-dotnet-tests-1  | [testcontainers.org 00:00:21.71] Docker container 2d84f1877b6ebfe9779f9a36b6f902f474072fc1e9bf8f12441e490938e5a30d created
opentelemetry-dotnet-tests-1  | [testcontainers.org 00:00:21.72] Start Docker container 2d84f1877b6ebfe9779f9a36b6f902f474072fc1e9bf8f12441e490938e5a30d
opentelemetry-dotnet-tests-1  | [testcontainers.org 00:00:22.06] Execute "/opt/mssql-tools/bin/sqlcmd -Q SELECT 1;" at Docker container 2d84f1877b6ebfe9779f9a36b6f902f474072fc1e9bf8f12441e490938e5a30d
opentelemetry-dotnet-tests-1  | [testcontainers.org 00:00:24.69] Delete Docker container 2d84f1877b6ebfe9779f9a36b6f902f474072fc1e9bf8f12441e490938e5a30d
opentelemetry-dotnet-tests-1  | 
opentelemetry-dotnet-tests-1  | Passed!  - Failed:     0, Passed:     7, Skipped:     0, Total:     7, Duration: 20 s - /test/OpenTelemetry.Instrumentation.SqlClient.Tests.dll (net6.0)
opentelemetry-dotnet-tests-1 exited with code 0

As mentioned above, you do not need Docker Compose anymore. You can simply run

dotnet test

on your CI or local development environment.

[github-actions]

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

[HofmeisterAn]

@cijothomas any feedback how to continue?

[cijothomas]

@cijothomas any feedback how to continue?

Lets wait for the signing fix, so we can make the CI green before proceeding further.
I like the overall approach, and yes, we can get rid of existing manual docker spin ups once we fully move to this.

[github-actions]

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

[HofmeisterAn]

I have updated TC to the latest version which includes strong name assemblies and dedicated modules. We can try it again. However, I'm uncertain whether we need to change or disable the Compose part in the pipeline already.

[HofmeisterAn]

System.ArgumentException : Cannot detect the Docker endpoint. Use either the environment variables or the ~/.testcontainers.properties file to customize your configuration:

This issue relates to the Docker Compose environment. The container created by Compose does not include Docker itself. We can either adjust the Compose configuration (Docker Desktop requires the env variable too) by mounting the daemon socket, or we can remove Compose since it is not needed. In that case, you can simply run the tests.

[github-actions]

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

[alanwest]

@HofmeisterAn I like this! I've been curious about Testcontainers but hadn't had a chance to try them out yet.

[cijothomas]

@HofmeisterAn Consider making a small PR into this repo and get it merged, so that you'll no longer considered "New contriutor". (New contributors require one of the maintainers to trigger the CI builds adding delays to you ability to test quikly)

[codecov]

Codecov Report

Merging #4198 (6d80187) into main (63de729) will decrease coverage by 0.02%.
The diff coverage is n/a.

❗ Current head 6d80187 differs from pull request most recent head 0aae64c. Consider uploading reports for the commit 0aae64c to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4198      +/-   ##
==========================================
- Coverage   84.71%   84.69%   -0.02%     
==========================================
  Files         298      298              
  Lines       11971    11936      -35     
==========================================
- Hits        10141    10109      -32     
+ Misses       1830     1827       -3     

see 7 files with indirect coverage changes

[alanwest]

Woot! @HofmeisterAn, I pulled down your branch and ran the tests on my M1 Mac within VSCode.

Before starting up docker - test is skipped

OpenTelemetry.Instrumentation.SqlClient.Tests.SqlClientIntegrationTests.SuccessfulCommandTest:
    Outcome: Skipped
    Standard Output Messages:
    The Docker Linux engine is not available.
    
Total tests: 1. Passed: 0. Failed: 0. Skipped: 1

After starting up docker - test runs and passes (takes about 2 minutes first run. 1 minute subsequent runs.)

[xUnit.net 00:00:01.03]   Starting:    OpenTelemetry.Instrumentation.SqlClient.Tests
[xUnit.net 00:01:57.99]   Finished:    OpenTelemetry.Instrumentation.SqlClient.Tests
----- Test Execution Summary -----

OpenTelemetry.Instrumentation.SqlClient.Tests.SqlClientIntegrationTests.SuccessfulCommandTest:
    Outcome: Passed
    
OpenTelemetry.Instrumentation.SqlClient.Tests.SqlClientIntegrationTests.SuccessfulCommandTest:
    Outcome: Passed
    
OpenTelemetry.Instrumentation.SqlClient.Tests.SqlClientIntegrationTests.SuccessfulCommandTest:
    Outcome: Passed
    
OpenTelemetry.Instrumentation.SqlClient.Tests.SqlClientIntegrationTests.SuccessfulCommandTest:
    Outcome: Passed
    
OpenTelemetry.Instrumentation.SqlClient.Tests.SqlClientIntegrationTests.SuccessfulCommandTest:
    Outcome: Passed
    
OpenTelemetry.Instrumentation.SqlClient.Tests.SqlClientIntegrationTests.SuccessfulCommandTest:
    Outcome: Passed
    
OpenTelemetry.Instrumentation.SqlClient.Tests.SqlClientIntegrationTests.SuccessfulCommandTest:
    Outcome: Passed
    
Total tests: 7. Passed: 7. Failed: 0. Skipped: 0

[alanwest]

Ran on my Windows machine too using Visual Studio. I got the test to pass... kinda. But Visual Studio's test discoverability is confused. Maybe something about the custom theory? It does not seem to find all the test cases - should be 7, only finds 1.

Maybe just my machine, but curious if you also ran into anything funky here.. I assume you're also running on Windows.. cause you know, BOMs 😆

[alanwest]

Other than the minor dotnet-format issue, LGTM! One more minor thing, I think we can remove the docker-compose and Dockerfile from this test project now, right?

I'm also curious why test discovery is not working.. but this could just be a "me" problem..

[HofmeisterAn]

I'm also curious why test discovery is not working.. but this could just be a "me" problem..

Even though I use Windows most of the time, I do not use Visual Studio. While there is an issue with xUnit.net's Theory when used with Visual Studio, I do not believe this relates to this issue. Test discovery works fine in Rider (macOS and Windows).

[alanwest]

While there is an issue with xUnit.net's Theory when used with Visual Studio, I do not believe this relates to this issue. Test discovery works fine in Rider (macOS and Windows).

Yes, I think it's a different issue.

Initial test discovery works fine in Visual Studio as well, and if Docker is running then they also run fine. The issue appears when the tests are attempted without Docker running and they're skipped. If you then start up Docker and attempt to run the test, the 7 test cases have been reduced to 1 "skipped" test case.

I think this is related to the DisplayName attribute on the Fact and Theory attributes.

See: https://github.com/xunit/xunit/blob/7fa38ffc931b7ef1d0f2938ee0341aa050af2c96/src/xunit.v3.core/FactAttribute.cs#L15-L19.

Gets the name of the test to be used when the test is skipped.

The name for each of the test cases of the Theory is the same, so the test explorer reduces the test cases to a single one. Unfortunately, it does not seem you can override the DisplayName on a test case by test case basis.

All that said, I'm not too concerned by this and I don't see it as a blocker for this PR. You can force test discovery again after starting up Docker and things work fine.