Add Unit and Functional test CI pipeline - #4506
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new Azure DevOps pipeline (sqlclient-ci-unit) that runs the SqlClient Unit and Functional test suites in Package reference mode, consuming the exact NuGet artifacts produced by the triggering sqlclient-ci-package run. This follows the same downstream-consume-upstream-artifacts pattern introduced by the stacked PR (#4499) and extends it to broad Unit/Functional coverage across OS/TFM/SNI combinations.
Changes:
- Introduces a package-triggered pipeline definition that runs only on completion of
sqlclient-ci-package(no PR/CI triggers). - Defines a 3-stage OS matrix (Windows/Linux/macOS) with the intended TFM and SNI coverage.
- Adds a reusable job template that aligns source to the upstream commit, downloads/stages driver packages, runs Unit + Functional suites, and publishes results/artifacts.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| eng/pipelines/ci/unit/sqlclient-ci-unit-stages.yml | Defines the Windows/Linux/macOS runtime/SNI job matrix for the unit+functional test pipeline. |
| eng/pipelines/ci/unit/sqlclient-ci-unit-pipeline.yml | New pipeline entrypoint that is triggered by sqlclient-ci-package completion and invokes the stage matrix. |
| eng/pipelines/ci/unit/sqlclient-ci-unit-job.yml | Job template to align to upstream commit, consume produced packages, run Unit/Functional tests, and publish results. |
paulmedynski
added
the
Area\Engineering
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (2)
eng/pipelines/ci/unit/sqlclient-ci-unit-job.yml:100
- The comment says this step downloads the exact driver packages, but
sqlServerVersionOverride: 1.0.0means$(sqlServerPackageVersion)will be pinned instead of using the version produced by the triggeringsqlclient-ci-packagerun. That makes the current comment misleading and obscures why the override exists.
Update the comment to reflect the pin (or remove the override if the intent is to validate the exact SqlServer package from the upstream run).
# Download the exact driver packages produced by the triggering pipeline.
- template: /eng/pipelines/common/steps/download-driver-packages-step.yml@self
parameters:
sqlServerVersionOverride: 1.0.0
eng/pipelines/ci/unit/sqlclient-ci-unit-job.yml:120
update-config-file-step.ymlis being used only to setUseManagedSNIOnWindows, but because most parameters are omitted (so they default to empty/false), this step also overwrites the non-empty defaults inconfig.default.jsonc(e.g., TCP/NP connection strings andSupportsIntegratedSecurity=true). That unintentionally changes the baseline test config for this pipeline.
Pass through the defaults from config.default.jsonc so this step doesn’t clobber them while toggling SNI.
# Configure the test suite's Windows SNI implementation.
- template: /eng/pipelines/common/templates/steps/update-config-file-step.yml@self
parameters:
debug: ${{ parameters.debug }}
saPassword: ''
UseManagedSNIOnWindows: ${{ parameters.useManagedSNI }}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
eng/pipelines/ci/unit/sqlclient-ci-unit-job.yml:8
- The header comment claims this job runs against the exact packages from the triggering
sqlclient-ci-packagerun, but the job forcessqlServerPackageVersionviasqlServerVersionOverride: 1.0.0, soMicrosoft.SqlServer.Serveris not necessarily taken from the upstream artifact (NU1605 workaround). Please adjust the comment to reflect this exception to avoid misleading future maintainers.
# Builds and runs the SqlClient Unit and Functional test suites in Package reference mode against
# the exact packages produced by the triggering sqlclient-ci-package run.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
eng/pipelines/ci/unit/sqlclient-ci-unit-job.yml:107
- This job sets sqlServerVersionOverride: 1.0.0 while the adjacent comment says it downloads the “exact driver packages”. Since the override intentionally diverges from the artifact-resolved version (to avoid NU1605 downgrade warnings), add an in-file explanation (similar to managed-instance/stress jobs) so the behavior is clear.
# Download the exact driver packages produced by the triggering pipeline.
- template: /eng/pipelines/common/steps/download-driver-packages-step.yml@self
parameters:
sqlServerVersionOverride: 1.0.0
eng/pipelines/ci/unit/sqlclient-ci-unit-job.yml:7
- The header comment says this job runs against the “exact packages produced” by the upstream run, but this job pins Microsoft.SqlServer.Server via sqlServerVersionOverride (so at least that package version is not taken from the upstream artifact). Update the comment to reflect the exception so future readers aren’t misled.
This issue also appears on line 104 of the same file.
# Builds and runs the SqlClient Unit and Functional test suites in Package reference mode against
# the exact packages produced by the triggering sqlclient-ci-package run.
paulmedynski
assigned
mdaigle and
apoorvdeshmukh and unassigned
benrr101 and
cheenamalhotra
priyankatiwari08
left a comment
priyankatiwari08
left a comment
There was a problem hiding this comment.
Design looks right. Two non-blocking things:
1. Coverage is collected but never consumed. TestCodeCoverage defaults to true (build.proj:232) and sqlclient-ci-unit-job.yml doesn't override it, so all 13 legs pay the collector cost and publish .coverage artifacts — but this pipeline has no coverage stage. Either pass -p:TestCodeCoverage=false or wire in the merge stage.
2. Artifact name collides on job re-run. The shared publish template uses artifact: '${{ parameters.targetFramework }}WinAz$(System.JobId)'. System.JobId is stable across attempts, so re-running one failed leg fails with "Artifact ... already exists for build". eng/pipelines/pr/steps/publish-test-results-step.yml already appends $(System.JobAttempt) for exactly this reason. Pre-existing, but a 13-leg matrix is where it starts to bite.
Nit: the csprojs compare '$(UseManagedSniOnWindows)' == 'true' case-sensitively while build.proj uses .ToLower(), so a hand-run -p:UseManagedSniOnWindows=True would silently no-op.
The signed sqlclient-ci-package driver assemblies grant InternalsVisibleTo to test assemblies signed with the dedicated test key's public key. Download the sqlclient-test-key.snk secure file and expose it as TestSigningKeyPath so build.proj signs the unit-test assemblies accordingly.
Promote the target-framework loop from the managed-instance job up into the stage, so each OS/SNI x runtime combination runs as its own parallel job with a single Unit/Functional/Manual test pass (job/display names now include the TFM). Add a sqlServerVersionOverride parameter to the shared download-driver-packages step and pass 1.0.0 from the managed-instance and stress jobs, so restore uses the released stable Microsoft.SqlServer.Server instead of the -ci prerelease and avoids the NU1605 downgrade against Microsoft.SqlServer.Types' >= 1.0.0 dependency. Overall package versioning is being addressed separately.
Replace the update-config-file-step call in the unit job with a UseManagedSNIOnWindows build property that sets the Switch.Microsoft.Data.SqlClient.UseManagedNetworkingOnWindows AppContext switch in the test host's runtime configuration. config.jsonc is only read by ManualTests, the Azure extension tests, and PerformanceTests, so the previous invocation never toggled SNI for the Unit and Functional suites while still rewriting unrelated config fields with defaults. Setting the switch via runtimeconfig applies it at process start, before the driver caches its value. Also restrict the sqlclient-ci-package completion trigger to main and internal/main, matching the stress and Kerberos pipelines. Refs #4605
Rename the UseManagedSNIOnWindows build property to UseManagedSniOnWindows and the unit pipeline's useManagedSNI parameter to useManagedSni, matching the repository's camel-case convention for abbreviations in identifiers.
TestDefaultAppContextSwitchValues asserted that UseManagedNetworking is false on Windows, which fails in the managed SNI test leg now that the switch is enabled process-wide through runtimeconfig.json. Compare the property against AppContext instead, so the assertion still verifies that the switch is honoured and remains unchanged when the switch is not configured.
b5ac613
f2bebd4 to
b5ac613
Compare
…args Rename the test results artifact in the shared publish template to TFM_Platform_job$(System.JobId)_attempt$(System.JobAttempt). This drops the inaccurate "WinAz" infix, which was introduced with the initial YAML CI pipeline and claimed Windows even for Linux and macOS legs, and adds the job attempt number. System.JobId is stable across attempts, so re-running a failed leg previously failed with "Artifact ... already exists for build". Remove -p:PackageVersionAbstractions and -p:PackageVersionLogging from the Functional Tests task in the unit CI job. Neither variable is defined by download-driver-packages-step.yml, which only sets sqlClientPackageVersion and sqlServerPackageVersion, so the empty macros could override build.proj's versioning during restore.
|
@priyankatiwari08 — thanks for the review. Addressed in 583af94. Taking your three points in order: 1. Coverage is collected but never consumed. Intentional, keeping it on. There will never be a coverage merge/publish stage in this pipeline. The design is for every upstream CI pipeline to collect and publish its own 2. Artifact name collides on job re-run. Good catch, fixed. While in there I also dropped the e.g. 3. Nit: case-sensitive Separately, while checking the version arguments I found the Functional Tests task was passing |
There was a problem hiding this comment.
🟡 Changes recommended
The new UseManagedSniOnWindows MSBuild conditions in the test csproj files are case-sensitive and can silently fail to enable the runtimeconfig switch when the property is passed as a boolean-style value (e.g., True).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 11/12 changed files
- Comments generated: 2
- Review effort level: Lite
|
Ok, moved to 8.0.0-preview1. The entire sqlclient-ci-unit pipeline runs in 10 minutes, and it adds missing coverage for Managed SNI, so I think it's valuable to prioritize once the current set of releases is done. |
Description
Adds a package-triggered
sqlclient-ci-unitpipeline for the SqlClient Unit and Functional test suites.I decided not to bother adding separate pipelines for unit, functional, and simulated tests right now. We can decide if such a distinction is worth it later.
net462on Windows with native SNI.net8.0,net9.0, andnet10.0on Windows with native and managed SNI.net8.0,net9.0, andnet10.0on Linux and macOS with managed SNI.ADO-Win25,ADO-UB24, and the Microsoft-hostedmacos-latestimage.sqlclient-ci-packagerun.build.projdefault filter.SNI coverage
The Windows managed-SNI legs configure the driver to use
ManagedSni. The implementation is chosen byLocalAppContextSwitches.UseManagedNetworking, which reads theSwitch.Microsoft.Data.SqlClient.UseManagedNetworkingOnWindowsAppContext switch. This pipeline sets that switch through the test host'sruntimeconfig.jsonvia a newUseManagedSniOnWindowsbuild property, so it applies at process start, before the driver caches the value.Writing
config.jsoncdoes not work for these suites: that file is read only byDataTestUtility(ManualTests), the Azure extension tests, and PerformanceTests. This matters because the Unit suite holds the densest SNI coverage in the repo —SimulatedServerTestsopen real TCP connections to an in-processTdsServer, includingSNICloseDeadlockTest,SNICloseRaceDeadlockTest, andSNICloseHandshakeCancellationTest.The legacy CI and PR pipelines share the same limitation: they pass
UseManagedSNIOnWindowsintoconfig.jsonc, but run Unit, Functional, and Manual in a single job, so only the Manual portion actually switches implementations. They are intentionally left unchanged, because this new CI unit pipeline closes that gap for CI. The modern PR pipeline has no SNI dimension at all for these suites; that is tracked separately in #4605.Testing