The build failures are all from crypto tests, nothing in your work is failing as far as I can see, the link from the logs is here https://mc.dot.net/#/user/dotnet-bot/pr~2Fdotnet~2Fcorefx~2Frefs~2Fpull~2F36123~2Fmerge/test~2Ffunctional~2Fcli~2F/20190318.13

What are these tests? All the failing checks show the following:

If you click on the send to helix step a log type window will pop out from the right. if you read that there's a bit of text that says "results will be available from" followed by a url, ctrl+click that url and you'll get to the page I linked.

Thanks, @Wraith2.

@roji, what is DbDataReaderExtensions.Facade.cs file and why it's good to modify it if it isn't used? Shouldn't it be removed instead?

@roji, what is DbDataReaderExtensions.Facade.cs file and why it's good to modify it if it isn't used? Shouldn't it be removed instead?

This indeed seems like a leftover - it contains a backwards compatibility shim for the new resultset metadata API introduced in .NET Core 1.x. But I'm not sure I have the whole story and it's probably better to not go into it right now. Since it does contain a DbDataReaderExtensions class, it seems to make sense to a partial to it, in case it does get compiled in some scenario.

(BTW thanks for the super quick turnaround on the PR!)

@roji, what is DbDataReaderExtensions.Facade.cs file and why it's good to modify it if it isn't used? Shouldn't it be removed instead?

This indeed seems like a leftover - it contains a backwards compatibility shim for the new resultset metadata API introduced in .NET Core 1.x. But I'm not sure I have the whole story and it's probably better to not go into it right now. Since it does contain a DbDataReaderExtensions class, it seems to make sense to a partial to it, in case it does get compiled in some scenario.

(BTW thanks for the super quick turnaround on the PR!)

@saurabh500 I know this was some time ago, but do you have any insights on DbDataReaderExtensions.Facade.cs?

The DbDataReaderExtensions and façade version implement the same things in different ways, the currently included version defers to the IDbColumnSchemaGenerator interface which is in desktop 4.7.1 onwards and façade does the work directly and was originally conditionally included on net45. I'd guess it's a historic polyfill for desktop and can possibly be excluded from this repo.

@saurabh500 I know this was some time ago, but do you have any insights on DbDataReaderExtensions.Facade.cs?

@divega The facade was added so that while using .Net Core and targeting a .Net framework version which didn't have the new API, the facade would kick in, and use the DataTable to construct IDBColumnSchema. It was added for backward compat.

@saurabh500 I know this was some time ago, but do you have any insights on DbDataReaderExtensions.Facade.cs?

@divega The facade was added so that while using .Net Core and targeting a .Net framework version which didn't have the new API, the facade would kick in, and use the DataTable to construct IDBColumnSchema. It was added for backward compat.

@saurabh500 the confusing part is there are actually two DbDataReaderExtensions defining GetColumnSchema():

1. DbDataReaderExtensions.cs. GetColumnSchema() checks if the reader implements IDbColumnSchemaGenerator and calls into that, otherwise throws.
2. DbDataReaderExtensions.Facade.cs. GetColumnSchema() calls the traditional GetSchemaTable() and translates to the new DbColumn (so a backwards compat shim). This file isn't referenced in the csproj.

As @Wraith2 wrote above, I'm guessing the 2nd was meant for .NET Framework, since in .NET Core pre-2.0 DataTable didn't exist. However, I'm not sure if there's any sense in keeping this in the corefx repo today.

@roji re DbDataReaderExtensions.Facade.cs, can you please create a separate issue so that we remember to follow up? Talked to @bricelam the other day about the possibility of using the code there as a fallback if IDbColummnSchemaGenerator isn’t implemented, but obviously we need to understand things better.

@divega opened #36235

@TheBlueSky, @roji, @ajcvickers & everyone, I spoke with @terrajobst in person:

1. It is a minor issue, but the preference is that we come up with a new name for the sponsor class on System.Data that is different from DbDataReaderExtensions. Having two classes with the same name on different namespace can cause some confusion. Maybe a little less if they are just extension method sponsor classes, but still. Here are a few ideas: DataReaderExtensions (drops 'db', still specific for DbDataReader methods), DataExtensions (maybe too general), DataCommonExtensions (extends types form Data.Common). Thoughts?

2. We can go ahead and put this under *.cs, without trying to special-case so that it is only included for netcoreapp. I will create another issue Created issue https://github.com/dotnet/corefx/issues/36243 to follow up with @terrajobst to make sure we do the right thing re adding this to .NET Standard 2.1, making sure it does not break anything on .NET Framework, etc.

For 1, for the same reason that having the same name doesn't matter much for extension methods (we do it in EF Core) it also doesn't matter much what the name is at all. So I say let's go with DataCommonExtensions so we can get this checked in. If we have to rename before we ship then so be it. (I believe this is the only decision preventing @roji from merging.)

.NET Framework and UAP builds fail. They pass when I ran them manually on my machine (works on my machine™)... I used the flow I found in https://github.com/dotnet/corefx/wiki/Build-and-run-tests; e.g., clean, rebuild, running the tests. What am I missing?

The tests are trying to find the extensions and failing, the tests need to be netcoreapp specific.

System\Data\Common\DbDataReaderTest.cs(187,49): error CS1503: Argument 1: cannot convert from 'string' to 'int' [D:\a\1\s\src\System.Data.Common\tests\System.Data.Common.Tests.csproj]
System\Data\Common\DbDataReaderTest.cs(202,46): error CS1503: Argument 1: cannot convert from 'string' to 'int' [D:\a\1\s\src\System.Data.Common\tests\System.Data.Common.Tests.csproj]
System\Data\Common\DbDataReaderTest.cs(218,47): error CS1503: Argument 1: cannot convert from 'string' to 'int' [D:\a\1\s\src\System.Data.Common\tests\System.Data.Common.Tests.csproj]
System\Data\Common\DbDataReaderTest.cs(237,46): error CS1503: Argument 1: cannot convert from 'string' to 'int' [D:\a\1\s\src\System.Data.Common\tests\System.Data.Common.Tests.csproj]
System\Data\Common\DbDataReaderTest.cs(255,47): error CS1503: Argument 1: cannot convert from 'string' to 'int' [D:\a\1\s\src\System.Data.Common\tests\System.Data.Common.Tests.csproj]
System\Data\Common\DbDataReaderTest.cs(274,50): error CS1503: Argument 1: cannot convert from 'string' to 'int' [D:\a\1\s\src\System.Data.Common\tests\System.Data.Common.Tests.csproj]
System\Data\Common\DbDataReaderTest.cs(289,49): error CS1503: Argument 1: cannot convert from 'string' to 'int' [D:\a\1\s\src\System.Data.Common\tests\System.Data.Common.Tests.csproj]
System\Data\Common\DbDataReaderTest.cs(304,48): error CS1503: Argument 1: cannot convert from 'string' to 'int' [D:\a\1\s\src\System.Data.Common\tests\System.Data.Common.Tests.csproj]
System\Data\Common\DbDataReaderTest.cs(319,60): error CS1503: Argument 1: cannot convert from 'string' to 'int' [D:\a\1\s\src\System.Data.Common\tests\System.Data.Common.Tests.csproj]
System\Data\Common\DbDataReaderTest.cs(338,47): error CS1503: Argument 1: cannot convert from 'string' to 'int' [D:\a\1\s\src\System.Data.Common\tests\System.Data.Common.Tests.csproj]
System\Data\Common\DbDataReaderTest.cs(353,46): error CS1503: Argument 1: cannot convert from 'string' to 'int' [D:\a\1\s\src\System.Data.Common\tests\System.Data.Common.Tests.csproj]
System\Data\Common\DbDataReaderTest.cs(368,47): error CS1503: Argument 1: cannot convert from 'string' to 'int' [D:\a\1\s\src\System.Data.Common\tests\System.Data.Common.Tests.csproj]
System\Data\Common\DbDataReaderTest.cs(383,47): error CS1503: Argument 1: cannot convert from 'string' to 'int' [D:\a\1\s\src\System.Data.Common\tests\System.Data.Common.Tests.csproj]
System\Data\Common\DbDataReaderTest.cs(398,47): error CS1503: Argument 1: cannot convert from 'string' to 'int' [D:\a\1\s\src\System.Data.Common\tests\System.Data.Common.Tests.csproj]
System\Data\Common\DbDataReaderTest.cs(413,48): error CS1503: Argument 1: cannot convert from 'string' to 'int' [D:\a\1\s\src\System.Data.Common\tests\System.Data.Common.Tests.csproj]
System\Data\Common\DbDataReaderTest.cs(436,48): error CS1503: Argument 1: cannot convert from 'string' to 'int' [D:\a\1\s\src\System.Data.Common\tests\System.Data.Common.Tests.csproj]
System\Data\Common\DbDataReaderTest.cs(452,56): error CS1503: Argument 1: cannot convert from 'string' to 'int' [D:\a\1\s\src\System.Data.Common\tests\System.Data.Common.Tests.csproj]
System\Data\Common\DbDataReaderTest.cs(470,47): error CS1503: Argument 1: cannot convert from 'string' to 'int' [D:\a\1\s\src\System.Data.Common\tests\System.Data.Common.Tests.csproj]
System\Data\Common\DbDataReaderTest.cs(484,47): error CS1503: Argument 1: cannot convert from 'string' to 'int' [D:\a\1\s\src\System.Data.Common\tests\System.Data.Common.Tests.csproj]
System\Data\Common\DbDataReaderTest.cs(485,46): error CS1503: Argument 1: cannot convert from 'string' to 'int' [D:\a\1\s\src\System.Data.Common\tests\System.Data.Common.Tests.csproj]
    0 Warning(s)
    20 Error(s)

@Wraith2, not as per the last decision from @divega (#36123 (comment))
Also, shouldn't that fail on my dev machine too?

only if you run the tests on that framework which isn't the default, I think the updated docs are here: https://github.com/dotnet/corefx/issues/35661

@Wraith2 So I am guessing the command to build just the netcorapp tests should be:

 build -buildtests -test -f netcoreapp

netcoreapp is the default (since we're working on corefx afterall…) so if you just want netcoreapp you don't have to specify the framework. But yes that looks right to me.

In my PR's I don't change surface area and I'm quite careful about switching in of netcore/netstandard specific functionality so I rely on the build -allconfigurations to pick up compile problems like this and then the CI to flag test issues.

@Wraith2 Thanks a lot (I am in the process of warming up to my first PR, I hope) - and how do you mark tests "netcoreapp only" ?

I found this:

   [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotWindowsNanoServer), nameof(PlatformDetection.IsNotArmProcess))] 
    [PlatformSpecific(TestPlatforms.Windows)]

Usually you'd put framework specific things in suffixed files, so in this PR you'll see that there's a System.Data.Common.netcoreapp.cs file and the project file has been setup so that it is only included in netcoreapp builds, the same applies for other items..

The conditions you've found are a different axis, they're the platform axis not the framework axis. Still valid and useful but not for what you're looking for right now.

only if you run the tests on that framework which isn't the default, I think the updated docs are here: #35661

I ran the build for each platform (.NET Core, .NET Framework, and UAP) in both Debug and Release configurations, then built and ran the test for each platform; however, I have not got that error (cannot convert from 'string' to 'int'). The CI builds are, obviously, doing something I'm not doing, but cannot figure it out.

Also, this does not answer @divega comment that this should not be .NET Core specific.

Ok, then the question becomes why aren't those overloads being found?
Looking at it I can't see the tests importing the System.Data namespace.

@TheBlueSky @Wraith2, sorry, but my understanding of how this code is built and shipped has evolved a bit in the last few days. See
https://github.com/dotnet/corefx/issues/36235#issuecomment-475838727, for example. I still believe DataReaderExtensions.cs doesn’t need to be netcoreapp.cs or be conditionally included in the project, but not because that way the methods will show up in other TFMs. In fact, all the code in the project is used for .NET Core.

Hence, I agree that the new tests need to somehow only run on .NET Core. Maybe the way new methods on DbProviderFactories are tested can serve as an example of the pattern to follow, since the situation of those new methods is very similar to what we are adding here.

Otherwise, we can ask for assistance from someone with more experience on working on this repo.

@divega @ajcvickers re the naming of the extensions class, like @ajcvickers I don't think this is extremely important. However, even if a second DbDataReaderExtensions class would be confusing, IMHO it's still the best option, as it maintains the rule that extension classes are named exactly after the classes they extend, plus an "Extensions" suffix. DataReaderExtensions.cs seems even a bit more confusing as a second DbDataReaderExtensions, but if the latter really isn't an option, then DataCommonExtensions sounds somewhat better. In any case I don't have very strong feelings about this, and DataReaderExtensions is also acceptable.

As to the CI test failures I'm just as mystified as the rest of you - @TheBlueSky affirms that they pass locally, and they logically should be working on other TFMs as the extension are supposed to appear there. @divega some assistance may help.

Re the name of the sponsor class, I think either is fine. DbDataReader is the base class. In ADO.NET the pattern is that all derived classes have a different prefix. The extensions technically target the base class, but in practice apply to all DataReaders.

I will try to get someone that can provide guidance.

@divega, I looked at DbProviderFactories tests. They way they are implemented is by including the test file when the target is .NET Core; i.e., conditionally including the file in the csproj file, similar to the way we did it here. This means that there will be no test for other targets. Shall I implement it this way?

@divega and @roji, what is the final name for the class now? :)

@TheBlueSky my understanding is that this is what we should do. We are effectively only adding this new extension methods to .NET Core. However, you may want to wait a bit to avoid more back and forth. @roji is following up internally to make sure our understanding is complete.

@divega, sure, I'll wait.

In the meantime, do you mind explaining to me if this repo is only for .NET Core, what does building against .NET Framework and UAP means? And what purpose does it serve?

@TheBlueSky I like the name you already have, DataReaderExtensions, for the reasons I explained above. Unless someone else feels strongly that that shouldn’t be the name, no need to change it.

I will let @roji close the loop on what configurations the code is built for, and what configurations are tested when he has finished collecting that information.

@TheBlueSky agree that we should condition out the tests for netcoreapp only. As the naming has been agreed upon, I think after this we can finally merge.

I admit I don't have a full understanding on exactly what is going on - here's an explanation from @ericstj on the CI builds and the different configurations:

When you build locally (eg: build.cmd) you are only building for a single vertical : netcoreapp on your current OS.

When the CI builds, it builds for all verticals as well all additional configurations that might not be tied to a specific vertical (eg: a configuration only for packaging).

The reason the official build is failing is because it is building the “netstandard” configuration for a test project and trying to use the API you’ve added. As I mentioned you did not add this API to the netstandard build.

I’d recommend that you make a UAP configuration for the tests, and condition out the new test additions so that they are only included for netcoreapp/uap. Alternatively you could factor into an entire new assembly if you happen to have business goals of shipping this new API to netstandard2.0 or desktop.

I'm still not sure in what sense the local uap build is different from the CI one. However, in any case the lack of tests will not affect the availability of the APIs in any way, so we should just move ahead with this.

@roji, thanks a lot. I'll rebase my local branch and push the changes later today.

To help me understand how thing are going on here, do you think you can answer my following question?

In the meantime, do you mind explaining to me if this repo is only for .NET Core, what does building against .NET Framework and UAP means? And what purpose does it serve?

In the meantime, do you mind explaining to me if this repo is only for .NET Core, what does building against .NET Framework and UAP means? And what purpose does it serve?

My understanding of how corefx exactly works is very partial (am also a bit of a newcomer to the repo). If I understand things correctly, netfx is no longer actually built from this repo (at least not the System.Data.Common part) - this is a leftover from days where the inbox System.Data.Common was replaced with out-of-band packages. However, a build config does exist for netfx so we need to take it into account (possibly for other parts outside of System.Data.Common).

The repo does build for UAP, but I'm not sure exactly how everything interacts and why the tests are failing.

@roji and @divega, I believe this is ready to be merged now. Do you have any other comments?

@roji Green light! 🚥

Thanks again @TheBlueSky! You had a lot of patience on this one 😆

Hooray!

You're welcome, @roji; it was fun :)

+1 Thanks @TheBlueSky for the contribution and for your patience!