Tests | Widen SqlVector test criteria, roundtrip additional values in tests #3794
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
| /// 'vector' data type. If a connection cannot be established or an error occurs during the query, the method | ||
| /// returns <see langword="false"/>.</remarks> | ||
| /// <returns><see langword="true"/> if the 'vector' data type is supported; otherwise, <see langword="false"/>.</returns> | ||
| public static bool IsSupportedSqlVector() |
There was a problem hiding this comment.
Can you convert this into a property, and only query database on its initialization?
There was a problem hiding this comment.
Thanks. I've done this (along with IsSupportedDataClassification as you mentioned in the other comment, although I've changed this method's approach slightly to use OBJECT_ID rather than to wait for an error.)
I've also cleaned up the test conditions slightly, since both properties will return false if the TCP connection string is unspecified.
| /// Checks if object SYS.SENSITIVITY_CLASSIFICATIONS exists in SQL Server | ||
| /// </summary> | ||
| /// <returns>True, if target SQL Server supports Data Classification</returns> | ||
| public static bool IsSupportedDataClassification() |
There was a problem hiding this comment.
I would also change this to a property and reduce server calls.
These are IsDataClassificationSupported and IsVectorSupported. In the process, removed the attempted caching for IsTDS8Supported. This caching didn't work properly, and never should have - the ctor for CertificateTest changed the certificate for the SQL Server instance, so the cached data could have been out of date.
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
paulmedynski
left a comment
paulmedynski
left a comment
There was a problem hiding this comment.
Can we avoid querying the server more than once per DataTestUtility property that you have touched?
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs
Show resolved
Hide resolved
| } | ||
| public static bool IsTDS8Supported => | ||
| IsTCPConnStringSetup() && | ||
| GetSQLServerStatusOnTDS8(TCPConnectionString); |
There was a problem hiding this comment.
This is still querying the server every time this property is accessed. Can you query the server once and store the result for all future access?
This would be a great use of the new field keyword if we were already using the .NET 10 SDK:
https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/field
There was a problem hiding this comment.
I thought about doing that, but the behaviour wouldn't be correct unfortunately. This is only used in the CertificateTest class; the constructor for this class runs the PowerShell script GenerateSelfSignedCertificate.ps1.
That PowerShell script changes whether TDS8 is supported; it generates a new certificate with the right ACL for the SQL Server service to use it, forces Windows to trust it, then configures SQL Server to use it. The value of the check would vary depending upon whether anything else has read the property before that caching occurred.
There was a problem hiding this comment.
I'm not sure I understand. The TCPConnString is constant for all tests in the test run, so the server is supports TDS8 for all tests in the run, or it doesn't. I'm not sure how that affects anything in the ps1 script - I don't see it performing any checks for TDS8.
There was a problem hiding this comment.
TCPConnString is constant, but the tests in CertificateTest only run when that connection string points to localhost. The script is run before the tests, and it dynamically reconfigures the local SQL Server instance to meet all of the prerequisites for TDS8 - if we make IsTDS8Supported cache its result and a future test runs prior to a test from CertificateTest, we could return a stale value.
That can't happen right now (CertificateTest is the only test which uses this condition) but it'd cause a problem if we use it from any other test.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/VectorTest/NativeVectorFloat32Tests.cs
Show resolved
Hide resolved
4 participants
Description
This PR does two things:
SqlVectortests to be run against a database server which supports thevectortype (not just Azure SQL instances.)float.MinValue, a value which can't be represented cleanly as a float, and negative zero) to demonstrate that they roundtrip, as per my comments on the original PR.Issues
Fixes #3789. Builds on my comments on the original PR #3433.
Testing
SqlVectortests continue to pass against a local SQL 2025 instance and a SQL Azure instance.