Tests | Refactor NativeVectorFloat32Tests for future Half support - #4347
Conversation
Removed use of var, ensured that all variables are disposed of where trivial
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Refactors the native SqlVector<T> manual test suite to use a generic test-data + shared test-logic base, so adding future element types (e.g., Half) becomes mostly a matter of adding a new *TestData + thin derived test class.
Changes:
- Adds
NativeVectorTestDataBase<TElement>to centralize per-element sample data and parameter-pattern cases. - Adds
NativeVectorTestsBase<TElement, TTestData>to host the shared insertion/read, stored-proc, bulk-copy, and prepare/execute test logic using RAII DB objects. - Refactors the float32 suite into
VectorFloat32TestData+NativeVectorFloat32Tests : NativeVectorTestsBase<...>.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/VectorTest/NativeVectorTestsBase.cs |
New generic base classes for vector manual tests + shared test logic and RAII-managed DB objects. |
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/VectorTest/NativeVectorFloat32Tests.cs |
Refactors float32 tests into new test-data class + thin derived test class using the new base. |
|
/azp run |
|
Azure Pipelines: Successfully started running 3 pipeline(s). |
apoorvdeshmukh
left a comment
There was a problem hiding this comment.
Just one minor suggestion. Looks good to me otherwise.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4347 +/- ##
==========================================
- Coverage 66.50% 63.56% -2.95%
==========================================
Files 285 280 -5
Lines 43311 66783 +23472
==========================================
+ Hits 28806 42450 +13644
- Misses 14505 24333 +9828
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
/azp run |
|
Azure Pipelines: Successfully started running 3 pipeline(s). |
Brings in the completed connection-pool pruning work (Story 2/3/4, #4463), which reworks PoolPruner to be driven by Connection Idle Timeout and only constructs a Pruner when IdleTimeout != 0. The single overlapping file, ChannelDbConnectionPool.cs, auto-merged cleanly: main's constructor pruner block coexists with this branch's ReplaceConnection additions. Also pulls in #4460 (unobserved-exception repro), #4347 (vector test refactor), and #4459 (pool benchmark coverage). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Description
This is somewhat inspired by #4234. We don't yet have support for
SqlVector<Half>and other types, but this should hopefully make the test logic trivial to add.I'd recommend moving commit by commit - the introduction of a base class has resulted in the PR looking much larger than it truly is.
The overall design is now more generic. We have:
NativeVectorTestDataBase<TElement>, which contains test data forSqlVector<TElement>NativeVectorTestsBase<TElement, TTestData>, which contains the test logic and links it to the test dataVectorFloat32TestData: derived from NativeVectorTestDataBase which containsfloat-based sample dataNativeVectorFloat32Tests: the same tests and the same logic as in main, but derived from NativeVectorTestsBaseAdding future support for
Halfshould mean simply introducingVectorFloat16TestDataandNativeVectorFloat16Testsclasses:I've also made a handful of hygiene improvements, but none are particularly controversial:
Assert.Fail, performingAssert.True(!verifyReader.IsDBNull(0), ...), etc.)Issues
In lieu of any more specific issue for Half/float16 support for SqlVector, this contributes to #3444.
Testing
All automated tests continue to pass.