arcade: RemoteExecutor Swallows Errors and Throws Invalid Exceptions

Thanks for reporting @JimBobSquarePants and @vitek-karas for the analysis. Vitek is right that RemoteExecutor doesn’t set HostRunner (the path to the host to invoke) to the right value in cases where VSTest’s apphost is used. This didn’t show up for us in dotnet/runtime as we don’t use VSTest’s apphost feature as we supply our custom host.

As a simple fallback mechanism we could check if processFileName ends with (dotnet[.exe]) and if not, use the dotnet in the %PATH%.

For a more complete implementation, we should follow what Vitek proposes:

It should be using dotnet.exe - but this will be tricky because it will need to pick the right one. The best way would probably be to use location of the runtime running the test (this will be tricky to get completely right with single-file, but it’s possible).

An implementation of that could look like this:

1. Retrieve the location of the shared framework assemblies currently being used by the process.
2. Climb up the directory tree until a dotnet host is found.
3. If none is found, use the one in the PATH.

@vitek-karas do you think we should stop at some point climbing up the directory tree? For self-contained application we need to fallback to %PATH% as a self-contained application doesn’t have a dotnet host. Right?

Thanks - I was able to repro it locally.

The command line it executes doesn’t seem to make sense to me:

F:\AppModel\repro\RemoteExecutorTests\ExecutorTest\bin\Debug\netcoreapp3.1\testhost.exe 
exec 
--runtimeconfig "F:\AppModel\repro\RemoteExecutorTests\ExecutorTest\bin\Debug\netcoreapp3.1\ExecutorTest.runtimeconfig.json" 
--depsfile "F:\AppModel\repro\RemoteExecutorTests\ExecutorTest\bin\Debug\netcoreapp3.1\ExecutorTest.deps.json" 
"F:\AppModel\repro\RemoteExecutorTests\ExecutorTest\bin\Debug\netcoreapp3.1\Microsoft.DotNet.RemoteExecutor.dll"
<args to the remote executor - not interesting>

This would work if it were running “dotnet.exe exec …”, but I doubt testhost recognizes “exec” (I could be wrong though).

In any case the command line won’t work because the special host parameters must be first on the command line: --runtimeconfig and so on. Since testhost.exe is an apphost it doesn’t inherently recognize exec on the host level, and so it runs itself as if there were no special parameters on the command line. And thus it tries to read the file testhost.runtimeconfig.json which doesn’t exist (the valid=[1] is misleading, internally the host treats missing runtime config as valid) - so it ends up with empty runtime config which means no framework references and thus tries to run as self-contained - which it’s not and so on.

I tried running the same command line via dotnet.exe exec and that works (well at least it runs the code).

I don’t know how this passes tests (if they run) in Arcade repo and/or how it’s used in a case where it would actually work. But as is, this seems completely broken.

Also - I which we could update the testhost.exe to some “recent” version, it’s 2.1 (I know we probably need it to be able to run 2.1 tests, unfortunately). In this case it means that the tracing from the testhost.exe is not visible in the trace file since it doesn’t recognize COREHOST_TRACEFILE which was added in 3.0.

I think somebody from Arcade should comment on what is the intended command line. I can definitely help with figuring out what it should be in reality to get all the desired behaviors.

Couple of notes about what I think it SHOULD be doing:

• Using testhost.exe won’t work because that is the VS test host runner, it does not support running the executor dll as a command line app
• It should be using dotnet.exe - but this will be tricky because it will need to pick the right one. The best way would probably be to use location of the runtime running the test (this will be tricky to get completely right with single-file, but it’s possible).

The break was probably introduced by changes to the VSTest platform - some time ago it switched from running the test via “dotnet.exe” to using the “testhost.exe”. But that’s been quite a while now if I remember correctly.

As for the rest of the comments - this is absolutely not a shipping product. And it’s expected that certain things won’t work, since we didn’t need them to work yet.

100% agreed. Even though it’s an open source project, there is no support policy attached to it. When I ported this library over from CoreFx couple years ago, I only did so to share the implementation with ML.NET, aspnetcore and later winforms. We try our best to resolve reported issues but RemoteExecutor was never intended to support every possible variant like single file applications, mobile, etc. As an example I removed the UWP support as well as we stopped testing it in CoreFx’s main branch.

• The application was run as a self-contained app because ‘C:\Users\igveliko.nuget\packages\microsoft.testplatform.testhost\16.5.0\build\netcoreapp2.1\x64\testhost.runtimeconfig.json’ was not found.
• If this should be a framework-dependent app, add the ‘C:\Users\igveliko.nuget\packages\microsoft.testplatform.testhost\16.5.0\build\netcoreapp2.1\x64\testhost.runtimeconfig.json’ file and specify the appropriate framework.

I think this nicely describes what happened and why it was trying to run the app as self-contained.

Haha, if only I read the test log 😃 But to be fair, this is a developer-centric error message, which is to a user (in this instance me or @JimBobSquarePants) carries little value, because there’s nothing the user can do to address the issue besides coming here and raise an issue.

As for the solutions mentioned above - I actually like the startup hook idea even more now. It would get us the exact same environment as the original test, and I don’t feel like this is too hacky anymore.

This approach shifts the onus of bootstrapping to the consumer of the RE. Whilst this may fair to suggest this direction for custom scenarios you’ll need to convince me why running a test with the RE from under VS is not a mainstream scenario.

I don’t have any problem with a partial fix.

I’m curious: In your comment you mention that it makes your coverage unreliable. Does it mean that it fails only sometimes, or that you have to avoid running certain configurations entirely because of this?

I just started to work on a minimalistic fix before @RussKie commented. There are more and more PR-s in ImageSharp with out-of-process tests, and our coverage became very unreliable. We need a fix ASAP. I don’t have the time and the knowledge for a full-scale solution using startup hooks.

I believe the most urgent thing is to get this fixed for the common Windows cases as simply and as quickly as possible. (Climb up to find dotnet.exe, keep original HostRunner if it’s not present.) This would unblock ImageSharp, and hopefully also @RussKie, without side effects on more exotic cases, and does not conflict with a future reimplementation with startup hooks. @vitek-karas will you accept such a PR?

Alternative idea: Remove readonly modifier from HostRunner, so users can configure it as needed.

It’s an environment variable read by the host - hostfxr.dll, I think (this isn’t my area). So you should be able to set it anywhere eg before launching dotnet test, or in your test just before launching the remote executor.