Skip to content

Create app context switch test helper #3371

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jun 5, 2025
Merged

Create app context switch test helper #3371

merged 3 commits into from
Jun 5, 2025

Conversation

paulmedynski
Copy link
Contributor

Description

This change adds a test helper to get/set app context switch values. The LocalAppContextSwitches class 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 of LocalAppContextSwitches so tests can manufacture the environment they need.

The new LocalAppContextSwitchesHelper class is needed by both the Functional and Manual tests, so I put it into a new sibling Common directory 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.

@Copilot Copilot AI review requested due to automatic review settings May 21, 2025 20:02
@paulmedynski paulmedynski requested a review from a team as a code owner May 21, 2025 20:02
Copilot
Copilot AI reviewed May 21, 2025
View reviewed changes
Copy link
Contributor

@Copilot Copilot AI left a 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 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 paulmedynski added this to the 6.1-preview2 milestone May 21, 2025
@paulmedynski paulmedynski added the Area\Tests Issues that are targeted to tests or test projects label May 21, 2025
@codecov Codecov
Copy link

codecov bot commented May 22, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 71.42857% with 6 lines in your changes missing coverage. Please review.

Project coverage is 67.67%. Comparing base (1eabf80) to head (db9609e).
Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
...icrosoft/Data/SqlClient/LocalAppContextSwitches.cs 77.77% 4 Missing ⚠️
.../netcore/src/Microsoft/Data/SqlClient/TdsParser.cs 0.00% 1 Missing ⚠️
...nt/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3371      +/-   ##
==========================================
+ Coverage   64.47%   67.67%   +3.19%     
==========================================
  Files         298      299       +1     
  Lines       65525    65640     +115     
==========================================
+ Hits        42248    44419    +2171     
+ Misses      23277    21221    -2056
Flag Coverage Δ
addons 92.58% <ø> (ø)
netcore 72.97% <71.42%> (+4.47%) ⬆️
netfx 65.43% <75.00%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@paulmedynski
User Story 37545: Create app context switch test helper
c10ae2a 
- 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 paulmedynski force-pushed the dev/paul/app-context-switch-test-helper branch from a3b5c48 to c10ae2a Compare May 28, 2025 09:37
mdaigle
mdaigle previously approved these changes May 29, 2025
View reviewed changes
benrr101
benrr101 requested changes May 29, 2025
View reviewed changes
Copy link
Contributor

@benrr101 benrr101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, good implementation, just a bunch of style stuff I want to make sure we get right.

@paulmedynski paulmedynski mentioned this pull request May 30, 2025
Open
@paulmedynski
User Story 37545: Create app context switch test helper
51aa108 
- Addressed review comments.
- Added UseConnectionPoolV2 switch to the Helper.
- Commented out some Debug.Asserts() that are failing.
@paulmedynski
User Story 37545: Create app context switch test helper
db9609e 
- Fixed typo in LocalAppContextSwitchesTests.
benrr101
benrr101 approved these changes May 30, 2025
View reviewed changes
Copy link
Contributor

@benrr101 benrr101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better, thank you for addressing the feedback 👍

@paulmedynski paulmedynski merged commit 154be88 into main Jun 5, 2025
251 checks passed
@paulmedynski paulmedynski deleted the dev/paul/app-context-switch-test-helper branch June 5, 2025 09:35
@edwardneal edwardneal mentioned this pull request Jun 7, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benrr101 benrr101 benrr101 approved these changes

@mdaigle mdaigle mdaigle approved these changes

@Wraith2 Wraith2 Wraith2 left review comments

@edwardneal edwardneal edwardneal left review comments

Copilot code review Copilot Copilot left review comments

Assignees
No one assigned
Labels
Area\Tests Issues that are targeted to tests or test projects
Projects
None yet
Milestone
6.1-preview2
Development

Successfully merging this pull request may close these issues.

Create app context switch test helper
5 participants
@paulmedynski @benrr101 @mdaigle @Wraith2 @edwardneal