-
Notifications
You must be signed in to change notification settings - Fork 317
[5.1] Update Dependencies & Fix Build Issues #3754
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: release/5.1
Are you sure you want to change the base?
[5.1] Update Dependencies & Fix Build Issues #3754
Conversation
- Updated key dependencies to the latest versions that don't trigger major version bumps. - Updated xUnit packages and made associated test changes for modernization. - Updated .NET Framework project to build on Linux.
- Fixed build issues that only appear when building on Windows.
- Removed ancient obsolete copy of XunitExtensions project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR modernizes the build system and test infrastructure for Microsoft.Data.SqlClient by updating dependencies, migrating to SDK-style projects, and improving test code quality.
Key Changes
- Dependency Updates: Azure.Identity (1.11.4 → 1.12.1), MSAL (4.61.3 → 4.78.0), Azure.Core (version ranges → 1.45.0), and test framework packages (xUnit 2.4.2 → 2.9.3, .NET Test SDK 17.4.1 → 17.12.0)
- .NET Framework Project Modernization: Migrated netfx/src project to SDK-style format, removing legacy COM references and adding proper .NET Framework reference assemblies support
- Test Code Improvements: Replaced
Assert.False(true, message)anti-pattern withAssert.Fail(message), updated ActiveIssue attributes to use string parameters, and converted synchronous tests to properly async methods
Reviewed Changes
Copilot reviewed 93 out of 94 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/props/Versions.props | Updated dependency versions across Azure SDK, MSAL, xUnit, and test infrastructure packages; changed Azure package versions from ranges to fixed versions |
| src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj | Migrated to SDK-style project format, removed COM references to mscoree, added Microsoft.NETFramework.ReferenceAssemblies support |
| src/Microsoft.Data.SqlClient/tests/ManualTests/*.cs | Modernized test assertions (Assert.Fail, proper async/await), updated ActiveIssue attributes to string format |
| src/Microsoft.Data.SqlClient/tests/FunctionalTests/*.cs | Improved test assertions (Assert.Empty, Assert.Single), converted tests to async, added AssertExtensions and PlatformDetection helpers |
| src/Microsoft.Data.SqlClient/tests/**/Microsoft.Data.SqlClient.*.Tests.csproj | Added xUnit and Microsoft.DotNet.XUnitExtensions package references, replaced project references with packages |
| src/Microsoft.Data.SqlClient/src/*/ActiveDirectoryAuthenticationProvider.cs | Added pragma warnings to suppress CS0618 for obsolete MSAL API usage |
| src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj | Removed explicit Microsoft.Win32.Registry reference, added Azure.Core package reference |
| src/Microsoft.Data.SqlClient.sln | Removed Microsoft.DotNet.XUnitExtensions project reference and dependencies |
| build.proj | Commented out Microsoft.DotNet.XUnitExtensions project from build targets |
| src/Directory.Build.props | Added MSBuild workaround for synthetic project references and suppressed NETSDK1138 warning for .NET 6.0 |
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/LocalDBTest/LocalDBTest.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ExceptionTest/ConnectionExceptionTest.cs
Show resolved
Hide resolved
...icrosoft.Data.SqlClient/tests/FunctionalTests/BaseProviderAsyncTest/BaseProviderAsyncTest.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release/5.1 #3754 +/- ##
===============================================
- Coverage 72.29% 70.35% -1.94%
===============================================
Files 293 293
Lines 61612 61928 +316
===============================================
- Hits 44544 43572 -972
- Misses 17068 18356 +1288
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:
|
- Fixed ManualTests SqlBulkCopy file inclusion issues.
- Fixed MDS nuspec to look for net462 artifacts in the correct place. - Reverted SqlBulkCopy tests Helpers namespace change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 96 out of 97 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (2)
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/LocalDBTest/LocalDBTest.cs:147
- Hardcoding the enum value
(TargetFrameworkMonikers)0x4instead of using the named enum memberTargetFrameworkMonikers.Uapreduces code readability. The original code was clear about the intent. Since theTargetFrameworkMonikersenum is being removed from the local XUnit extensions and now comes from the NuGet package, verify that the value0x4corresponds to the correct enum member in the new package version.
src/Microsoft.Data.SqlClient/tests/FunctionalTests/AssertExtensions.cs:317 - The exception type changed from
AssertActualExpectedExceptiontoEqualException, and the exception creation method changed from constructor toEqualException.ForMismatchedStrings(). The new method requires index parameters (0, 0) that may not accurately represent where the mismatch occurred in the byte arrays. Consider calculating the actual mismatch index to provide more accurate error messages.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlBulkCopyTest/Helpers.cs
Outdated
Show resolved
Hide resolved
- Fixed Helpers namespace for real. - Added sunit.runner.json config to avoid XUnitExtensions strong name errors.
- Fixed SNI DLL copying for .NET Framework test projects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 96 out of 97 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
src/Microsoft.Data.SqlClient/tests/FunctionalTests/AssertExtensions.cs:317
- The
EqualException.ForMismatchedStringscall is missing the expected and actual array lengths which are required parameters. The correct signature should include the index position and lengths:EqualException.ForMismatchedStrings(expectedString, actualString, indexOfFirstDifference, indexOfFirstDifference)or similar. Please verify the correct overload for xUnit 2.9.3 and provide appropriate parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
paulmedynski
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commentary for reviewers.
| <ItemGroup Condition="'$(TargetFramework)' == 'netstandard2.0'"> | ||
| <PackageReference Include="System.Buffers" Version="$(SystemBuffersVersion)" /> | ||
| </ItemGroup> | ||
| <ItemGroup Condition="'$(TargetsWindows)' == 'true' and '$(IsUAPAssembly)' != 'true'"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're already including the NuGet package on line 31 for netstandard. Removing this fixed a duplication error for that target, and didn't seem to affect the others.
| </ItemGroup> | ||
| <ItemGroup> | ||
| <PackageReference Include="Azure.Identity" Version="$(AzureIdentityVersion)" /> | ||
| <PackageReference Include="Azure.Core" Version="$(AzureCoreVersion)" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Necessary to avoid a transitive vulnerability.
| </ItemGroup> | ||
| <ItemGroup> | ||
| <PackageReference Include="Azure.Core" Version="$(AzureCoreVersion)" /> | ||
| <PackageReference Include="System.Private.Uri" Version="$(SystemPrivateUriVersion)" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Necessary to avoid a transitive vulnerability.
| <?xml version="1.0" encoding="utf-8"?> | ||
| <Project ToolsVersion="Current" xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> | ||
| <Import Project="$(MSBuildExtensionsPath)\$(MSBuildToolsVersion)\Microsoft.Common.props" Condition="Exists('$(MSBuildExtensionsPath)\$(MSBuildToolsVersion)\Microsoft.Common.props')" /> | ||
| <Project Sdk="Microsoft.NET.Sdk"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh boy - this was an old-style project. Converted it to modern SDK style.
| <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory> | ||
| </EmbeddedResource> | ||
| </ItemGroup> | ||
| <ItemGroup> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From ancient times; unused here.
| <Compile Include="SQL\RetryLogic\SqlConfigurationManagerReliabilityTest.cs" /> | ||
| <Compile Include="SQL\SqlBulkCopyTest\AdjustPrecScaleForBulkCopy.cs" /> | ||
| <Compile Include="SQL\SqlBulkCopyTest\AzureDistributedTransaction.cs" /> | ||
| <Compile Include="SQL\SqlBulkCopyTest\ErrorOnRowsMarkedAsDeleted.cs" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorted all of these SqlBulkCopyTest files alphabetically.
| <ItemGroup> | ||
| <PackageReference Include="Newtonsoft.Json" Version="$(NewtonsoftJsonVersion)" /> | ||
| <PackageReference Include="Microsoft.SqlServer.SqlManagementObjects" Version="$(MicrosoftSqlServerSqlManagementObjectsVersion)" /> | ||
| <PackageReference Include="System.Formats.Asn1" Version="$(SystemFormatsAsn1Version)" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid transitive vulnerability.
| "$schema": "https://xunit.net/schema/current/xunit.runner.schema.json", | ||
| "diagnosticMessages": true, | ||
| "parallelizeAssembly": true, | ||
| "shadowCopy": false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed this to avoid .NET Framework test failures refusing to load xUnit DLLs.
| <dependency id="Azure.Core" version="1.45.0" /> | ||
| <dependency id="Azure.Identity" version="1.12.1" /> | ||
| <dependency id="Microsoft.Identity.Client" version="4.78.0" /> | ||
| <dependency id="Microsoft.IdentityModel.JsonWebTokens" version="6.35.0" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't update these to 6.36.0 because then include major version bumps for transitive dependencies.
| <file src="..\..\artifacts\Project\bin\Windows_NT\$Configuration$.AnyCPU\Microsoft.Data.SqlClient\netfx\zh-Hans\Microsoft.Data.SqlClient.resources.dll" target="lib\net462\zh-Hans\" exclude="" /> | ||
| <file src="..\..\artifacts\Project\bin\Windows_NT\$Configuration$.AnyCPU\Microsoft.Data.SqlClient\netfx\zh-Hant\Microsoft.Data.SqlClient.resources.dll" target="lib\net462\zh-Hant\" exclude="" /> | ||
|
|
||
| <file src="..\..\artifacts\Project\bin\Windows_NT\$Configuration$.AnyCPU\Microsoft.Data.SqlClient\netfx\net462\de\Microsoft.Data.SqlClient.resources.dll" target="lib\net462\de\" exclude="" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added net462\ to these paths since that's where the new .NET Framework SDK project puts them.
- Whoops - forget to add the new target file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 97 out of 98 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
src/Microsoft.Data.SqlClient/tests/FunctionalTests/AssertExtensions.cs:317
- The change from
AssertActualExpectedExceptiontoEqualExceptionis correct for xUnit 2.9.3, but the constructor callEqualException.ForMismatchedStrings(expectedString, actualString, 0, 0)may not work as intended. The last two parameters (0, 0) represent the index and pointer position in the strings. This should likely use actual mismatch positions or a different factory method. Consider using justnew EqualException(expectedString, actualString)if the simpler constructor is available.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ExceptionTest/ConnectionExceptionTest.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConfigurableRetryLogicTest.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/LocalDBTest/LocalDBTest.cs
Show resolved
Hide resolved
- Pipeline tweaks in preparation for the official 5.1.8 build.
paulmedynski
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commentary for reviewers.
| # See the LICENSE file in the project root for more information. # | ||
| ################################################################################# | ||
| parameters: | ||
| - name: mdsPackageVersion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer template parameters to variables with no provenance.
| stages: | ||
| - stage: buildAKV | ||
| displayName: 'Build AKV Provider' | ||
| - stage: buildMDS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-arranged to build the MDS package before AKV, since the latter depends on the former and it makes sense to see it this way in the pipeline.
- Fixed variable -> parameter that I missed earlier. - Added target framework to paths for .NET Framework DLL copying steps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 102 out of 103 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
src/Microsoft.Data.SqlClient/tests/FunctionalTests/AssertExtensions.cs:313
- The catch block catches EqualException and the throw statement calls EqualException.ForMismatchedStrings(). This change suggests migration to a newer xUnit version where AssertActualExpectedException was replaced with EqualException. However, the ForMismatchedStrings method signature with four parameters (two index parameters at the end) might not be correct. Verify that EqualException.ForMismatchedStrings accepts these parameters in xUnit 2.9.3, as this could cause runtime errors.
src/Microsoft.Data.SqlClient/tests/FunctionalTests/AssertExtensions.cs:317 - The catch block catches EqualException and the throw statement calls EqualException.ForMismatchedStrings(). This change suggests migration to a newer xUnit version where AssertActualExpectedException was replaced with EqualException. However, the ForMismatchedStrings method signature with four parameters (two index parameters at the end) might not be correct. Verify that EqualException.ForMismatchedStrings accepts these parameters in xUnit 2.9.3, as this could cause runtime errors.
src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionTest.RetrieveStatistics.cs
Show resolved
Hide resolved
- Ignoring ADO Library variables for NuGet and assembly versioning.
paulmedynski
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commentary for reviewers.
| if ($tf.StartsWith('net4')) | ||
| { | ||
| Copy-Item "artifacts\${{parameters.referenceType }}\bin\Windows_NT\${{parameters.Configuration }}.AnyCPU\Microsoft.Data.SqlClient\netfx\Microsoft.Data.SqlClient.dll" "$software\win\$tf" -recurse | ||
| Copy-Item "artifacts\${{parameters.referenceType }}\bin\Windows_NT\${{parameters.Configuration }}.AnyCPU\Microsoft.Data.SqlClient\netfx\$tf\Microsoft.Data.SqlClient.dll" "$software\win\$tf" -recurse |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The updated .NET Framework project publishes artifacts into a new sub-directory using the target framework, similar to the .NET project. A side-effect of converting it to a modern SDK style prokject.
| - group: AKV Release Variables | ||
| - name: AKVMajor | ||
|
|
||
| # The above Library contains variables named AKVMajor, AKVMinor, and AKVPatch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've had enough of the problems trying to figure out where the version values come from. Now they come only from these YAML files.
mdaigle
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally would have liked to see this as three separate PRs: build improvements, xunit updates, and version bumps
| <Compile Include="Microsoft\Data\SqlClient\TdsParserStateObjectFactory.Managed.cs" /> | ||
| <Compile Include="Microsoft\Data\SqlClient\TdsParser.Unix.cs" /> | ||
| </ItemGroup> | ||
| <ItemGroup Condition="'$(TargetsWindows)' == 'true' and '$(IsUAPAssembly)' != 'true'"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove this here because it only applies to netstandard, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct - removing this had no effect on the other target frameworks. It is already included as a package ref for .NET Standard on line 961.
...ts/FunctionalTests/AlwaysEncryptedTests/SqlColumnEncryptionCertificateStoreProviderShould.cs
Show resolved
Hide resolved
|
|
||
| [Fact] | ||
| [SkipOnTargetFramework(~TargetFrameworkMonikers.Uap)] | ||
| [SkipOnTargetFramework(~(TargetFrameworkMonikers)0x4)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment here to capture that? for history :)
Agreed - this one got away from me! |
Description
Testing
Existing CI should find any problems.