Can Newtonsoft.Json get updated to 11.x or 12.x?

[lg2de]

The package Microsoft.TestPlatform.TestHost has a dependency on Newtonsoft.Json, for target framework .NETCoreApp 2.1 it refers to version 9.0.1.

Would be possible to update the reference to at least version 11.x.

Why?
The package Newtonsoft.Json in version 9.0.1 has no target framework for .NETCore >= 2 or .NETStandard >= 2. When restoring packages for the solution, the direct dependency to Microsoft.NET.Test.Sdk causes indirect dependencies for several compatibility packages (e.g. System.Text.Encoding). This is because the latest target framework of Newtonsoft.Json is .NETStandard 1.0.
When forcing Newtonsoft.Json to version 11.0.2 in the project, the compatibility packages are not required and not restored.

We are using .net core 3.x and we want to limit the list of nuget packages in our internal package feed due to license review process. This is why we don't want to have all the small compatibility packages in our feed.

[nohwnd]

I think we will try to drop the dependency on Newtonsoft.Json and migrate to System.Text.Json. I did not have a chance to see how difficult that change will be, but hopefully not much. Would that help you with your review process?

[lg2de]

Yes, for the future this looks like a good solution.

I took a short look into the current usage of Newtonsoft.Json. I think it will take a lot of effort to replace it.

So, maybe it would be easier to upgrade the referenced version.

[nohwnd]

I quickly tried to replace the library and ran into many issues with projects still using netcoreapp1.0 I think we need to do a a major cleanup either way, because System.Text.Json does not support net451 and so on.

I need to clear up which versions we should be using and see what the impact is in either case.

[lg2de]

Updating to Newtonsoft.Json 11.0.2 looks very easy. Could this be added to 16.5.0?
For e.g. 17.0 you may schedule switch to System.Text.Json.

[nohwnd]

It can't 16.5.0 was released this morning. I am not sure what the release cadence is here, but imho this should be okay for patch release. I can talk with the team about 16.5.1, or at worst getting it into 16.6.0. We already agreed on using milestones here and in testfx so hopefully the process will be more transparent to you and others once we put that into practice.

[nohwnd]

@lg2de I did not have a chance to look into this yet, would you consider implementing this to see what the actual impact will be, and PRing? Then we can get it to 16.6, but otherwise this will not get in as we are focusing on performance and regressions in this sprint.

[lg2de]

The PR is created, @nohwnd.
Currently some build steps fail because they are (still) referring to Newtonsoft.Json 9.0.0. So far I could not identify where this expectation is coming from.

[plioi]

Is this really related to #2279? Why should consumers of a test framework experience VSTest's own internal dependencies in any way? If Discovery ran from the nuget cache like Execution does, none of these files would need to appear in the build output folder.

[ViktorHofer]

We deliberately didn't update the Newtonsoft.Json's version as that will force test projects to at least target VSTest's version. With AssemblyLoadContext and/or System.Text.Json this issue should go away.

[plioi]

@ViktorHofer Is there any hope of VsTest running discovery/execution from within the nuget packages folder rather than polluting the build output folder, long term? In the linked issue above, there was evidence that it was only touching the build output folder at discovery time, so fixing the root cause seems plausible.

I fear there's going to be a lot of treating-the-symptoms. This all seems to be downstream side effects from the decision to have the Test SDK work by overwriting the system under test, which is making me consider recommending that folks never use Test Explorer or dotnet test.

[ViktorHofer]

to have the Test SDK work by overwriting the system under test

Sorry can't follow, can you please ellaborate?

Is there any hope of VsTest running discovery/execution from within the nuget packages folder rather than polluting the build output folder, long term?

This sounds like a feature request. If it's feasible and make the product better, sure why not. Can you please create a new issue for that?

[plioi]

I think #2279 covers both questions.

[ezgambac]

This is still broken, and it's breaking source generator development when doing tests in azure dev ops in windows, as the source gen test code seems to depend on newtonsoft internally for a specific interface.
It's 2022, newtonsoft is already at version 13, and version 9 is from 2016, 6 years old and deprecated, yet this issue persists even with v17 of vstest.

[nohwnd]

We are keeping an old version to avoid upgrading tested projects to the newest version. Believe me I would like to upgrade it to the latest, but I can't because of back-compat.

[lg2de]

How about adding new version for next target framework?

If you plan to switch to System.Text.Json, what is the difference in terms of backward compatibility?

[nohwnd]

We build what we ship for .net and .net framework from the same codebase for all the TFMs we support, down to netcoreapp2.1. Newtonsoft.Json dependency is deeply rooted in our libraries (unfortunately). That is why we did not move to System.Text.Json. And we need a solution for .NET Framework, where we will have the same problem with System.Text.Json, even though it will probably be less visible as the user base on that TFM for that nuget is likely much smaller than for Newtonsoft.Json.

[plioi]

The JSON dependency issue is just a symptom of a much deeper problem ( #2279 ). As long as the discussion is about "what to do about Newtonsoft.Json" then there's absolutely no hope of the real problem being addressed.

There is no defensible reason for vstest to drop half of its own implementation details on top of the test project's own build output folder. It should not matter what version of Newtonsoft VS happens to use internally for internal purposes. It wasn't always like that. It started happening sometime around .net core 3. Newtonsoft is just the most noticable symptom of the problem because so many end users coincidenally need a dll of the same name at runtime, and it's one of the implementation details that vstest accidentally copy/pastes on top of end user build output.

When I type in the VS text editor, the VS text editor's implementation details don't get dumped on top of my build output. When I run under the debugger, the debugger's implementation details don't get dumped on top of my build output. Vstest is the only thing that does so.

There's evidence ( #2279 (comment) ) that we're hitting what was supposed to be a fallback edge case of looking for dependencies in the build output folder, as seen in that other thread. It was supposed to be finding the run-time dependencies over in the nuget cache, and that even appears to be the case during discovery but (unintentionally) not execution. If it went back to doing things like it did originally, then TestAdapter plugins would have the freedom to interact with the test assembly and its own dependencies the way they see fit, with no need for random dlls and other support files to be copy/pasted from vstest on top of the system being tested.

I'm honestly surprised this hasn't been a big deal-breaker internally at Microsoft. All .NET testing in the presence of vstest is suspect as it bears no resemblence to the dlls that will be shipped after a passing build.

All in all, VSTest should not use the test project's build output folder for its own needs. The test project's build output folder is sacrosanct and does not belong to VSTest.

[nohwnd]

@plioi What you talk about is also a symptom of a much deeper problem. It does not matter if we load Newtonsoft.Json from nuget cache of your output folder. (Unless your tests count the number of dlls in the bin or something). It will end up being loaded into the same process, and there is currently no isolation for running tests, in neither .NET and .NET Framework testhost. There is isolation into appdomains for Discovery, but not for Run (AFAIK).

So as far as I can say, it was always like that, at least since json serialization was used for IPC. And it is not a huge deal breaker, because you are free to upgrade to a newer version of the library by referencing it, or by getting it from transitive dependencies. On the other hand if we upgrade our shipped version, we might force your project to be different from what you intended it to be, that is why we are sticking to version 9, and our only other library dependency is nuget.frameworks.

The only solution to this would be adding isolation on the ObjectModel level, cutting out all additional dependencies from ObjectModel and only present that to the isolated test dll. That way test adapters, test frameworks and SUT are able to run the test independently from test platform.

[plioi]

Test frameworks are free to kick off test running in their own process. Only the TestAdapter needs to be "stuck" with the original process.

[nohwnd]

One more comment on the resolving of testhost. Resolving from bin is now the primary way, because it is faster, relies less on parsing of deps.json which has a bunch of edge cases. And it also allows test projects that are built with RID to run, without us providing additional deps paths, and additional deps files.

[nohwnd]

Test frameworks are free to kick off test running in their own process. Only the TestAdapter needs to be "stuck" with the original process.

Sure, but then then need to somehow communicate the results back, and if they use any serialization or communication library for that, they are back in square 1.

[plioi]

Agreed. Fixie for example farms out to a separate process and uses the built-in json parser to send messages between the processes over a named pipe. The test assembly is free to load any real dependencies it wants, except for those trampled by vstest.

[nohwnd]

But we (vstest) don't trample any dependencies. If your projects selects higher version, the higher version is used.

[plioi]

So it does trample them. And the solution to the trampling is to untrample them in this specific way, so long as the end user knows which dlls are silently trampled, now and into the future for all of time.

[nohwnd]

How can it trample what is not there? And if it is there, and uses newer version, that is then used. What is the real harm?

[nohwnd]

FWIW I took some time to upgrade TP to System.Text.Json again, and there are few major issues, most importantly there is no support for *DataMember attributes, which would force us to (1) annotate our models in ObjectModel with attributes from system.text.json. Or duplicate the type (2) definitions, and convert them after deserialization.

We definitely don't want to do (1). (2) might be feasible with some cs file linking magic, and compiler attributes. Still there would be overhead of creating each object twice. Or maybe inheritance could be used, but it is all pretty brittle. :/

[AArnott]

@nohwnd We need to get Newtonsoft.Json upgraded promptly. The 9.0.1 version that Microsoft.TestPlatform.TestHost is using today is triggering CG warnings.

[nohwnd]

@AArnott trying to do that in #3815

[nohwnd]

@AArnott Upgraded it in #3815 and asked our release team to publish a new public preview asap, so people can grab it and test it. We plan to lift the minimum supported tfms in 17.4 to net462 and netcoreapp3.1, and update other packages as well. And continue updating these packages, to use the oldest version that does not have security issue. Consuming projects should also update themselves.

Unfortunately there is no other way for us to do this (other than removing dependency on Newtonsoft.Json entirely), that would prevent us from upgrading the tested project in case they use older version of that nuget.