Share more files and add MultiPartIdentifier tests #827
Conversation
|
Wonder if it would be good to add |
|
Perhaps. It's above my level to make that decision. It'll only affect .Net5 at the moment but moving to 5 is a good idea for sql perf not least because of dotnet/runtime#1637 |
|
@David-Engel @cheenamalhotra any chance of getting this and the other couple of recent merge PR's reviewed and merged? There's more to do but having too many PR's open makes it confusing to work on. |
src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj
Outdated
Show resolved
Hide resolved
@Wraith2 Sorry for the delay. Holidays + vacations has made for less time to get things done. Thanks for the contributions! I'll try and look at the others ASAP. |
|
Netfx subtype project changes reverted. |
| @@ -55,6 +56,7 @@ | |||
| <Compile Include="SerializeSqlTypesTest.cs" /> | |||
| <Compile Include="TestTdsServer.cs" /> | |||
| <Compile Include="SqlHelperTest.cs" /> | |||
| <Compile Include="..\..\src\Microsoft\Data\Common\MultipartIdentifier.cs" /> | |||
There was a problem hiding this comment.
I understand why you did this to be able to access this source code here, but this approach will not give code coverage to netcore\M.D.S project csproj (as that is what we measure). This class will be executed from within this project.
You could instead provide access to test namespace by adding assembly tag:
[assembly:InternalsVisibleTo("")]
You'd not need ADP overrides too.
There was a problem hiding this comment.
In general I avoid InternalsVisibleTo because it's deprecated in netcore and wasn't a good idea to begin with. While it doesn't give code coverage it doesn't decrease code coverage either and it does increase test coverage so that the behaviour, which is most important, is now tested and deviations can be caught.
There was a problem hiding this comment.
Is there an official doc on it's deprecated status or why it's not a good idea? I mean there should be a better way of referencing internals if this is going to be deprecated! I don't think we need to limit ourselves.
If not, I'd rather increase code coverage than use this approach.
There was a problem hiding this comment.
Good question. I know there are many placed in corefx/runtime where it's been removed and in general they don't seem to want to use it but i don't have concrete information about why, some mention of build perf and trimming problems. I've asked on the discord if there's an official opinion on it's use.
There is the obvious negative that it adds the name of the test assemblies irrevocably into the released binary which is unneeded and since there is no strong name capability on core anything that wants to can see those names and pretend to be that assembly allowing them to screw with internals.
I can understand why you'd want to increase coverage but contributors still can't see any of those coverage numbers and as I said I've increased test coverage., Requiring me to hit a target I can't evaluate seems unreasonable.
There was a problem hiding this comment.
It's actually a work in progress to be able to report coverage on PRs as they come along, there's just so much activity on higher priority. But we regularly run code coverage internally (link here). I was saying in reference to new tests that we are adding, they should contribute to code coverage, and doesn't sound like we have large concerns over using the assembly tag.
it adds the name of the test assemblies irrevocably into the released binary
We already have a few assembly tags in other classes for internal testing, so it's not going to make additional difference.
There was a problem hiding this comment.
I've had a look and there are no test related InternalsVisibleTo attributes in the library. I also tried to look at the code coverage from your link but all I can find is an overall percentage and a .coverage file which I can't open.
There was a problem hiding this comment.
Sorry my bad. I think the alternate approach is using Reflection to access internal classes if not this way.
You're right, we should not use InternalsVisibleTo, as it causes assembly signing issues too.
It just hurts to lose code coverage.
|
Any chance this can get merged and then when code coverage support is ready the tests can be changed around to use internals visibility if needed? I'd really like to get my open PR's in before the holiday so I've got a clean start on some of the more complex files. |
|
Excellent, thanks 😁 |
Merged a couple of files into the shared location, MultipartIdentifier and DBConnectionPoolKey. Datastorage in netfx was not included in the project and was not used so i removed it. I've also added some tests for the MultipartIdentifier by including the file in the functional tests project and giving it some local context (ADP methods it calls) in the test class.