-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Enable BeginExecuteReader methods #33660
Changes from all commits
595a1f6
6064218
4820e5f
31a4944
d3440c0
860ef7c
6d286d3
e64f019
d6a4710
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,83 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
// See the LICENSE file in the project root for more information. | ||
|
||
using Xunit; | ||
|
||
namespace System.Data.SqlClient.ManualTesting.Tests | ||
{ | ||
public static class BeginExecReaderAsyncTest | ||
{ | ||
private static string GenerateCommandText() | ||
{ | ||
int suffix = (new Random()).Next(5000); | ||
string companyName = "M Inc."; | ||
string phone = "777-1111"; | ||
|
||
string commandText = | ||
$"CREATE TABLE #Shippers{suffix}(" + | ||
$"[ShipperID][int] NULL," + | ||
$"[CompanyName] [nvarchar] (40) NOT NULL," + | ||
$"[Phone] [nvarchar] (24) NULL )" + | ||
$"INSERT INTO #Shippers{suffix}" + | ||
$"([CompanyName] " + | ||
$",[Phone])" + | ||
$"VALUES " + | ||
$"('{companyName}' " + | ||
$",'{phone}'); " + | ||
$"WAITFOR DELAY '0:0:3'; " + | ||
$"select s.ShipperID, s.CompanyName, s.Phone " + | ||
$"from #Shippers{suffix} s; "; | ||
|
||
return commandText; | ||
} | ||
|
||
[ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))] | ||
public static void ExecuteTest() | ||
{ | ||
using (SqlConnection connection = new SqlConnection(DataTestUtility.TcpConnStr)) | ||
{ | ||
SqlCommand command = new SqlCommand(GenerateCommandText(), connection); | ||
connection.Open(); | ||
|
||
IAsyncResult result = command.BeginExecuteReader(); | ||
while (!result.IsCompleted) | ||
{ | ||
System.Threading.Thread.Sleep(100); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Instead of testing BeginExecuteReader with a Sleep statement, could you test an overload which takes in a callback, which is called when the execution finishes? This way you can test EndExecuteReader in the same function and it could provide more branch coverage. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was confused with "Asynchronous Processing" too. I have added test with callback. |
||
} | ||
SqlDataReader reader = command.EndExecuteReader(result); | ||
Assert.True(reader.HasRows, $"FAILED: Reader has no rows"); | ||
} | ||
} | ||
|
||
[ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))] | ||
public static void BeginExecuteReaderWithCallback() | ||
{ | ||
object state = new object(); | ||
bool callbackExecutedFlag = false; | ||
|
||
using (SqlConnection connection = new SqlConnection(DataTestUtility.TcpConnStr)) | ||
using (SqlCommand command = new SqlCommand(GenerateCommandText(), connection)) | ||
{ | ||
connection.Open(); | ||
|
||
Tuple<object, SqlCommand> stateAndCommand = new Tuple<object, SqlCommand>(state, command); | ||
|
||
IAsyncResult result = command.BeginExecuteReader(ar => | ||
{ | ||
Tuple<object, SqlCommand> asyncArgs = ar.AsyncState as Tuple<object, SqlCommand>; | ||
Assert.NotNull(asyncArgs); | ||
|
||
SqlDataReader reader = asyncArgs.Item2.EndExecuteReader(ar); | ||
callbackExecutedFlag = true; | ||
Assert.True(reader.HasRows, $"FAILED: Reader has no rows"); | ||
Assert.Equal(state, asyncArgs.Item1); | ||
}, stateAndCommand); | ||
|
||
System.Threading.Thread.Sleep(TimeSpan.FromSeconds(10)); | ||
|
||
Assert.True(callbackExecutedFlag, $"FAILED: Callback did not executed"); | ||
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@galakt These edits should be added to System.Data.SqlClient.NetCoreApp.cs and not the System.Data.SqlClient.cs file.
These new APIs should be exposed in netcoreapp specific build. Netstandard cannot be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really confused by this:
Btw, i have moved ref changes to the NetCoreApp.cs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @galakt
@danmosemsft My understanding is that any new APIs added even to existing types, should be moved to netcoreapp specific builds based on the conversations we had on this thread. #32245 (review)
Is this assumption correct? I guess we will need to update the baseline files to represent this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@danmosemsft Can we finally resolve question about place for my Refs please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we have been consistent - @ericstj?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@karelz Sorry, can you push this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@saurabh500 @ericstj @danmosemsft Any updates on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SqlClient is not part of netstandard, so its APIs are not pinned by being in netstandard. You do however, need to consider support. If you add API to the reference assembly you should be sure it is supported everywhere that API can be used. So if you're exposing it in a netstandard2.0 reference assembly this means it needs to be supported in .NET 4.6.1 and later, .NETCoreApp 2.0 or later, UAP, etc (see https://docs.microsoft.com/en-us/dotnet/standard/net-standard#net-implementation-support). For UAP and NETCoreApp you can simply update the implementation assembly that is in the package, desktop is the limiting concern here since you cannot update that (and perhaps mono/Xamarin).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@saurabh500 If i understood correct, i should revert "move to NetCoreApp.cs" commit. Am i right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@galakt Yes that is right.
@ericstj Thanks for the clarification.