Sqlclient: Improve code coverage for System.Data.SqlClient (18.1%)

[danmosemsft]

Report here

[danmosemsft]

This may help? https://github.com/dotnet/corefx/pull/13665

[FransBouma]

It might help perhaps to port the internal tests for SqlClient to corefx? I assume it has a lot more coverage than the corefx counterpart?

[danmosemsft]

@saurabh500 coverage for SqlClient is still only 22%. Is there any internal test code that might help, that someone could help clean up possibly?

https://ci.dot.net/job/dotnet_corefx/job/master/job/code_coverage_windows/Code_Coverage_Report/

[danmosemsft]

These are the types with the most missing blocks, in descending order

System.Data.SqlClient.TdsParser
Microsoft.SqlServer.Server.ValueUtilsSmi
System.Data.SqlClient.SqlDataReader
System.Data.SqlClient.SqlBulkCopy
System.Data.SqlClient.TdsParserStateObject
System.Data.SqlClient.SqlCommand
System.Data.SqlClient.SqlParameter
Microsoft.SqlServer.Server.SqlMetaData
System.Data.SqlClient.SqlBuffer
System.Net.Security.SafeDeleteContext
System.Data.Common.ADP
System.Data.SqlClient.SqlConnection
System.Data.SqlClient.SqlInternalConnectionTds
System.Data.SqlClient.TdsValueSetter
Microsoft.SqlServer.Server.SqlDataRecord
System.Data.SqlClient.MetaType
Microsoft.SqlServer.Server.MetaDataUtilsSmi
System.Data.SqlClient.SQL
System.Net.NetEventSource
System.Net.SSPIWrapper
Microsoft.SqlServer.Server.SqlRecordBuffer

@saurabh500 the amount of code is substantial so starting with any existing tests would be ideal.

[danmosemsft]

Info on code coverage work:

1) Here's the all up report
https://ci.dot.net/job/dotnet_corefx/job/master/job/code_coverage_windows/Code_Coverage_Report/
Caveat - doesn't show coverage of any types whose implementation is in corelib (eg String, etc). If you want to look at that for now you must gather manually using instructions below.

2) Docs on code coverage work are here
https://github.com/dotnet/corefx/blob/master/Documentation/building/code-coverage.md
and some in
https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/developer-guide.md

3) There's innumerable places to add coverage -- we would love to focus on the most heavily used types and the least covered types to get the most benefit.

[danmosemsft]

@saurabh500 not a high priority, but question above about existing tests for sql client. Is there a body of existing code that we can dump into the repo for folks to begin to clean up and enable as tests? Can you research?

@FransBouma another option is borrowing Mono tests. This is totally legitimate. Any interest?

[saurabh500]

@danmosemsft there are a lot of tests that we have in our internal repo that I can make available. We also have tests in manual tests folder in sqlclient tests. They are not run automatically due to the absence of a SQL server accessible to these tests in the CI. We run those tests while making changes to sqlclient. They depend on a connection string being provided in environment variable.
If mono tests for sqlclient don't need a server to run against or if servers can be made available then lighting up tests and allowing additional coverage is not hard.

[FransBouma]

I think a public testserver would indeed be key to get started. Everyone testing against their own local box isn't reliable, I think.

[danmosemsft]

adding @dhoehna since he asked about contributing in this area. @dhoehna one possible thing you could do is look at what MOno does to test SqlClient, and see whether anything can be reused.

[dhoehna]

@danmosemsft There are a lot of tests in System.Data.SqlClient. Where is the lack of code coverage?

[danmosemsft]

@dhoehna take a look at the all up report linked above https://ci.dot.net/job/dotnet_corefx/job/master/job/code_coverage_windows/Code_Coverage_Report/ search for SqlClient you see there is only 20% coverage.

[dhoehna]

@danmosemsft I found that. SO, all the classes under the SqlClient heading need tests? What is Mono?

[dhoehna]

@danmosemsft ?

[danmosemsft]

@dhoehna Mono is an open source project to port .NET to other platforms (originated long ago). We can reuse its tests if they're valuable. Take a look and see:
https://github.com/mono/mono/tree/master/mcs/class/System.Data/System.Data.SqlClient

[EshG]

@danmosemsft It looks like mono has a tool that generates test databases locally. I'd be glad to try and implement something similar. What do you think?
https://github.com/mono/mono/tree/master/mcs/class/System.Data/Test/DataProviderTests

[danmosemsft]

I don't own this area. @saurabh500 @corivera can you provide guidance here...

[RemiBou]

I want to start working on this, I can start with TdsParser, but I can't find any existing test about it, are they on the repo ? (the coverage report shows 21%)

[saurabh500]

The issue with SqlClient is that it needs a server to test most of the code.
There could be tests for types that don't require the server, and maybe Mono tests can be used here as they already depend on non-internal MS frameworks for testing.
@RemiBou The TdsParser is an internal class and cannot be tested in isolation unless we expose the internals to the test assembly. However this idea was ruled out because we want to be able to run the same tests on all frameworks and the tests shouldn't be internal dependant.

One of the approaches I had started with, was to use a Test TDS server from which I am returning responses to simple queries. Ref https://github.com/dotnet/corefx/blob/master/src/System.Data.SqlClient/tests/FunctionalTests/DiagnosticTest.cs#L703
You could enhance the server to mock responses to more kinds of queries and improve the test execution coverage in the FunctionalTests.

[saurabh500]

The example of some of the queries that can be responded to are at
https://github.com/dotnet/corefx/blob/a27929935d2782cdc3e7be21653e8c4530274c6f/src/System.Data.SqlClient/tests/Tools/TDS/TDS.Servers/QueryEngine.cs#L67

One needs to be careful about crafting the response with the right Tds packet.
If I had to enhance the query engine, I would run a particular test with Sql Server and debug the code to see what response the server is sending and try to code it up in the Query engine.

I think more low hanging fruits are public types like SqlMetaData SqlDataRecord SqlConnectionStringBuilder etc

[RemiBou]

OK Tds is a bit too difficult for a first time, I'll go to SqlDataRecord and see what I can test. Because it's testing after coding, I plan to comment some instruction, create a failing test, uncomment and check all the tests pass, do you think that's the right approach ?

[saurabh500]

@RemiBou

I plan to comment some instruction, create a failing test, uncomment and check all the tests pass, do you think that's the right approach

I dont quite follow. What do you mean by comment some instructions?

[RemiBou]

Comment out some code in SqlDataRecord, write a test that is supposed to validate this part of code, see it fail and then uncomment it and see the test pass (I won't commit the commented code).

[saurabh500]

Got it.

You could look at the test for SqlDataRecord at https://github.com/dotnet/corefx/blob/master/src/System.Data.SqlClient/tests/FunctionalTests/SqlDataRecordTest.cs

It still has low line coverage. 39.3%
Further looking at the coverage report methods like GetGuid(...) GetInt16 etc are not being tested at all.

You could enhance the test above to pass in a SqlMetadata for the Types the Get methods are currently not testing.

After doing this exercise you could move on to the setters.

[saurabh500]

@RemiBou If you are comfortable and want to move forward with SqlDataRecord test, I can open a separate issue and we can discuss further on that issue. This is a catch all issue for SqlClient as such and individual Types discussion can be moved to a separate issue to maintain clarity of discussion.

[RemiBou]

I you prefer we can do that, or we can discuss on each PR, I'll send a small one (a few test) for validating that I am on the good track

[saurabh500]

Here you go https://github.com/dotnet/corefx/issues/27416

[dustyhoppe]

@saurabh500 , @danmosemsft: is there a mechanism for running code coverage + report generator locally? I'm looking into contributing on this issue and hoping such a mechanism exists. Thanks.

[stephentoub]

is there a mechanism for running code coverage + report generator locally?

When you run the tests locally, instead of just doing:

msbuild /t:rebuildandtest

you can do:

msbuild /t:rebuildandtest /p:Coverage

See https://github.com/dotnet/corefx/blob/968cfcf373633a82e1dc1850e9852e0c491112dd/Documentation/building/code-coverage.md for more details.

[dustyhoppe]

@stephentoub thanks. If its fine with you, I'm going to start working this issue. This is my first time contributing and I was wondering if it is acceptable to use the AAA pattern when writing unit tests?

[stephentoub]

That's fine. Though we don't need each "section" named with a comment as some people do, and it's also fine to collapse them (I personally disagree with folks that it's a test smell doing so).

[IcanBENCHurCAT]

Looks like there is still a lot of code coverage to be desired here. I will be digging into Mono tests this week to see if there is anything that can be pulled over.

FYI - this link to the mono tests are here now.

[divega]

As recently announced in the .NET Blog, focus on new SqlClient features an improvements is moving to the new Microsoft.Data.SqlClient package. For this reason, we are moving this issue to the new repo at https://github.com/dotnet/SqlClient. We will still use https://github.com/dotnet/corefx to track issues on other providers like System.Data.Odbc and System.Data.OleDB, and general ADO.NET and .NET data access issues.

[cheenamalhotra]

Hi @danmosemsft

The current code coverage for Microsoft.Data.SqlClient is currently ~55% (Internal pipeline report).

If you agree, we'd like to close the issue and keep working on code coverage improvement as we move forward.

Let us know what you think!

[danmosemsft]

That's a big improvement, I think it can be closed now 😸