Address Copilot review comments on PR 4242

- 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)

Author: paulmedynski

.github/instructions/testing.instructions.md

@@ -182,16 +182,16 @@ class and method level.
 - Add XML `<summary>` comments to every test class.
 - Add XML `<summary>` comments to every test method (`[Fact]`, `[Theory]`, conditional variants).
 - For helper methods used by tests, add XML `<summary>` comments and XML `<param>` / `<returns>`
-    where applicable.
+  where applicable.
 - For fixture and collection types, add XML `<summary>` comments describing why the fixture exists
-    (for example, serialization of console-mutating tests).
+  (for example, serialization of console-mutating tests).
 
 ### What the Comments Must Explain
 - The behavior/contract being tested (not just restating the method name).
 - Why the scenario matters (for example: regression guard, parsing contract, sync/async parity,
-    isolation requirement).
+  isolation requirement).
 - For helper methods, what side effects occur (for example console redirection, file system
-    copying, process execution) and why they are needed.
+  copying, process execution) and why they are needed.
 
 ### Style Guidance
 - Keep comments concise and factual.

tools/PackageCompatibility/Directory.Packages.props

@@ -17,7 +17,7 @@
   <!-- SqlClient Packages -->
   <ItemGroup>
     <!--
-      Normal apps wouldn't explcitily reference these 2 packages, but we do here to force the
+      Normal apps wouldn't explicitly reference these 2 packages, but we do here to force the
       desired versions.  This helps detect version conflicts.
     -->
     <PackageVersion Include="Microsoft.Data.SqlClient.Extensions.Abstractions" Version="$(AbstractionsVersion)" />

tools/PackageCompatibility/README.md

@@ -184,7 +184,7 @@ dotnet run \
   -p:AzureVersion=1.0.0 \
   -p:LoggingVersion=1.0.1 \
   -p:SqlClientVersion=7.1.0-preview1 \
-  -p:SqlServerVersio=1.0.0 \
+  -p:SqlServerVersion=1.0.0 \
   -- -c "<connection string>"
 ```
 

tools/PackageCompatibility/test/BuildVersionOverrideTests.cs

@@ -138,13 +138,21 @@ private static BuildArtifacts BuildAppWithProperties(Dictionary<string, string>
         var psi = new ProcessStartInfo
         {
             FileName = "dotnet",
-            Arguments = string.Join(" ", buildArgs),
             UseShellExecute = false,
             RedirectStandardOutput = true,
             RedirectStandardError = true,
             CreateNoWindow = true
         };
 
+#if NET
+        foreach (string buildArg in buildArgs)
+        {
+            psi.ArgumentList.Add(buildArg);
+        }
+#else
+        psi.Arguments = string.Join(" ", buildArgs.Select(QuoteArgument));
+#endif
+
         using (var process = Process.Start(psi))
         {
             if (process == null)
@@ -162,12 +170,21 @@ private static BuildArtifacts BuildAppWithProperties(Dictionary<string, string>
                 process.Kill();
                 throw new InvalidOperationException("Build timed out after 60 seconds");
             }
-#else
-            process.WaitForExit(60000);
-#endif
 
             stdout = process.StandardOutput.ReadToEnd();
             stderr = process.StandardError.ReadToEnd();
+#else
+            Task<string> stdoutTask = process.StandardOutput.ReadToEndAsync();
+            Task<string> stderrTask = process.StandardError.ReadToEndAsync();
+            bool completed = process.WaitForExit(60000);
+            stdout = stdoutTask.GetAwaiter().GetResult();
+            stderr = stderrTask.GetAwaiter().GetResult();
+            if (!completed)
+            {
+                process.Kill();
+                throw new InvalidOperationException("Build timed out after 60 seconds");
+            }
+#endif
 
             // Fail fast with full command/stdout/stderr context for easy diagnosis.
             if (process.ExitCode != 0)
@@ -219,7 +236,7 @@ private static void CopyDirectory(string sourceDirectory, string destinationDire
 
         foreach (string directory in Directory.GetDirectories(sourceDirectory, "*", SearchOption.AllDirectories))
         {
-            string relativePath = Path.GetRelativePath(sourceDirectory, directory);
+            string relativePath = GetRelativePath(sourceDirectory, directory);
             if (ShouldSkip(relativePath))
             {
                 continue;
@@ -230,7 +247,7 @@ private static void CopyDirectory(string sourceDirectory, string destinationDire
 
         foreach (string file in Directory.GetFiles(sourceDirectory, "*", SearchOption.AllDirectories))
         {
-            string relativePath = Path.GetRelativePath(sourceDirectory, file);
+            string relativePath = GetRelativePath(sourceDirectory, file);
             if (ShouldSkip(relativePath))
             {
                 continue;
@@ -251,7 +268,7 @@ private static bool ShouldSkip(string relativePath)
     {
         // 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(Path.DirectorySeparatorChar, StringSplitOptions.RemoveEmptyEntries);
+        string[] segments = normalizedPath.Split(new[] { Path.DirectorySeparatorChar }, StringSplitOptions.RemoveEmptyEntries);
 
         foreach (string segment in segments)
         {
@@ -308,4 +325,42 @@ private static string GetPackageCompatibilityDirectory()
     /// <param name="OutputDirectory">Build output directory for binaries.</param>
     /// <param name="GeneratedVersionsFile">Path to generated <c>PackageVersions.g.cs</c>.</param>
     private sealed record BuildArtifacts(string ProjectDirectory, string OutputDirectory, string GeneratedVersionsFile);
+
+#if !NET
+    /// <summary>
+    /// Quotes a single command-line argument for use with <see cref="System.Diagnostics.ProcessStartInfo.Arguments"/>
+    /// on .NET Framework, where <c>ArgumentList</c> is unavailable.
+    /// Wraps the value in double-quotes and escapes any embedded double-quotes.
+    /// </summary>
+    /// <param name="arg">The argument to quote.</param>
+    /// <returns>The argument, quoted if it contains whitespace or double-quote characters.</returns>
+    private static string QuoteArgument(string arg)
+    {
+        if (arg.IndexOfAny(new[] { ' ', '\t', '"' }) < 0)
+        {
+            return arg;
+        }
+
+        return "\"" + arg.Replace("\\", "\\\\").Replace("\"", "\\\"") + "\"";
+    }
+#endif
+
+    /// <summary>
+    /// Returns the relative path from <paramref name="relativeTo"/> to <paramref name="path"/>.
+    /// Polyfills <see cref="Path.GetRelativePath"/> which is unavailable on .NET Framework.
+    /// </summary>
+    private static string GetRelativePath(string relativeTo, string path)
+    {
+#if NET
+        return Path.GetRelativePath(relativeTo, path);
+#else
+        // Ensure the base URI ends with a separator so MakeRelativeUri treats it as a directory.
+        string baseStr = relativeTo.TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar)
+            + Path.DirectorySeparatorChar;
+        Uri baseUri = new Uri(baseStr);
+        Uri targetUri = new Uri(path);
+        return Uri.UnescapeDataString(baseUri.MakeRelativeUri(targetUri).ToString())
+            .Replace('/', Path.DirectorySeparatorChar);
+#endif
+    }
 }

tools/PackageCompatibility/test/EntryPointTests.cs

@@ -28,6 +28,7 @@ public void EntryPointWithoutConnectionStringReturnsNonZero()
         using StringWriter output = new();
 
         int exitCode;
+        string commandOutput;
 
         try
         {
@@ -36,6 +37,9 @@ public void EntryPointWithoutConnectionStringReturnsNonZero()
 
             // Act: invoke with no arguments; --connection-string is required.
             exitCode = EntryPoint.Main(Array.Empty<string>());
+
+            // Wait for command help/validation text to flush before asserting.
+            commandOutput = WaitForCapturedOutput(output, "--connection-string");
         }
         finally
         {
@@ -44,9 +48,6 @@ public void EntryPointWithoutConnectionStringReturnsNonZero()
             Console.SetError(originalError);
         }
 
-        // Wait for command help/validation text to flush before asserting.
-        string commandOutput = WaitForCapturedOutput(output, "--connection-string");
-
         // Assert: parser should reject the command and mention the missing option.
         Assert.NotEqual(0, exitCode);
         Assert.Contains("--connection-string", commandOutput, StringComparison.Ordinal);
@@ -75,7 +76,8 @@ public void HelpOutputContainsDefaultVersions()
             int exitCode = EntryPoint.Main(new[] { "--help" });
             Assert.Equal(0, exitCode);
 
-            helpOutput = output.ToString();
+            // Ensure final help footer is present so all formatted content has been emitted.
+            helpOutput = WaitForCapturedOutput(output, "--version");
         }
         finally
         {
@@ -84,9 +86,6 @@ public void HelpOutputContainsDefaultVersions()
             Console.SetError(originalError);
         }
 
-        // Ensure final help footer is present so all formatted content has been emitted.
-        helpOutput = WaitForCapturedOutput(output, "--version");
-
         // Assert: sample property overrides remain documented in help text.
         Assert.Contains("-p:AbstractionsVersion=1.0.1", helpOutput, StringComparison.Ordinal);
         Assert.Contains("-p:AkvProviderVersion=7.0.1-preview2", helpOutput, StringComparison.Ordinal);
@@ -96,12 +95,12 @@ public void HelpOutputContainsDefaultVersions()
         Assert.Contains("-p:SqlServerVersion=1.0.0", helpOutput, StringComparison.Ordinal);
 
         // Assert: currently resolved package defaults are visible to aid troubleshooting.
-        Assert.Contains("Abstractions:  1.0.0", helpOutput, StringComparison.Ordinal);
-        Assert.Contains("AKV Provider:  7.0.0", helpOutput, StringComparison.Ordinal);
+        Assert.Contains($"Abstractions:  {PackageVersions.MicrosoftDataSqlClientExtensionsAbstractions}", helpOutput, StringComparison.Ordinal);
+        Assert.Contains($"AKV Provider:  {PackageVersions.MicrosoftDataSqlClientAlwaysEncryptedAzureKeyVaultProvider}", helpOutput, StringComparison.Ordinal);
         Assert.Contains("Azure:         N/A", helpOutput, StringComparison.Ordinal);
-        Assert.Contains("Logging:       1.0.0", helpOutput, StringComparison.Ordinal);
-        Assert.Contains("SqlClient:     7.0.1", helpOutput, StringComparison.Ordinal);
-        Assert.Contains("SqlServer:     1.0.0", helpOutput, StringComparison.Ordinal);
+        Assert.Contains($"Logging:       {PackageVersions.MicrosoftDataSqlClientInternalLogging}", helpOutput, StringComparison.Ordinal);
+        Assert.Contains($"SqlClient:     {PackageVersions.MicrosoftDataSqlClient}", helpOutput, StringComparison.Ordinal);
+        Assert.Contains($"SqlServer:     {PackageVersions.MicrosoftSqlServerServer}", helpOutput, StringComparison.Ordinal);
     }
 
     /// <summary>

tools/PackageCompatibility/test/IsExternalInit.cs

@@ -0,0 +1,13 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+// Polyfill required for 'record' types and 'init' setters on .NET Framework.
+// The compiler emits a reference to this type when using these C# features; .NET 5+
+// includes it in the BCL, but .NET Framework does not.
+#if NETFRAMEWORK
+namespace System.Runtime.CompilerServices
+{
+    internal static class IsExternalInit { }
+}
+#endif