Remove dependencies from Microsoft.Net.Test.Sdk #1764
Comments
|
@tmat those are necessary dependencies for the testplatform to function. This is a meta package and the purpose it to bring in these transitive dependencies via the meta package. Please use the following option to instruct the testplatform to load the dll.config of the test sources and thus picking up the versions of these dlls that your tests are specifying in their dll.config files False |
|
@ShreyasRmsft Why are they necessary? Why does TestPlatform need to add these dependencies to test projects? |
|
@tmat they are required by the testhost process and the way we leverage these dependencies in the dotnet core world is via the meta package. |
|
@singhsarab to provide a more detailed explanation |
Just curious, did you get into issues where there were conflicts because of these dependencies? If yes, could you please share a repro project with us? |
Indeed. To repro, clone repository https://github.com/tmat/roslyn and checkout branch You'll see the following errors: If you change this line in to The |
|
If you need some files to be copied to the output directory of the test project, you can do so without adding nuget dependencies. Include these files in |
|
@tmat We will be looking into the options we have based on your suggestions. Shreyas and I will keep you posted. [Edit]: While we are at it, to speed up our investigations, can you share this custom 15.9.0-dev2 package that you have created ? |
|
The Microsoft.NET.Test.Sdk package is a metapackage. Its job is to bring in all the dependencies we need to run tests. If you go expand on the transitive dependencies of this package you see that it depends on certain commonly used nuget packages. For instance Newtonsoft.Json Now the Microsoft.NET.Test.Sdk specifies in it’s nuspec file it needs a minimum version of its dependencies and this is applied transitively to all sub dependencies. Anything lesser could destabilize or cause the testplatform to stop working. Ideally any user code that references an older version ends up using the version the platform mandates as minimum unless a separate appdomain is created for running tests. Now using a newer version should be acceptable as compat will generally be maintained with newer versions. The problem we believe you are facing is that your test project is trying to take a dependency on a lower version of one of these dependencies and the nuget restore fails with the following message: D:\R2\src\CodeStyle\CSharp\Tests\Microsoft.CodeAnalysis.CSharp.CodeStyle.UnitTests.csproj : error NU1605: Detected package downgrade: System.Runtime.InteropServices from 4.3.0 to 4.1.0. Reference the package directly from the project to select a different version. Now let’s say we get past this and somehow allow you to take a dependency on an older version than the minimum mandated by the Microsoft.NET.Test.Sdk package. When you run your tests without the testplatform creating a new appdomain (We have seen multiple issues with appdomains and will soon be making the default execution flow to be without creating a new appdomain) the older version will get loaded and the testplatform execution is likely to fail. I have a sample project here that you can try running to see the consequences. Now let’s say somehow we do allow you to run your tests in a separate appdomain, you will still end up with a problem running your tests after publishing the test project. Therefore it is best for us to warn the user upfront about this if ever a lower versioned dependency is added. Can you please either share this custom 15.9.0-dev2 package you have created or yourself try it on the sample project I have created on GitHub and share the results? |
|
@ShreyasRmsft The links to the project that you listed above actually point to the metapackage definition. The package is here: https://dotnet.myget.org/feed/roslyn-tools/package/nuget/Microsoft.NET.Test.Sdk I have simply removed all its dependencies, so none are copied to the output directory. What functionality in test platform needs these dependencies? Running tests from Test Explorer in Visual Studio works just fine without the dependencies. |
|
@tmat apologies, fixing the links in a minute I have fixed the links. please take a look |
|
The test project references Newtonsoft.Json that's not compatible with the TFM (netcoreapp2.1). So that's not really a valid scenario. That said I get your point that if the Test Platform needs newer version of Newtonsoft.Json than the test project specifies, you might run into issues. That's why the Test Platform component that needs to be loaded into the process that runs the tests needs to have minimal set of dependencies and certainly not on any particular version of Newtonsoft.Json. You can't be forcing test projects to auto upgrade their dependencies. By doing so you are subverting the customer's intent to test their code against exactly the versions of these dependencies they want to and effectively making it possible for a bug to slip thru their tests. Hence my question above that you haven't answered: "What functionality in test platform needs these dependencies?" |
|
@tmat Totally agree with you on
Newtonsoft is for serializing and deserializing messages used for communication between the testplatform/datacollector/testhost processes. The others are transitive dependencies of packages we use. You can walk through this dependency tree on the nuget package dependencies section here
|
|
@tmat we may have found a way to reduce the minimum version of system.runtime.interopservices required. I'm testing out the changes. Will get back to you once I've validated the solution |
|
I attached to one of our test projects, running tests from VS 15.9 Test Explorer on .NET Framework. The debugger shows 3 AppDomains in the Modules window:
Not sure why the last one is needed, but it seems that Test Platform is only loaded into the first one, while the test code is loaded into the second one. This is what I’d expect as it allows to isolate the dependencies of the TP from the dependencies of the tests being run and thus we do not need them to be compatible with each other. To me this design is sound and allows us to remove all the package dependencies from Microsoft.Test.Sdk. The same isolation can be implement on .NET Core using AssemblyLoadContext. You can load the test assembly and its dependencies to one context and the Test Platoform assemblies to another one, isolating their dependencies. One oddity that we should also look at is the xunit.runner.visualstudio.testadapter.dll dependency on Microsoft.VisualStudio.TestPlatform.ObjectModel.dll (the assembly has a reference). Seems like that shouldn’t be there. It doesn’t seem that this assembly reference is actually used at runtime in the common test run scenarip as Microsoft.VisualStudio.TestPlatform.ObjectModel is not loaded to Microsoft.Build.Tasks.Git.UnitTests AppDomain. It would be better if the reference wasn’t even there. The list of modules by AppDomain: Testhost.x86.exe xunit.runner.utility.net452 Microsoft.Build.Tasks.Git.UnitTests |
I think that would be a mistake. What issues are there with AppDomains? |
|
@tmat the test sdk package released after the above checkin should solve you issue. Can you generate a local package and verify if the fix works for you? |
|
Simply changing the version of one dependency does not resolve this issue. It might address the problem with the specific snapshot of the Roslyn repo, but that's just one problem. The package still has the same dependencies after that change. |
SummaryMain concern: The ability to be able to test any version of a dll or dll having dependency, via TestPlatform v2. Problem: Testhost has a bunch of dependencies, targeting specific versions, for example NewtonSoft.Json (currently 9.0.0.0). If the user wants to test NewtonSoft.Json 8.0.0.0, it is not possible given Test host process requires a higher version. There are 2 scenarios and possible solutions that need to be tested:
The solution discussed above suggest that testhost dependencies should be isolated from the test project’s dependencies. We will try to experiment using these solutions and see if we can make changes without impacting performance. Test platform currently has dependency on relatively older or min versions for all the dependencies, which allows users to use higher versions of these dependencies. Hence, there is no immediate need to react and expedite the changes based on our hypothesis unless there is real customer ask. That said, if any of dependencies in question, need/are to be upgraded to a higher version, we will run into issues. Hence long term, we will try to isolate, given the possible solutions discussed above. |
|
@tmat we are mainly concerned about the .NET Core scenario here, right? |
|
@ViktorHofer We are concerned about both desktop and core equally. Many of our repos target desktop. |
|
I'm hitting this now too in corefx. We recently enabled project restore (in a separate branch yet) and when referencing Have you thought about @tmat's initial proposal to copy dependencies from the package's tools directory to the output directory via an msbuild file? In example: https://github.com/dotnet/arcade/blob/master/src/Microsoft.DotNet.RemoteExecutor/src/Microsoft.DotNet.RemoteExecutor/build/Microsoft.DotNet.RemoteExecutor.targets#L5-L6 |
|
Also after reading the full conversation I assume that work hasn't yet started to isolate the testhost from test assemblies and their dependencies? @cltshivash I believe this is a bug and should be marked as such. Whenever dependencies are needed app-local which conflict with what is used by the testhost, you will most likely hit issues. |
|
@ViktorHofer @tmat I am closing this one as the current set of dependencies point to the minimum versions. I have created another issue to track issues/improvements while trying integrate with Arcade I am closing this issue, please let me know if you have any concerns. Thanks, |
|
Can anyone share status of this? I believe it is not fixed as my "Microsoft.NET.Test.Sdk": {
"type": "Direct",
"requested": "[16.9.1, )",
"resolved": "16.9.1",
"contentHash": "9acz3fExifstaoKTAvHVwRGMFrtb1QLlT6KfxOFTYM4dksuzwpkApjt0xP+yJcuRsPcf14F1b0Du3GgfKZWHJw==",
"dependencies": {
"Microsoft.CodeCoverage": "16.9.1",
"Microsoft.TestPlatform.TestHost": "16.9.1"
}
},
// ...
"Microsoft.TestPlatform.TestHost": {
"type": "Transitive",
"resolved": "16.9.1",
"contentHash": "j/lZDlkuoUJ+lRJXOXLJpwUGXmko5/woAPo/hN6QhFRo0J5wscQPoPJ1isvXpB4Iw7x7A3jYllxR5QjV3cMlRw==",
"dependencies": {
"Microsoft.TestPlatform.ObjectModel": "16.9.1",
"Newtonsoft.Json": "9.0.1"
}
},
// ...
"Newtonsoft.Json": {
"type": "Transitive",
"resolved": "9.0.1",
"contentHash": "U82mHQSKaIk+lpSVCbWYKNavmNH1i5xrExDEquU1i6I5pV6UMOqRnJRSlKO3cMPfcpp0RgDY+8jUXHdQ4IfXvw==",
"dependencies": {
"Microsoft.CSharp": "4.0.1",
"System.Collections": "4.0.11",
"System.Diagnostics.Debug": "4.0.11",
"System.Dynamic.Runtime": "4.0.11",
"System.Globalization": "4.0.11",
"System.IO": "4.1.0",
"System.Linq": "4.1.0",
"System.Linq.Expressions": "4.1.0",
"System.ObjectModel": "4.0.12",
"System.Reflection": "4.1.0",
"System.Reflection.Extensions": "4.0.1",
"System.Resources.ResourceManager": "4.0.1",
"System.Runtime": "4.1.0",
"System.Runtime.Extensions": "4.1.0",
"System.Runtime.Serialization.Primitives": "4.1.1",
"System.Text.Encoding": "4.0.11",
"System.Text.Encoding.Extensions": "4.0.11",
"System.Text.RegularExpressions": "4.1.0",
"System.Threading": "4.0.11",
"System.Threading.Tasks": "4.0.11",
"System.Xml.ReaderWriter": "4.0.11",
"System.Xml.XDocument": "4.0.11"
}
},
// ...This is very inconvenient because we don't use Hopefully, I'm not missing something. I have only limited knowledge of NuGet. Edit: Is there possibly a workaround for this? I have tried changing: <PackageReference Include="Microsoft.NET.Test.Sdk" Version="16.9.1" />to <PackageReference Include="Microsoft.NET.Test.Sdk" Version="16.9.1">
<PrivateAssets>all</PrivateAssets>
</PackageReference>but that does not seem to help. |
|
Why is the Test Platform not using System.Text.Json instead of NewtonSoft.Json? |
|
@MichaelPuckett2 please see #2488 |
|
This issue is still exists, and is preventing us from adopting The test platform should not be influencing the dependencies used by the code being tested. Adopting S.T.J within the current architecture simply trades testing with the wrong version of Newtonsoft.Json to testing with the wrong version of S.T.J. |
|
@fowl2 As you are using MSTest, why not switch to MSTest runner? You could get rid of |
|
@Evangelink apologies - vstest vs MSTest confusion strikes again. It seems like MSTest has the same problem, I guess over to https://github.com/microsoft/testfx? eg. the notionally "empty" csproj: <Project Sdk="MSTest.Sdk/3.6.4">
<PropertyGroup>
<TargetFramework>net48</TargetFramework>
<EnableMSTestRunner>true</EnableMSTestRunner>
<OutputType>Exe</OutputType>
</PropertyGroup>
</Project>Ends up with a bunch of packages installed/available/referenced:
Including |
|
You would be better opening an issue on https://github.com/microsoft/codecoverage because this is a deps on Code Coverage tool. In the meantime, you can add |
|
I just learned that code coverage is keeping the dependency to be compatible with vstest apparently. So once we get approval to stop supporting all .net up to net8, and stay with just the officially supported TFMs both deps can to be updated. (the update is in this PR #10565 ) |
The Microsoft.Net.Test.Sdk package depends on the following packages, which causes these dependencies to flow to unit tests, potentially causing conflicts with other dependencies that the tests have. The test runner should not leak its implementation details. Please remove all these dependencies from the package nuspec.
The text was updated successfully, but these errors were encountered: