Skip to content

Test | Convert DateTimeVariantTests to xunit#4196

Open
mdaigle wants to merge 15 commits intomainfrom
dev/mdaigle/datetime-xunit
Open

Test | Convert DateTimeVariantTests to xunit#4196
mdaigle wants to merge 15 commits intomainfrom
dev/mdaigle/datetime-xunit

Conversation

@mdaigle
Copy link
Copy Markdown
Contributor

@mdaigle mdaigle commented Apr 14, 2026

Parameterizes tests using shared test data. Expected exceptions are made explicit in test data. Logging and baseline file comparisons are replaced with assertions.
Each commit can be reviewed individually to see the logical progression of the conversion.

mdaigle added 10 commits April 14, 2026 16:09
Remove the [Collection], [Trait("Category", "flaky")] attributes and
CultureInfo workaround. These were masking underlying issues that will
be addressed by converting to direct xunit assertions.
Convert DateTimeVariantTests from a single monolithic test to xunit
[ConditionalTheory] with [MemberData] providing parameterized test
cases for each date/time type combination. Each test case now calls
SendInfo on its specific (value, typeName, baseTypeName) combination
and compares against its own per-test baseline file.

Remove the TestAllDateTimeWithDataTypeAndVariant dispatch method from
DateTimeVariantTest since each test case now invokes SendInfo directly.
Make SendInfo public and pass the connection string explicitly.
Split test logic from error handling in DateTimeVariantTest.cs. Each
test method now separates the core SQL operations from the try/catch
exception handling. Consolidate error handling patterns across all 16
test variations. Pull some expected errors up into parameterized test
input in DateTimeVariantTests.cs so callers declare which exceptions
are expected. Remove exception handlers that are no longer needed.
Add TestVariations enum to explicitly identify each of the 16 test
methods (TestSimpleParameter_Type, TestSimpleParameter_Variant, TVP
variations, DataReader, BulkCopy, etc.). Associate expected exceptions
with specific test variations in DateTimeVariantTests using dictionaries
passed through MemberData. Register expected-but-uncaught exceptions so
they can be tracked and asserted later.
Consolidate verification into a single method. Simplify date comparison
logic. Remove unnecessary checks, casts, and unused parameters from
the helper methods. Pull expected value mismatches and unexpected value
overrides up into the parameterized test input so each test case
declares its full expected behavior upfront.
Add xunit Assert.Equal/Assert.True for exception validation, value
comparison, type checking, and base type verification. Replace all
console output logging with direct assertions:
- Exception logs → Assert.True with ExceptionChecker delegates
- Value mismatch logs → Assert.Equal with expected value overrides
- Type mismatch logs → Assert.Equal for type comparison
- Base type mismatch logs → Assert.Equal with base type overrides

Remove unused display methods. The baseline .bsl files are now empty
since all verification is done through assertions.
All verification is now done through xunit assertions. The baseline
.bsl files are empty and no longer needed. Remove the 16 per-test
baseline files and the DateTimeVariant glob Content entry from the
csproj.
Remove unused helper methods, description strings, and unnecessary
casts from DateTimeVariantTest.cs. Consolidate exception checker
delegates and value override dictionaries in DateTimeVariantTests.cs.
Split test cases into separate methods and add missing expected values.
Fix type comparison to remove cast exception.
Move all helper methods (xsql, DropStoredProcedure, DropTable, DropType,
GetSqlDbType, RunTest) and the ExceptionChecker delegates from the old
DateTimeVariantTest.cs static class into DateTimeVariantTests.cs. Each
test variation is now a separate [ConditionalTheory] test method directly
in the test class. Remove DateTimeVariantTest.cs and its csproj entry.
Copilot AI review requested due to automatic review settings April 14, 2026 23:34
@github-project-automation github-project-automation Bot moved this to To triage in SqlClient Board Apr 14, 2026
Copy link
Copy Markdown
Contributor

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 refactors the manual DateTime/sql_variant coverage from a baseline-file-driven harness into xUnit ConditionalTheory test cases, and removes the legacy helper/baseline artifacts.

Changes:

  • Replaced the single baseline-comparison test runner with multiple [MemberData]-driven theory tests for different API paths (simple params, TVPs, bulk copy, reader paths).
  • Deleted the legacy DateTimeVariantTest.cs harness and the DateTimeVariant.bsl baseline file.
  • Updated the ManualTests project file to stop compiling/copying the removed files.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File Description
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ParameterTest/DateTimeVariantTests.cs Reworked DateTime variant testing into xUnit theory-based tests with shared runner/helpers.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ParameterTest/DateTimeVariantTest.cs Removed legacy standalone test harness implementation.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ParameterTest/DateTimeVariant.bsl Removed baseline output file previously used for comparison.
src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTests.csproj Removed compile/content entries for the deleted harness and baseline file.

@mdaigle mdaigle changed the title Dev/mdaigle/datetime xunit Test | Convert DateTimeVariantTests to xunit Apr 14, 2026
@mdaigle mdaigle added this to the 7.1.0-preview1 milestone Apr 14, 2026
@mdaigle mdaigle added Hotfix 7.0.2 PRs targeting main whose changes should be backported to release/7.0 for the 7.0.2 release. Hotfix 6.1.6 and removed Hotfix 7.0.2 PRs targeting main whose changes should be backported to release/7.0 for the 7.0.2 release. Hotfix 6.1.6 labels Apr 14, 2026
Date/time formatting in the test helpers (e.g. DateTime.ToString("M/d/yyyy"))
is locale-dependent. On Linux, the default culture may not be en-US,
causing SQL INSERT statements to produce values the server cannot parse.
Wrap RunTest with CultureInfo("en-US") to ensure consistent formatting.
@mdaigle mdaigle marked this pull request as ready for review April 15, 2026 00:24
@mdaigle mdaigle requested a review from a team as a code owner April 15, 2026 00:24
Copilot AI review requested due to automatic review settings April 15, 2026 00:24
Copy link
Copy Markdown
Contributor

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

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

paulmedynski
paulmedynski previously approved these changes Apr 24, 2026
@paulmedynski paulmedynski enabled auto-merge (squash) April 24, 2026 13:50
@paulmedynski paulmedynski disabled auto-merge April 24, 2026 13:50
@benrr101 benrr101 moved this from To triage to In review in SqlClient Board Apr 27, 2026
```

**Rules:**
- Open the connection **before** constructing any database object (the constructor executes DDL immediately)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We don't need to open the connection in advance; the RAII objects take responsibility for opening the connection (if necessary) before creating or dropping the database object. The example pattern below would work just as well:

using SqlConnection conn = new(DataTestUtility.TCPConnectionString);
using Table testTable = new(conn, "MyTable", "(Id INT, Name NVARCHAR(100))");
using StoredProcedure proc = new(conn, "MyProc", $"AS BEGIN SELECT * FROM {testTable.Name} END");

using SqlCommand cmd = conn.CreateCommand();
cmd.CommandText = proc.Name;
cmd.CommandType = CommandType.StoredProcedure;
// ... objects are automatically dropped when the scope ends

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think I'll leave it just to try and keep things explicit. It's a bit harder to track down a connection failure inside a helper method.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 6, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.32%. Comparing base (fdbc56e) to head (9688288).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4196      +/-   ##
==========================================
- Coverage   65.97%   64.32%   -1.66%     
==========================================
  Files         276      270       -6     
  Lines       42994    65800   +22806     
==========================================
+ Hits        28366    42323   +13957     
- Misses      14628    23477    +8849     
Flag Coverage Δ
CI-SqlClient ?
PR-SqlClient-Project 64.32% <ø> (?)

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.

{
using SqlConnection conn = new(connStr);
conn.Open();
using StoredProcedure proc = new(conn, "paramProc1", $"(@param {expectedBaseTypeName}) AS BEGIN SELECT @param, sql_variant_property(@param,'BaseType') AS BaseType END;");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I must have missed the PR that added these RAII objects.

One thing I'd like fixed (in this PR) is to ensure that the Dispose() methods never throw. As-is, a failure in one Dispose() will kill the test and inhibit other RAII objects from attempting their cleanup.

@github-project-automation github-project-automation Bot moved this from In review to In progress in SqlClient Board May 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In progress

Development

Successfully merging this pull request may close these issues.

6 participants