SqlClient Manual Test fixes and documentation update #34546

Wraith2 · 2019-01-11T13:36:12Z

Fixes CodeCoverageSqlClient after SyncRoot changes in #34198

Adds the UdtTestDatabase creation script.

Removes Utf8SupportTest and replaced it with a DataTestUtility.IsUTF8Supported function. The test wasn't doing anything useful and makes the console output look strange, it looked like it was the prereq to a bunch of utf8 specific test functionality.

Adds extra conditions to tests affected by https://github.com/dotnet/corefx/issues/33930 so they they will be skipped if managed SNI is being used. The accepted way to do this would be to add [ActiveIssue(33930)] but that would disable the tests for the native implementation and they're still functional there. Instead I chose to add a new condition function in DataTestUtility.IsUsingNativeSNI and use that condition to skip the tests in managed mode. Each location that this condition function is used has a comment containing the text of the active issue attribute that would normally have been applied so that searching will find them when we fix the bug.

Updates the manual tests readme with more information and itemisation.

/cc all the usual suspects, @afsanehr @keeratsingh @saurabh500

remove UTFSupportedTest test

update manual test readme with more detail on running them fix minor code oops

Wraith2 · 2019-01-11T15:37:44Z

Test failure is unrelated and to do with the even log assembly, lets retry anyway.
@dotnet-bot test Windows x86 Release Build please

keeratsingh · 2019-01-28T19:47:00Z

Removes Utf8SupportTest and replaced it with a DataTestUtility.IsUTF8Supported function. The test wasn't doing anything useful and makes the console output look strange, it looked like it was the prereq to a bunch of utf8 specific test functionality.

@Wraith2 This test was added as part of #30917: Support for UTF8 Feature Extension.
Think of this a bare minimum unit test to test if UTF-8 functionality is setup correctly on the client side if the SQL Server supports UTF-8.
Here is how the test case works:

SELECT CONNECTIONPROPERTY('SUPPORT_UTF8') would return NULL if the SQL Server does not have UTF-8 support, in which case the test outputs the not supported message.
SELECT CONNECTIONPROPERTY('SUPPORT_UTF8') is expected to return 1 if both:
- SQL Server has UTF-8 support SQL Server 2019
- Client driver has UTF-8 support

Which means that if SELECT CONNECTIONPROPERTY('SUPPORT_UTF8') returns 0, we know that something is wrong with the UTF-8 functionality on the client side.
If the console output looking strange is your concern I would fix that rather than remove the test altogether.

Wraith2 · 2019-01-28T21:19:48Z

As a standalone test it has no function. I'm testing against 2017 which doesn't have support and it just tells me that UTF8 isn't supported but that is neither a failure or a success it's simply a fact. Why tell me a fact each time I run it? it adds nothing useful and distracts from possible important data.

If UTF8 support were required then using it as a guard condition makes sense and you'll see the test being skipped in the output but at the moment there are no tests which use uft8 facilities and as such it serves no useful purpose.

As you can see I didn't remove the logic I just changed it to be a guard function so when you do add utf8 specific tests it's easily available.

keeratsingh · 2019-01-28T21:58:54Z

As a standalone test it has no function. I'm testing against 2017 which doesn't have support and it just tells me that UTF8 isn't supported but that is neither a failure or a success it's simply a fact. Why tell me a fact each time I run it? it adds nothing useful and distracts from possible important data.

While I agree that it states a fact when SQL Server doesn't support UTF-8, however I beg to differ that it has no function, as explained above it tells you if the UTF-8 support is setup correctly on client-side when the server supports UTF-8.

Why don't we break up the test into a guard function and use that guard function to determine if to skip or run the test, this should alleviate your concerns regarding the console output as well.
Doing so will skip the test with SQL Server versions that don't support UTF-8, which means you will not see a fact in the console, and also the more important part is that it will test the client with SQL Server versions that support UTF-8. While the code looks awfully similar the important part is Assert.Equal(1, reader.GetInt32(0)); which checks that UTF-8 is supported correctly on the client side.

Guard Function:

 public static bool IsUTF8Supported()
{
    bool retval = false;
    if (AreConnStringsSetup())
    {
        using (SqlConnection connection = new SqlConnection(DataTestUtility.TcpConnStr))
        using (SqlCommand command = new SqlCommand())
        {
            command.Connection = connection;
            command.CommandText = "SELECT CONNECTIONPROPERTY('SUPPORT_UTF8')";
            connection.Open();

            using (SqlDataReader reader = command.ExecuteReader())
            {
                while (reader.Read())
                {
                    // CONNECTIONPROPERTY('SUPPORT_UTF8') returns NULL in SQLServer versions that don't support UTF-8.
                    retval = !reader.IsDBNull(0);
                }
            }
        }
    }
    return retval;
}

Unit Test (Uses the Guard Function):

public static class Utf8SupportTest
{
    [ConditionalFact(typeof(DataTestUtility),nameof(DataTestUtility.AreConnStringsSetup),nameof(DataTestUtility.IsUTF8Supported))]
    public static void CheckSupportUtf8ConnectionProperty()
    {
        using (SqlConnection connection = new SqlConnection(DataTestUtility.TcpConnStr))
        using (SqlCommand command = new SqlCommand())
        {
            command.Connection = connection;
            command.CommandText = "SELECT CONNECTIONPROPERTY('SUPPORT_UTF8')";
            connection.Open();

            using (SqlDataReader reader = command.ExecuteReader())
            {
                while (reader.Read())
                {
                    Assert.Equal(1, reader.GetInt32(0));
                }
            }
        }
    }

src/System.Data.SqlClient/tests/ManualTests/README.md

keeratsingh · 2019-01-28T22:25:40Z

src/System.Data.SqlClient/tests/ManualTests/README.md

+## Running All Tests
+
+1. run `build src\System.Data.SqlClient -allconfigurations` and make sure the builds all work


1 & 4 can be combined into a single statement
dotnet msbuild .\src\System.Data.SqlClient\tests\ManualTests\System.Data.SqlClient.ManualTesting.Tests.csproj /t:RebuildAndTest
This would build and run only the Manual Tests as compared to running all tests with command build src\System.Data.SqlClient -debug /p:ForceRunTests=true

If you combine then you get test infrastructure failures:

Executing in E:\Programming\csharp7\corefx\artifacts\bin\tests\System.Data.SqlClient.Tests\netcoreapp-Windows_NT-Debug-x64\ ----- start 22:39:13.28 =============== To repro directly: ===================================================== pushd E:\Programming\csharp7\corefx\artifacts\bin\tests\System.Data.SqlClient.Tests\netcoreapp-Windows_NT-Debug-x64\ popd =========================================================================================================== A JSON parsing exception occurred in [E:\Programming\csharp7\corefx\artifacts\bin\tests\System.Data.SqlClient.Tests\netcoreapp-Windows_NT-Debug-x64\xunit.console.runtimeconfig.json]: * Line 1, Column 2 Syntax error: Malformed token Invalid runtimeconfig.json [E:\Programming\csharp7\corefx\artifacts\bin\tests\System.Data.SqlClient.Tests\netcoreapp-Windows_NT-Debug-x64\xunit.console.runtimeconfig.json] [E:\Programming\csharp7\corefx\artifacts\bin\tests\System.Data.SqlClient.Tests\netcoreapp-Windows_NT-Debug-x64\xunit.console.runtimeconfig.dev.json] ----- end 22:39:13.31 ----- exit code -2147450733 ----------------------------------------------------------

Running the specific build you want to test one at a time works.

Here is my console output for the command dotnet msbuild .\src\System.Data.SqlClient\tests\ManualTests\System.Data.SqlClient.ManualTesting.Tests.csproj /t:RebuildAndTest

PS D:\dotNetCore\corefx> dotnet msbuild .\src\System.Data.SqlClient\tests\ManualTests\System.Data.SqlClient.ManualTesting.Tests.csproj /t:RebuildAndTest Microsoft (R) Build Engine version 15.9.20+g88f5fadfbe for .NET Core Copyright (C) Microsoft Corporation. All rights reserved. Address -> D:\dotNetCore\corefx\artifacts\bin\Address\netstandard-AnyOS-Debug\Address.dll Utf8String -> D:\dotNetCore\corefx\artifacts\bin\Utf8String\netstandard-AnyOS-Debug\Utf8String.dll Shapes -> D:\dotNetCore\corefx\artifacts\bin\Shapes\netstandard-AnyOS-Debug\Shapes.dll Circle -> D:\dotNetCore\corefx\artifacts\bin\Circle\netstandard-AnyOS-Debug\Circle.dll System.Data.SqlClient.ManualTesting.Tests -> D:\dotNetCore\corefx\artifacts\bin\System.Data.SqlClient.ManualTesting.Tests\netcoreapp-AnyOS-Debug\System.Data.SqlClient.ManualTesting.Tests.dll Using D:\dotNetCore\corefx\artifacts\bin\testhost\netcoreapp-Windows_NT-Debug-x64 as the test runtime folder. Executing in D:\dotNetCore\corefx\artifacts\bin\tests\System.Data.SqlClient.ManualTesting.Tests\netcoreapp-Windows_NT-Debug-x64\ ----- start 15:13:58.80 =============== To repro directly: ===================================================== pushd D:\dotNetCore\corefx\artifacts\bin\tests\System.Data.SqlClient.ManualTesting.Tests\netcoreapp-Windows_NT-Debug-x64\ D:\dotNetCore\corefx\artifacts\bin\testhost\netcoreapp-Windows_NT-Debug-x64\dotnet.exe xunit.console.dll System.Data.SqlClient.ManualTesting.Tests.dll -xml testResults.xml -nologo -notrait category=nonnetcoreapptests -notrait category=nonwindowstests -notrait category=failing -notrait category=Outerloop popd =========================================================================================================== Discovering: System.Data.SqlClient.ManualTesting.Tests (method display = ClassAndMethod, method display options = None) Discovered: System.Data.SqlClient.ManualTesting.Tests (found 206 test cases) Starting: System.Data.SqlClient.ManualTesting.Tests (parallel test collections = on, max threads = 8)

I will get the same thing if I use msbuild in the directory or build with either release or debug. If I run an allconfigurations build with force run tests 4 of the error I showed above occur as it tries to run the tests for configurations that don't work.

So allconfigurations with tests doesn't work, a single config does. Since it's just supposed to get people started I wanted to break the build and test steps out individually.

So running the above command will only build and test on platform based on the RID picked up automatically, which is netcoreapp-Windows_NT-Debug-x64 in the case above. allconfigurations will not work, since this is not building all configurations. The thing is that build src\System.Data.SqlClient chooses the csproj in the folder src\System.Data.SqlClient which means it is running all the other tests in SqlClient along with the manual tests.

I've tweaked it a bit. See if that's better. I also recently found you don't need to use msbuild to run a specific test, you can do it straight from build.

I see two potential issues:

Using build src\System.Data.SqlClient -allconfigurations builds all the tests and SqlClient and not just manual tests.

Kindly confirm this, as per my understanding build might not on unix, as on windows, the build command picks up build.cmd implicitly, whereas on unix it will not pickup build.sh implicitly.
The suggested command dotnet msbuild will work across all platforms and will build only manual tests as per the csproj path specified.

You're right, it does. I was also crediting linux users with enough intelligence to realise that the shell script needed to be executable if they're trying to do development work. I've changed it anyway.

Thank you, changes look better, you can get rid of the Step 1, since Step 4 is taking care of building the Manual Tests.

Wraith2 · 2019-01-28T22:46:12Z

Why don't we break up the test into a guard function and use that guard function to determine if to skip or run the test

Agreed, and done.

keeratsingh · 2019-02-01T05:30:13Z

@dotnet-bot
test Linux x64 Release
test OSX x64 Debug Build

keeratsingh

Code changes look good to me, except the last requested change in the README file.
I'll let @afsanehr @saurabh500 have the final say on the approval.

AfsanehR-zz · 2019-02-01T22:35:16Z

@dotnet-bot test corefx-ci

AfsanehR-zz · 2019-02-07T16:31:46Z

@dotnet-bot test corefx-ci

Wraith2 · 2019-02-07T18:32:30Z

synced and pushed to re-trigger

AfsanehR-zz · 2019-02-12T19:51:03Z

/azp run

azure-pipelines · 2019-02-12T19:51:47Z

Azure Pipelines successfully started running 4 pipeline(s).

Wraith2 · 2019-02-13T01:03:24Z

The failures are all sending to helix, which wouldn't run the manual tests that were changed or care about the readme.

AfsanehR-zz · 2019-02-13T19:25:14Z

Merging this PR as the CI failures are not related to the changes made with this PR. Thanks @Wraith2

SqlClient Manual Test fixes and documentation update Commit migrated from dotnet/corefx@f749fca

Wraith2 added 4 commits January 11, 2019 10:17

add IsUsingNativeSNI, IsUFT8Supported DataUtility methods for conditions

349ac63

remove UTFSupportedTest test

fixup syncroot check after changes in PR 34198

39e33ae

disable tests in managed mode affected by 33930

0705eba

add UdtTestDatabase creatiuon script

640e3ed

update manual test readme with more detail on running them fix minor code oops

Wraith2 mentioned this pull request Jan 26, 2019

SqlClient enhancement enable GetFieldValue<XmlReader> #34880

Merged

keeratsingh reviewed Jan 28, 2019

View reviewed changes

src/System.Data.SqlClient/tests/ManualTests/README.md Outdated Show resolved Hide resolved

keeratsingh reviewed Jan 28, 2019

View reviewed changes

re-add UTF8 test using guard condition

652f5c1

Wraith2 added 3 commits January 28, 2019 23:24

addressed feedback

e4ced62

address feedback

ec2c252

address feedback

7f396d1

keeratsingh reviewed Feb 1, 2019

View reviewed changes

keeratsingh added the area-System.Data.SqlClient label Feb 1, 2019

keeratsingh requested review from saurabh500 and AfsanehR-zz February 1, 2019 05:32

address feedback

5456a4c

AfsanehR-zz approved these changes Feb 1, 2019

View reviewed changes

AfsanehR-zz mentioned this pull request Feb 7, 2019

SqlClient reduce metadata size #34393

Merged

Merge remote-tracking branch 'dotnet/master' into sqltests-updates

fe85a32

AfsanehR-zz merged commit f749fca into dotnet:master Feb 13, 2019

karelz added this to the 3.0 milestone Mar 18, 2019

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/corefx#34546 from Wraith2/sqltests-updates

360f43f

SqlClient Manual Test fixes and documentation update Commit migrated from dotnet/corefx@f749fca

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SqlClient Manual Test fixes and documentation update #34546

SqlClient Manual Test fixes and documentation update #34546

Wraith2 commented Jan 11, 2019

Wraith2 commented Jan 11, 2019

keeratsingh commented Jan 28, 2019

Wraith2 commented Jan 28, 2019

keeratsingh commented Jan 28, 2019

keeratsingh Jan 28, 2019

Wraith2 Jan 28, 2019

keeratsingh Jan 28, 2019

Wraith2 Jan 28, 2019

keeratsingh Jan 29, 2019

Wraith2 Jan 29, 2019

keeratsingh Jan 31, 2019

Wraith2 Jan 31, 2019

keeratsingh Feb 1, 2019

Wraith2 Feb 1, 2019

Wraith2 commented Jan 28, 2019

keeratsingh commented Feb 1, 2019

keeratsingh left a comment

AfsanehR-zz commented Feb 1, 2019

AfsanehR-zz commented Feb 7, 2019

Wraith2 commented Feb 7, 2019

AfsanehR-zz commented Feb 12, 2019

azure-pipelines bot commented Feb 12, 2019

Wraith2 commented Feb 13, 2019

AfsanehR-zz commented Feb 13, 2019

		## Running All Tests

		1. run `build src\System.Data.SqlClient -allconfigurations` and make sure the builds all work

SqlClient Manual Test fixes and documentation update #34546

SqlClient Manual Test fixes and documentation update #34546

Conversation

Wraith2 commented Jan 11, 2019

Wraith2 commented Jan 11, 2019

keeratsingh commented Jan 28, 2019

Wraith2 commented Jan 28, 2019

keeratsingh commented Jan 28, 2019

Guard Function:

Unit Test (Uses the Guard Function):

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wraith2 commented Jan 28, 2019

keeratsingh commented Feb 1, 2019

keeratsingh left a comment

Choose a reason for hiding this comment

AfsanehR-zz commented Feb 1, 2019

AfsanehR-zz commented Feb 7, 2019

Wraith2 commented Feb 7, 2019

AfsanehR-zz commented Feb 12, 2019

azure-pipelines bot commented Feb 12, 2019

Wraith2 commented Feb 13, 2019

AfsanehR-zz commented Feb 13, 2019