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.

Sign up for GitHub

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

Jump to bottom

Fixed testhost crash for net7 #4112

Merged
merged 10 commits into from
Nov 11, 2022
Merged

Fixed testhost crash for net7 #4112

merged 10 commits into from
Nov 11, 2022

Conversation

cvpoienaru
Copy link
Member

@cvpoienaru cvpoienaru commented Nov 4, 2022 •
edited
Loading

This PR fixes two issues:

  • Testhost crashing when using the in-proc wrapper to run net7 tests because dotnet couldn't correctly resolve the net7 runtime. Because we now run in-proc (meaning inside the TestWindowStoreHost process), certain environment variables such as DOTNET_ROOT and DOTNET_MULTILEVEL_LOOKUP are set. We cannot unset them because that would change the behavior of the TestWindowStoreHost process, but we certainly need them unset for dotnet runtime resolution to be successful. AppDomains provide no isolation since environment variables are shared at the process level. The simplest and most elegant solution is to set the VSTEST_WINAPPHOST_DOTNET_ROOT and this would provide successful resolution. More info about VSTEST_WINAPPHOST_DOTNET_ROOT can be found here.

Error here:

Microsoft.VisualStudio.TestPlatform.ObjectModel.TestPlatformException: Testhost process for source(s) 'C:\Users\<USER>\source\repos\TestProject12\TestProject12\bin\Debug\net7.0\TestProject12.dll' exited with error: You must install or update .NET to run this application.
App: C:\Users\<USER>\source\repos\TestProject12\TestProject12\bin\Debug\net7.0\testhost.exe
Architecture: x64
Framework: 'Microsoft.NETCore.App', version '7.0.0-rc.2.22472.3' (x64)
.NET location: C:\Program Files\Microsoft Visual Studio\2022\Preview\dotnet\runtime
The following frameworks were found:
  6.0.9 at [C:\Program Files\Microsoft Visual Studio\2022\Preview\dotnet\runtime\shared\Microsoft.NETCore.App]
Learn about framework resolution:
https://aka.ms/dotnet/app-launch-failed
To install missing framework, download:
https://aka.ms/dotnet-core-applaunch?framework=Microsoft.NETCore.App&framework_version=7.0.0-rc.2.22472.3&arch=x64&rid=win10-x64
. Please check the diagnostic logs for more information.
  • An unimplemented handler for raw messages got called on TW side when running in-proc. With out-of-proc, raw messages never reach TW since they aren't dequeued by the translation layer. Duplicating the same logic here, since TW wouldn't even know what to do with raw messages. This is not indicating a malfunction on either TP or TW side, but rather the lack of adherence to a convention. Even if this exception is logged in the output window, the tests are still successfully discovered/run.

Error here:

System.NotImplementedException: The method or operation is not implemented.
   at Microsoft.VisualStudio.TestWindow.Host.TestPlatformProvider.TestResponseHandler.HandleRawMessageAsync(String rawMessage)
   at Microsoft.VisualStudio.TestWindow.RemoteAgent.TestAgentBridgeCallback.HandleRawMessageAsync(String rawMessage)
   at Microsoft.VisualStudio.TestWindow.Logging.ILoggerExtensions.<CallWithCatchAsync>d__10.MoveNext()

@cvpoienaru cvpoienaru enabled auto-merge (squash) November 4, 2022 15:06
@nohwnd
Copy link
Member

nohwnd commented Nov 4, 2022

Please provide more information about the fix and the reasoning the description.

Evangelink
Evangelink reviewed Nov 7, 2022
View reviewed changes
Copy link
Member

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I said during our talk, this seems to be working but I am quite worried this will end up reverted like the DOTNET_ROOT update I did because we are too far away from understanding all the scenario.

Evangelink
Evangelink reviewed Nov 10, 2022
View reviewed changes
src/vstest.console/vstest.console.csproj
@@ -49,7 +50,7 @@
<ProjectReference Include="..\Microsoft.TestPlatform.ObjectModel\Microsoft.TestPlatform.ObjectModel.csproj" />
<ProjectReference Include="..\Microsoft.TestPlatform.Client\Microsoft.TestPlatform.Client.csproj" />
<ProjectReference Include="..\Microsoft.TestPlatform.CommunicationUtilities\Microsoft.TestPlatform.CommunicationUtilities.csproj" />
<ProjectReference Include="..\Microsoft.TestPlatform.PlatformAbstractions\Microsoft.TestPlatform.PlatformAbstractions.csproj" />
<ProjectReference Include="..\Microsoft.TestPlatform.PlatformAbstractions\Microsoft.TestPlatform.PlatformAbstractions.csproj" Aliases="Abstraction" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hack to work around the issue where nullable attributes are coming from both PlatformAbstractions and CoreUtilities. We have tried to solve it in a better way but that was leading to too complex refactoring. Using the alias trick helps to isolate what's coming from this assembly.

Note that I hope to remove such hack by using a source generator for the polyfill (@MarcoRossignoli do you remember the name of the project?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/Sergio0694/PolySharp

@Evangelink Evangelink disabled auto-merge November 10, 2022 17:04
Evangelink
Evangelink approved these changes Nov 10, 2022
View reviewed changes
Copy link
Member

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Please wait for approval from @nohwnd and @MarcoRossignoli before merging (I have disabled auto-merge).

@nohwnd
Copy link
Member

nohwnd commented Nov 11, 2022

I would prefer to not propagate the inherit flag all the way down to ProcessHelper, instead the code that sets the variables (in wrapper) should check what the preference is and either provide just what it was given (inherit=false), or merge it with the current set of env variables (inherit=true) and set that to ExternalEnvVariables. This way providing external env variables will work the same no matter if we are inheriting or not in terms of updates etc.

MarcoRossignoli
MarcoRossignoli approved these changes Nov 11, 2022
View reviewed changes
Copy link
Contributor

@MarcoRossignoli MarcoRossignoli left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls @cvpoienaru when you have a pair of hours can you add the explanation of the in-process working with this "details" here https://github.com/microsoft/vstest-docs/tree/main/RFCs

And maybe after link in the code here, for future devs, we're moving in different context and this repo doesn't have any information of the other world.

@cvpoienaru cvpoienaru enabled auto-merge (squash) November 11, 2022 14:39
@cvpoienaru cvpoienaru mentioned this pull request Nov 11, 2022
Closed
@cvpoienaru
Copy link
Member Author

Follow up: #4119

@cvpoienaru cvpoienaru merged commit b619739 into microsoft:main Nov 11, 2022
@cvpoienaru cvpoienaru deleted the dev/copoiena/testhost-crash-net7-preview branch November 11, 2022 15:26
@cvpoienaru cvpoienaru restored the dev/copoiena/testhost-crash-net7-preview branch November 11, 2022 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nohwnd nohwnd nohwnd approved these changes

@MarcoRossignoli MarcoRossignoli MarcoRossignoli approved these changes

@Evangelink Evangelink Evangelink approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cvpoienaru @nohwnd @MarcoRossignoli @Evangelink