Create app context switch test helper #3371
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new test helper—LocalAppContextSwitchesHelper—to safely capture and restore global app context switches using the RAII pattern. The changes update various test projects to utilize the helper instead of direct reflection-based switch manipulation, refactor the TdsParserStateObject to dispose of switch state correctly, and adjust some test inline data to match the new API.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/ManualTests/SQL/DataReaderTest/DataReaderTest.cs | Replaced direct switch handling with the helper to ensure proper restoration. |
| tests/ManualTests/Microsoft.Data.SqlClient.ManualTesting.Tests.csproj | Added compilation reference for the new helper. |
| tests/FunctionalTests/TdsParserStateObject.TestHarness.cs | Refactored to use a disposable helper instance and implemented IDisposable. |
| tests/FunctionalTests/SqlParameterTest.cs | Updated test inline data and usage of the helper for switch configuration. |
| tests/FunctionalTests/MultiplexerTests.cs | Converted state object instantiation to a using statement for proper disposal. |
| tests/FunctionalTests/Microsoft.Data.SqlClient.Tests.csproj | Included the helper compilation. |
| tests/FunctionalTests/LocalAppContextSwitchesTests.cs | Amended inline test data to cover new switch properties. |
| tests/Common/LocalAppContextSwitchesHelper.cs | Introduced the new helper class implementation. |
Comments suppressed due to low confidence (2)
src/Microsoft.Data.SqlClient/tests/FunctionalTests/TdsParserStateObject.TestHarness.cs:161
- [nitpick] Consider renaming the 'LocalAppContextSwitches' field to clearly indicate it is an instance of the helper rather than the original static switch holder.
private SwitchesHelper LocalAppContextSwitches = new();
src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlParameterTest.cs:1947
- The removal of test cases using null switch values changes the expected behavior. Please verify that omitting these cases is intentional and that the documentation and coverage adequately address the default switch behavior.
[InlineData(null, 7, null)]
paulmedynski
added
the
Area\Tests
| @@ -1945,89 +1947,29 @@ private enum Int64Enum : long | |||
| [InlineData(5, 5, false)] | |||
| [InlineData(6, 6, false)] | |||
| [InlineData(7, 7, false)] | |||
| [InlineData(null, 7, null)] | |||
There was a problem hiding this comment.
These data sets were just testing that the LegacyVarTimeZeroScale switch defaults to True, which is now covered above.
| @@ -17,7 +19,7 @@ internal struct PacketHandle | |||
| { | |||
| } | |||
| #endif | |||
| internal partial class TdsParserStateObject | |||
| internal partial class TdsParserStateObject : IDisposable | |||
There was a problem hiding this comment.
This harness adds implementation to TdsParserStateObject, and then the test project explicitly includes some of the MDS source files so they get compiled together. I think we need to revisit this approach.
For now, I have made this implementation disposable and updated the multiplexer tests to take advantage of that.
There was a problem hiding this comment.
Yeah. We really need to revisit this approach because it is a nightmare.
| get | ||
| { | ||
| var switchesType = typeof(SqlCommand).Assembly.GetType("Microsoft.Data.SqlClient.LocalAppContextSwitches"); | ||
| private SwitchesHelper LocalAppContextSwitches = new(); |
There was a problem hiding this comment.
This member needs to match the MDS internal class name that the MDS portion of TdsParserStateObject expects. The real MDS code resolves this as a class type (LocalAppContextSwitches), whereas the test resolves it as a the private field whose type is LocalAppContextSwitchesHelper - yikes!
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3371 +/- ##
==========================================
+ Coverage 64.47% 65.26% +0.79%
==========================================
Files 298 298
Lines 65525 65525
==========================================
+ Hits 42248 42766 +518
+ Misses 23277 22759 -518
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:
|
- Added a test helper to get/set app context switch values. - Updated existing tests to use the helper. - Added missing switches to LocalAppContextSwitches tests.
paulmedynski
force-pushed
the
dev/paul/app-context-switch-test-helper
branch
from
a3b5c48 to
c10ae2a
Compare
| @@ -0,0 +1,355 @@ | |||
| using System; | |||
There was a problem hiding this comment.
I'd prefer to see this be part of a common test project instead of being included manually to both projects.
| // | ||
| public sealed class LocalAppContextSwitchesHelper : IDisposable | ||
| { | ||
| #region Public Types |
There was a problem hiding this comment.
Nit: I usually don't break out the regions until I hit 3 items to add to the region
| { | ||
| #region Public Types | ||
|
|
||
| // This enum is used to represent the state of a switch. |
There was a problem hiding this comment.
Please use xmldocs for block comments like this and methods
| } | ||
|
|
||
| // A local helper to acquire a handle to a property. | ||
| void InitProperty(string name, out PropertyInfo property) |
There was a problem hiding this comment.
I'm not really sure there's a benefit for using a local helper rather than a private instance method. Imho, I try to avoid using local functions unless it provides some perf benefit (eg, when it can be used over a lambda)
| // Acquire a handle to the LocalAppContextSwitches type. | ||
| var assembly = typeof(SqlCommandBuilder).Assembly; | ||
| var switchesType = assembly.GetType( | ||
| "Microsoft.Data.SqlClient.LocalAppContextSwitches"); |
There was a problem hiding this comment.
Nit: What line length limit are you using? I've got mine set at 120, but it's a soft limit (and I try to limit comments to 100). In this case this whole thing could fit on one line, and I'd kinda prefer it did.
| if (failedFields.Count > 0) | ||
| { | ||
| Assert.Fail( | ||
| $"Failed to restore the following fields: " + |
There was a problem hiding this comment.
🤔 You're using an interpolated string here, but not interpolating anything...
This could all fit on one line as an interpolated string :)
|
|
||
| #endregion | ||
|
|
||
| #region Private Members |
There was a problem hiding this comment.
Whoa whoa whoa ... private members should be at the top of the class!
| private readonly PropertyInfo _useCompatProcessSniProperty; | ||
| private readonly PropertyInfo _useCompatAsyncBehaviourProperty; | ||
| #if NETFRAMEWORK | ||
| private readonly PropertyInfo _disableTNIRByDefaultProperty; |
There was a problem hiding this comment.
I'm gonna be that guy and say i don't really care what the property is called in the local app context switches, they were written before we took over, and now that we're taking over, we're gonna name things properly (ie, no capitalized acronyms)!
| [InlineData(6, 6, null)] | ||
| [InlineData(7, 7, null)] | ||
| public void SqlDatetime2Scale_Legacy(int? setScale, byte outputScale, bool? legacyVarTimeZeroScaleSwitchValue) | ||
| public void SqlDatetime2Scale_Legacy(int? setScale, byte outputScale, bool legacyVarTimeZeroScaleSwitchValue) |
There was a problem hiding this comment.
Super nit: I thiink there's precedent for It being DateTime
| @@ -17,7 +19,7 @@ internal struct PacketHandle | |||
| { | |||
| } | |||
| #endif | |||
| internal partial class TdsParserStateObject | |||
| internal partial class TdsParserStateObject : IDisposable | |||
There was a problem hiding this comment.
Yeah. We really need to revisit this approach because it is a nightmare.
Description
This change adds a test helper to get/set app context switch values. The
LocalAppContextSwitchesclass is internal and global, which makes it difficult to manipulate safely in the tests. The helper implements the RAII pattern, capturing the switch values on construction and restoring them on disposal. The helper also provides reflection-based access to the properties and fields ofLocalAppContextSwitchesso tests can manufacture the environment they need.The new
LocalAppContextSwitchesHelperclass is needed by both the Functional and Manual tests, so I put it into a new siblingCommondirectory and reference it directly in the two test projects.All existing tests that touched the app context switches have been updated.
Issues
Fixes #3370 .
Testing
Modified tests were run locally and are passing. CI will run full regression.