Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable SqlDataRecord TVPs on netstandard1.3. #802

Merged

Conversation

sergiivolchkov
Copy link
Contributor

This is a version of #801 applied to netstandard2 branch in Dapper.

Dapper's support for passing TVPs as IEnumerable (contrary to DataTable) using SqlMapper.AsTableValuedParameter or type handlers seem to work perfectly fine on .net core 1.x (tested on 1.1.2 runtime).

One caveat here is that Dapper dependency on System.Data.SqlClient has to be brought back for netstandard1.3.

@NickCraver
Copy link
Member

@sergiivolchkov looking good - thanks for the rebase! We'll try and get this merged in very soon, the netstandard2.0 build blocker on Appveyor is our current issue, but they've got an image so I hope to get it building right this week and merge down into master overall. No problem yanking this into the 2.0 branch as-is while I work on the overall merge though, here we go!

@sergiivolchkov
Copy link
Contributor Author

@NickCraver AppVeyor has been updated to VS 2017 15.3 Preview 3 + .NET Core SDK 2.0 Preview 2, according to appveyor/ci#1629, now tests for this PR finally pass - although this required some extra changes not relevant to the original intent for this PR.

One thing that is especially worth pointing out was that a lot of tests relying on TypeHandlers were randomly failing for me, on different target frameworks (including net452), although they would always pass when run individually. I believe it might be related to xUnit 2's default behavior to parallelize by class (see https://xunit.github.io/docs/running-tests-in-parallel.html), and this did not work well with test cases using SqlMapper.ResetTypeHandlers (e.g. some test cases in TypeHandlerTests and ParameterTests are resetting the handlers and thus break other concurrently running test cases setting their own handlers).
After disabling parallel runs, first by an option to dotnet xunit and after by using CollectionBehavior assembly attribute, the tests became stable.

@NickCraver
Copy link
Member

@sergiivolchkov rather than disabling of all tests, it's better to use collections to disable parallelization of those tests. We'd never want to disable parallelization overall...as that'll hide issues.

@NickCraver NickCraver merged commit a9560b3 into DapperLib:netstandard2 Aug 15, 2017
@NickCraver
Copy link
Member

Merging in, prepping for a new release - thanks!

@sergiivolchkov
Copy link
Contributor Author

@NickCraver unfortunately I have not really succeeded in making the tests completely stable, you can see that AppVeyor build for my latest commit to this PR still failed. If I remember correctly, my idea to introduce two test collections, TypeHandlerTests for test cases doing ResetTypeHandlers and QueryCacheTests for test cases doing PurgeQueryCache, did not cover all possibilities of shared state updated in parallel test threads, so maybe putting them into a single collection could be a better option.
Anyway, thanks for merging the PR! Looking forward to the release of Dapper with netstandard2.0 support.

@sergiivolchkov sergiivolchkov deleted the netstandard1.3-sqldatarecord branch August 15, 2017 20:22
@NickCraver
Copy link
Member

@sergiivolchkov Yep, I'm aware :) I'm working on adding functionality to xUnit to support these kinds of global test situations now.

@NickCraver
Copy link
Member

FYI: I opened a PR with xUnit to support these types of cases: xunit/xunit#1411

@NickCraver
Copy link
Member

Heads up: this is now on NuGet: https://www.nuget.org/packages/Dapper/1.50.4-alpha1-00070

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants