Update SqlClient manual test feature detection #34040

Wraith2 · 2018-12-12T22:07:13Z

This changes to the ManualTests to add conditional facts on Udt and ServiceBroker reliant test cases so they won't run if the resources required aren't present or enabled. Without these changes local test runs have many errors and it can be difficult to see real problems for the noise.

Can a script or bak file for the UdtTestDb database be provided by someone who has it? It isn't possible to run a full set of manual tests without it.

This PR was split from #33155 and into multiple commits for easier review.

/cc @keeratsingh @afsanehr @saurabh500 @David-Engel

…ases by name and attributes to use them

keeratsingh · 2018-12-13T21:36:45Z

Can a script or bak file for the UdtTestDb database be provided by someone who has it? It isn't possible to run a full set of manual tests without it.

@Wraith2 This should help you setup the UdtTestDb, let me know if you encounter any issues or have questions.
https://gist.github.com/keeratsingh/d2d222340041569aedb5b892d5e15aad

saurabh500

Comments are around using ConditionalFact instead of creating new attributes.
E.g.

corefx/src/System.Diagnostics.PerformanceCounter/tests/PerformanceCounterTests.cs

Line 15 in a10890f

    
           [ConditionalFact(typeof(Helpers), nameof(Helpers.IsElevatedAndCanWriteToPerfCounters))]

saurabh500 · 2018-12-13T21:33:25Z

src/System.Data.SqlClient/tests/ManualTests/DataCommon/CheckDatabaseIsPresentFactAttribute.cs

+
+namespace System.Data.SqlClient.ManualTesting.Tests
+{
+    public class CheckDatabaseIsPresentFactAttribute : FactAttribute


The general recommendation in CoreFx is to use ConditionalFact
CheckConnStrSetupFact was setup before ConditionalFact was introduced.

This particular check needs a parameter. You could add a static in the test files which need to check for the IsDatabasePresent(string) , and pass in the Database name in the test specific static method.

saurabh500 · 2018-12-13T21:36:03Z

...System.Data.SqlClient/tests/ManualTests/DataCommon/CheckServiceBrokerEnabledFactAttribute.cs

+
+namespace System.Data.SqlClient.ManualTesting.Tests
+{
+    public class CheckServiceBrokerEnabledFactAttribute : FactAttribute


Use ConditionalFact over new FactAttributes

saurabh500 · 2018-12-13T21:37:17Z

src/System.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs

+
+        public static bool IsDatabasePresent(string name)
+        {
+            if (databasesAvailable == null)


Nit: Could be databasesAvailable = databasesAvailable ?? new Dictionary<string, bool>();

Wraith2 · 2018-12-13T23:58:02Z

All feedback addressed, new attributes are gone replaced with ConditionalFacts
Would it be ok to add the udt database script to the project next to the readme so others can get to it easily? I'm intending to update the readme with the information I've gathered in another PR later.

saurabh500 · 2018-12-13T23:59:38Z

That's a good idea

src/System.Data.SqlClient/tests/ManualTests/SQL/SqlNotificationTest/SqlNotificationTest.cs

src/System.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs

saurabh500 · 2018-12-14T01:21:31Z

src/System.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs

+                        string database = builder.InitialCatalog;
+                        builder.ConnectTimeout = 2;
+                        using (var connection = new SqlConnection(builder.ToString()))
+                        using (var command = new SqlCommand("SELECT is_service_broker_enabled FROM sys.sys.databases WHERE name=@name", connection))


Here's a bug. The name of the DB is hardcoded to @name. When I try to run the tests on my server where SB is enabled, the tests are skipped. Please fix this to pass in the correct DB name

saurabh500

Please fix the check for the Service Broker to pass in the DB name and to surface the exception if the Service Broker query fails.

Wraith2 · 2018-12-14T08:58:24Z

Ok, but how? the sql dependency test don't use a specific database which is why the current code tried to pull the database name out of the connection string, it failed due to other bugs but i'll fix those as suggested.

saurabh500 · 2018-12-14T09:21:07Z

I looked at the notifications tests, the constructor is supposed to setup the required infra for query notification
https://github.com/dotnet/corefx/blob/master/src/System.Data.SqlClient/tests/ManualTests/SQL/SqlNotificationTest/SqlNotificationTest.cs#L312
Is that not working for you? The check for query notification is not needed.

saurabh500 · 2018-12-14T09:33:49Z

The more I think about this change, I feel you should remove public static bool IsServiceBrokerEnabled from the change along with its usage and update the PR. The rest of it looks good.
I would like to know why Sql Notification tests are failing for you. Setup issue/server configuration etc. Perhaps it warrants an issue where this could be discussed further?

Wraith2 · 2018-12-14T09:52:09Z

That should fix it. The connection string has northwind in it so anything not using northwind will have to specify the database for itself, on that basis it'll either work or throw as you suggested indicating the setup is incorrect.

saurabh500 · 2018-12-14T16:16:38Z

If the setup of the notification test changes to create a new database and tables for the Notification tests, then they will continue to be skipped because the Northwind database is being checked for Service Broker enablement.
This doesn't look right.
I still think that you should figure out where the setup for the notification tests is failing. The setup doesn't need any external scripts. Its embedded in the test itself.

Right now this is what is happening.

The Service Broker test is doing some the setup.

tring.Format("CREATE TABLE {0}(a INT NOT NULL, b NVARCHAR(10), c INT NOT NULL)", tableName),
                string.Format("INSERT INTO {0} (a, b, c) VALUES (1, 'foo', 0)", tableName),
                string.Format("CREATE QUEUE {0}", queueName),
                string.Format("CREATE SERVICE [{0}] ON QUEUE {1} ([http://schemas.microsoft.com/SQL/Notifications/PostQueryNotification])", serviceName, queueName)

The setup is failing for you
The ConditionalFact returns false for Northwind database's broker enablement
The test is skipped.

The problem is that we are hiding the shortcomings of step 2. I would much rather fix that, than skipping the tests because the setup failed.

What errors do you get when you try to run the above SQL Statements in SSMS or any other tool that can work with SQL Server??

Wraith2 · 2018-12-14T18:01:26Z

The tests run and pass as of the fix I pushed this morning. I just tried those statements, they work as expected. If the test changes to use a specific database then it would need to pass the constr being used as a parameter, which I can do with a local static function similar to the ones I removed yesterday.

saurabh500 · 2018-12-14T18:30:54Z

The tests run and pass as of the fix I pushed this morning.

This is good to know. I don't understand what part of the fix, alleviated the issue for you? The way I see it, is that the test is self contained. We shouldn't need this check. If the setup in the test fails, then that's something worth look into.

Can we get rid of this check for IsServiceBrokerEnabled now?

Wraith2 · 2018-12-14T19:06:05Z

Ok, shall I leave the utility function in DataTestUtility in case someone else needs it or get rid of everything?

saurabh500 · 2018-12-14T19:07:50Z

You should get rid of everything related to IsServiceBrokerEnabled including the implementation. The lesser the code, the better.

Wraith2 · 2018-12-14T19:11:32Z

Done.

src/System.Data.SqlClient/tests/ManualTests/SQL/SqlNotificationTest/SqlNotificationTest.cs

saurabh500 · 2018-12-14T19:18:52Z

src/System.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs

@@ -21,6 +25,10 @@ public static class DataTestUtility
                                                                     ".database.cloudapi.de",
                                                                     ".database.usgovcloudapi.net",
                                                                     ".database.chinacloudapi.cn"};
+
+        private static bool? serviceBrokerEnabled;


Not needed anymore.

saurabh500 · 2018-12-14T19:58:40Z

@Wraith2 The CI is failing on a warning being treated as error

D:\j\workspace\windows-TGrou---f8ac6754\src\System.Data.SqlClient\tests\ManualTests\DataCommon\DataTestUtility.cs(29,30): error CS0169: The field 'DataTestUtility.serviceBrokerEnabled' is never used [D:\j\workspace\windows-TGrou---f8ac6754\src\System.Data.SqlClient\tests\FunctionalTests\System.Data.SqlClient.Tests.csproj]

Wraith2 · 2018-12-14T20:43:54Z

Fixed, I had to wait for tests to finish on another branch before I could switch and fix.

saurabh500 · 2018-12-14T21:37:19Z

Can be merged once the CI is green

* add DataUtility functions to detect service broker and specific databases by name and attributes to use them * change UDT tests to use database detection fact for UDTTestDatabase * change service broker tests to use conditional fact * add the new attribute files to the project * address feedback * remember to open connection before running query * address feedback * fixup IsServiceBrokerEnabled * remove IsServiceBrokerEnabled * re-enabled tests * remove unused variable

* add DataUtility functions to detect service broker and specific databases by name and attributes to use them * change UDT tests to use database detection fact for UDTTestDatabase * change service broker tests to use conditional fact * add the new attribute files to the project * address feedback * remember to open connection before running query * address feedback * fixup IsServiceBrokerEnabled * remove IsServiceBrokerEnabled * re-enabled tests * remove unused variable Commit migrated from dotnet/corefx@202c98a

Wraith2 added 4 commits December 12, 2018 19:34

add DataUtility functions to detect service broker and specific datab…

86c0018

…ases by name and attributes to use them

change UDT tests to use database detection fact for UDTTestDatabase

3cfa7f2

change service broker tests to use conditional fact

7e86ed5

add the new attribute files to the project

9507ae0

Wraith2 mentioned this pull request Dec 12, 2018

Change SqlClient to use strongly typed packet and session handles #33155

Closed

saurabh500 suggested changes Dec 13, 2018

View reviewed changes

saurabh500 added the area-System.Data.SqlClient label Dec 13, 2018

address feedback

e739e68