Move package compatibility app to tools and add robust test coverage#4242
Move package compatibility app to tools and add robust test coverage#4242paulmedynski wants to merge 7 commits intomainfrom
Conversation
…zureAuthentication app - Add LoggingVersion and AbstractionsVersion MSBuild properties (default 1.0.0) - Add PackageVersion entries for Microsoft.Data.SqlClient.Internal.Logging and Microsoft.Data.SqlClient.Extensions.Abstractions - Reference both packages explicitly in the project to surface their resolved versions in the generated PackageVersions class - Emit Logging and Abstractions versions alongside the other package versions in App.cs and EntryPoint.cs startup output - Rename PackageVersions.AzureExtensionsVersion to MicrosoftDataSqlClientExtensionsAzureVersion to match the naming pattern used by all other generated version constants - Update README with new build parameters, help-text examples, and sample output
The primary purpose of this app is to test package compatibility, not specifically Azure authentication, so the name now better reflects its purpose. - git mv doc/apps/AzureAuthentication/ tools/PackageCompatibility/ - Rename AzureAuthentication.csproj to PackageCompatibility.csproj - Update RootNamespace and all source namespaces to Microsoft.Data.SqlClient.Tools.PackageCompatibility - Rename AppName constant to 'Package Compatibility Tester' - Update README title, help-text block, and sample output accordingly
- add explicit Microsoft.SqlServer.Server version support and output - keep PackageCompatibility package/version ordering as currently arranged - update help text and sample output for current package version set - expand README guidance around package compatibility and auth flows
- Added support to specify all SqlClient suite package versions. - Added a modern xUnit3 test suite.
There was a problem hiding this comment.
Pull request overview
This PR relocates the package-compatibility smoke app into tools/PackageCompatibility and adds an xUnit v3 test suite to validate CLI behavior, App.Run error paths, and MSBuild package-version override/codegen output.
Changes:
- Moved/renamed the compatibility tool and updated namespaces/solution wiring.
- Added MSBuild-driven
PackageVersions.g.csgeneration plus expanded version override support. - Added xUnit v3 tests covering CLI required-args/help output, malformed connection handling, and
-p:override propagation.
Reviewed changes
Copilot reviewed 17 out of 21 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/PackageCompatibility/src/PackageCompatibility.csproj | Renames root namespace, adds package references, and grants InternalsVisibleTo to the new test project. |
| tools/PackageCompatibility/src/App.cs | Updates banner/package reporting and app name; keeps connectivity test behavior. |
| tools/PackageCompatibility/src/EntryPoint.cs | Updates help text and prints additional resolved package versions. |
| tools/PackageCompatibility/src/GeneratePackageVersions.targets | Adds MSBuild target + Roslyn task to generate PackageVersions.g.cs from resolved package versions. |
| tools/PackageCompatibility/src/PackageVersions.Partial.cs | Renames the Azure “N/A” constant and aligns namespace with new tool location. |
| tools/PackageCompatibility/src/SqlClientEventListener.cs | Namespace update to match the new tool location. |
| tools/PackageCompatibility/test/PackageCompatibility.Test.csproj | New xUnit v3 test project targeting net481/net10.0. |
| tools/PackageCompatibility/test/EntryPointTests.cs | New tests for required args and --help output contracts. |
| tools/PackageCompatibility/test/AppTests.cs | New tests for malformed connection-string handling and log-events banner ordering. |
| tools/PackageCompatibility/test/BuildVersionOverrideTests.cs | New hermetic build test validating -p: overrides are reflected in generated version constants. |
| tools/PackageCompatibility/test/ConsoleCollection.cs | Adds a non-parallel xUnit collection for console-mutating tests. |
| tools/PackageCompatibility/Directory.Build.props | Enables CPM for the tool subtree. |
| tools/PackageCompatibility/Directory.Packages.props | Tool-local CPM versions + MSBuild override defaults for package versions. |
| tools/PackageCompatibility/NuGet.config | Adds local packages/ feed for unpublished nupkgs. |
| tools/PackageCompatibility/global.json | Pins SDK and forces Microsoft.Testing.Platform runner within the subtree. |
| tools/PackageCompatibility/README.md | New tool documentation and usage examples. |
| tools/PackageCompatibility/packages/.gitkeep | Ensures packages/ exists for the local NuGet feed. |
| src/Microsoft.Data.SqlClient.slnx | Removes old doc app and adds the tool + test projects to the solution. |
| doc/apps/AzureAuthentication/README.md | Removes the old doc sample app README. |
| doc/apps/AzureAuthentication/Directory.Packages.props | Removes old doc sample’s CPM file. |
| .github/instructions/testing.instructions.md | Adds guidance requiring XML docs on tests/helpers/fixtures. |
Comments suppressed due to low confidence (1)
tools/PackageCompatibility/src/PackageCompatibility.csproj:8
- The project targets net481 unconditionally. This can break cross-platform builds/solution loading on non-Windows hosts (net48* TFMs aren't supported there). The repo convention is to add netfx TFMs only when building on Windows (e.g., src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft.Data.SqlClient.UnitTests.csproj:12-19). Consider making net481 conditional on '$(OS)' == 'Windows_NT' (or dropping it if not required).
- Fix net481 build: add IsExternalInit polyfill, GetRelativePath helper, and Split overload compat for net481 in BuildVersionOverrideTests - Use PackageVersions constants instead of hard-coded version strings in EntryPointTests.HelpOutputContainsDefaultVersions (fix #1) - Fix WaitForExit deadlock risk on net481: read stdout/stderr async before waiting; check timeout bool and kill/throw on expiry (fix #2) - Fix Directory.Packages.props typo: explcitily -> explicitly (fix #4) - Fix README.md typo: -p:SqlServerVersio= -> -p:SqlServerVersion= (fix #5) - Move WaitForCapturedOutput inside try block so late console output is captured before streams are restored (fix #6) - Use ArgumentList on NET and QuoteArgument helper on net481 so paths with spaces are passed correctly to dotnet build (fix #7) - Fix testing.instructions.md list continuation indents (4-space -> 2-space) so GitHub renders them as wrapped text not code blocks (fix #8)
| @@ -0,0 +1,197 @@ | |||
| # PackageCompatibility Tool | |||
There was a problem hiding this comment.
How is it different than running Manual Tests in package reference mode? What's missing in Manual tests (package reference) that this tool will cover?
There was a problem hiding this comment.
Added an explanation in the README.
… running manual tests.
| // Ignore generated artifacts and the test project itself to avoid recursive/self-copy issues. | ||
| string normalizedPath = relativePath.Replace(Path.AltDirectorySeparatorChar, Path.DirectorySeparatorChar); | ||
| string[] segments = normalizedPath.Split(new[] { Path.DirectorySeparatorChar }, StringSplitOptions.RemoveEmptyEntries); | ||
|
|
||
| foreach (string segment in segments) | ||
| { | ||
| if (segment.Equals("bin", StringComparison.Ordinal) | ||
| || segment.Equals("obj", StringComparison.Ordinal) | ||
| || segment.Equals("test", StringComparison.Ordinal)) | ||
| { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; |
There was a problem hiding this comment.
This test copies the entire tools/PackageCompatibility subtree to a temp folder, but ShouldSkip doesn’t exclude packages/. If developers have large local .nupkg files there, each test run may copy a lot of data and slow down significantly. Consider skipping packages/ contents during copy (while still creating an empty packages/ directory in the temp workspace so NuGet.config remains valid).
| private static BuildArtifacts BuildAppWithProperties(Dictionary<string, string> properties) | ||
| { | ||
| // Locate and clone the package-compatibility subtree so test builds are hermetic. | ||
| string packageCompatibilityDir = GetPackageCompatibilityDirectory(); | ||
| string tempProjectDir = Path.Combine(Path.GetTempPath(), $"PackageCompatibilityProject_{Guid.NewGuid():N}"); | ||
| string tempOutputDir = Path.Combine(Path.GetTempPath(), $"PackageCompatibility_{Guid.NewGuid():N}"); |
There was a problem hiding this comment.
BuildAppWithProperties creates temp project/output directories but never cleans them up on success or failure. Over time this can accumulate in %TEMP% and make failures harder to diagnose. Consider wrapping the build + readback in a try/finally that deletes the temp directories (optionally preserving them when a test fails, e.g., via an env var).
| private static BuildArtifacts BuildAppWithProperties(Dictionary<string, string> properties) | |
| { | |
| // Locate and clone the package-compatibility subtree so test builds are hermetic. | |
| string packageCompatibilityDir = GetPackageCompatibilityDirectory(); | |
| string tempProjectDir = Path.Combine(Path.GetTempPath(), $"PackageCompatibilityProject_{Guid.NewGuid():N}"); | |
| string tempOutputDir = Path.Combine(Path.GetTempPath(), $"PackageCompatibility_{Guid.NewGuid():N}"); | |
| private static bool ShouldPreserveTempBuildDirectories() | |
| { | |
| string? preserve = Environment.GetEnvironmentVariable("PACKAGECOMPATIBILITY_PRESERVE_TEMP"); | |
| return string.Equals(preserve, "1", StringComparison.OrdinalIgnoreCase) | |
| || string.Equals(preserve, "true", StringComparison.OrdinalIgnoreCase); | |
| } | |
| private static void TryDeleteDirectory(string path) | |
| { | |
| if (string.IsNullOrEmpty(path) || !Directory.Exists(path)) | |
| { | |
| return; | |
| } | |
| try | |
| { | |
| Directory.Delete(path, recursive: true); | |
| } | |
| catch (IOException) | |
| { | |
| } | |
| catch (UnauthorizedAccessException) | |
| { | |
| } | |
| } | |
| private static BuildArtifacts BuildAppWithProperties(Dictionary<string, string> properties) | |
| { | |
| // Locate and clone the package-compatibility subtree so test builds are hermetic. | |
| string packageCompatibilityDir = GetPackageCompatibilityDirectory(); | |
| string tempRootDir = Path.Combine(Path.GetTempPath(), $"PackageCompatibility_{Guid.NewGuid():N}"); | |
| string tempProjectDir = Path.Combine(tempRootDir, "project"); | |
| string tempOutputDir = Path.Combine(tempRootDir, "output"); | |
| bool preserveTempBuildDirectories = ShouldPreserveTempBuildDirectories(); | |
| Directory.CreateDirectory(tempRootDir); |
| ## Writing Tests | ||
|
|
||
| ## Test Documentation Requirements | ||
|
|
There was a problem hiding this comment.
Markdown structure: ## Test Documentation Requirements is currently the same heading level as ## Writing Tests, so it renders as a sibling section rather than a subsection. Consider changing it to ### Test Documentation Requirements (and adjusting subsequent headings accordingly) so the new guidance is clearly nested under "Writing Tests".
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4242 +/- ##
==========================================
- Coverage 65.95% 64.33% -1.63%
==========================================
Files 277 272 -5
Lines 42989 65783 +22794
==========================================
+ Hits 28354 42320 +13966
- Misses 14635 23463 +8828
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary
doc/apps/AzureAuthenticationtotools/PackageCompatibility..github/instructions/testing.instructions.md.Azure.Corepackage version from the local CPM file.Validation
dotnet test -f net10.0intools/PackageCompatibility/test(pass: 8/8)Notes
tools/PackageCompatibility/global.jsonto force Microsoft.Testing.Platform runner for local xUnit v3 compatibility on net10.Maintainer checklist