dotnet / dotnet/SqlClient

Modern PR pipeline does not exercise managed SNI for Unit/Functional tests

Open
#4,605 1 comment 1 reaction 0 assignees View on GitHub
Area\Engineering
Dominant language
C#
Stars
989
Forks
340
Avg merge
4d 19h
Merged PRs (30d)
72

Description

## Summary

The modern PR pipeline (`eng/pipelines/pr/`) runs `TestSqlClientUnit` and `TestSqlClientFunctional` with **no managed/native SNI dimension at all**, so managed SNI on Windows is never exercised for these suites during PR validation.

## Background

`LocalAppContextSwitches.UseManagedNetworking` selects the SNI implementation in `TdsParserStateObjectFactory`, and also branches in `TdsParser` and `TdsParserStateObject`. It is controlled by the `Switch.Microsoft.Data.SqlClient.UseManagedNetworkingOnWindows` AppContext switch (Windows + .NET only; .NET Framework always uses native SNI, non-Windows always uses managed).

The Unit test suite contains the densest SNI coverage in the repo. `SimulatedServerTests` open real TCP connections to an in-process `TdsServer`, including tests written specifically for SNI behaviour:

- `SNICloseDeadlockTest`
- `SNICloseRaceDeadlockTest`
- `SNICloseHandshakeCancellationTest`
- plus SSPI, failover, routing, feature-negotiation, and TDS token-bounds tests

Functional tests also reach the SNI layer, though more lightly (e.g. `LocalizationTest` starts a `TdsServer` and opens a connection).

## Current state

| Pipeline | SNI dimension | Effective for Unit/Functional |
|---|---|---|
| `eng/pipelines/pr/` (modern PR) | none | no |
| `dotnet-sqlclient-ci-core.yml` (legacy CI) | `useManagedSNI: [false, true]` | no — see below |
| `sqlclient-pr-*-ref-pipeline.yml` (legacy PR) | `useManagedSNI` | no — see below |
| `sqlclient-ci-unit` (new) | `useManagedSNI` | yes, once dotnet/SqlClient#4506 merges |

The legacy pipelines pass `UseManagedSNIOnWindows` into `update-config-file-step.yml`, which writes `config.jsonc`. That file is only read by `DataTestUtility` (ManualTests), the Azure extension tests, and PerformanceTests. Because legacy jobs run Unit, Functional, and Manual in the same job via `run-all-tests-step.yml`, the toggle takes effect for the Manual portion only — the Unit and Functional runs in a "ManagedSNI" leg still execute against native SNI.

## Proposed fix

Mirror the approach landing in dotnet/SqlClient#4506 for the new `sqlclient-ci-unit` pipeline: set the AppContext switch through the test host's runtime configuration (`RuntimeHostConfigurationOption`) rather than through `config.jsonc`, driven by a `UseManagedSniOnWindows` build property threaded from the pipeline into `build.proj`.

For the modern PR pipeline specifically, add a managed-SNI dimension to the Windows Unit/Functional legs in `eng/pipelines/pr/stages/test-stages.yml` (via `test-buildproj-job.yml`), weighed against PR-validation runtime budget. A reduced matrix (for example, a single representative TFM) may be the right trade-off.

## Notes

- The legacy CI and PR pipelines are intentionally being left alone; the new `sqlclient-ci-unit` pipeline closes the coverage gap for CI.
- `net462` should be excluded, since .NET Framework only supports native SNI.
- This combination has never run in CI, so the first enabling run may surface latent failures.

## Related

- dotnet/SqlClient#4506

Contributor guide

Open the contributing guide

Research direction

Start with eng/pipelines/pr/stages/test-stages.yml and test-buildproj-job.yml, then compare the approach in dotnet/SqlClient#4506 and the sqlclient-ci-unit pipeline. Trace how UseManagedSniOnWindows reaches build.proj and the test host's RuntimeHostConfigurationOption; validate a Windows Unit/Functional run with managed SNI, excluding net462, and confirm the PR matrix exercises it.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
ci-cd, testing
Issue type
Feature
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.