Test projects do not convert correctly and result in TestContainer was not found error

[tomaustin700]

Hi,

I am attempting to convert a .net framework test project to core and I am running into the following issue. Once converted the project shows as a class library and no tests seem to be discovered. When a test is ran manually the test output is the following:

StreamJsonRpc.RemoteInvocationException: The following TestContainer was not found 'C:\... path to test dll'
   at StreamJsonRpc.JsonRpc.<InvokeCoreAsync>d__96`1.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.VisualStudio.TestWindow.Host.TestStoreHandler.<AddSourceBasedTestsAsync>d__23.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.VisualStudio.TestWindow.Extensibility.ILoggerExtensions.<CallWithCatchAsync>d__9.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at Microsoft.VisualStudio.Telemetry.WindowsErrorReporting.WatsonReport.GetClrWatsonExceptionInfo(Exception exceptionObject)
Executing test method: PatientLedger.PatientLedgerTests.TestClass.Test1
System.InvalidOperationException: Sequence contains no elements
   at System.Linq.Enumerable.Single[TSource](IEnumerable`1 source)
   at Microsoft.VisualStudio.TestWindow.Controller.TestContainerConfigurationQueryByContext.<GetTestContainerConfigurationsInternalAsync>d__8.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.VisualStudio.TestWindow.Controller.TestContainerConfigurationQuery.<GetTestContainerConfigurationsAsync>d__16.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.VisualStudio.TestWindow.Controller.TestRunConfiguration.<UpdateAsync>d__13.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.VisualStudio.TestWindow.Controller.RunOperation.<RunTestsAsync>d__15.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.VisualStudio.TestWindow.Controller.Operation.<<Execute>b__37_0>d.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.VisualStudio.TestWindow.Extensibility.ILoggerExtensions.<CallWithCatchAsync>d__10`1.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at Microsoft.VisualStudio.Telemetry.WindowsErrorReporting.WatsonReport.GetClrWatsonExceptionInfo(Exception exceptionObject)

I have uploaded a sample solution here: https://github.com/tomaustin700/TestConvertIssue

I am building try-convert locally as the current released version is missing the following fix #156

[jmarolf]

Initial look at this suggests that it is an MSTest issue. The version of MSTest being used fails to load on .NET Core but succeeds on framework

[tomaustin700]

Hi @jmarolf, thanks for investigating. Is this something that needs to be raised with the MSTest team?

Thanks

[dotnokato]

Hi @tomaustin700,
I've tried to reproduce this issue today using your sample solution and was not able.
I used try-convert built from commit 8d2600c.

Could you verify if the issue is still happening or was it fixed in the meantime?

[tomaustin700]

Hi @dotnokato
I have pulled master and rebuilt and I seem to be getting a slightly different exception now.

System.InvalidOperationException: Sequence contains no elements
   at System.Linq.Enumerable.Single[TSource](IEnumerable`1 source)
   at Microsoft.VisualStudio.TestWindow.Controller.TestContainerConfigurationQueryByContext.<GetTestContainerConfigurationsInternalAsync>d__8.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.VisualStudio.TestWindow.Controller.TestContainerConfigurationQuery.<GetTestContainerConfigurationsAsync>d__16.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.VisualStudio.TestWindow.Controller.TestRunConfiguration.<UpdateAsync>d__9.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.VisualStudio.TestWindow.Controller.RunOperation.<RunTestsAsync>d__15.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.VisualStudio.TestWindow.Controller.Operation.<<Execute>b__41_0>d.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.VisualStudio.TestWindow.Extensibility.ILoggerExtensions.<CallWithCatchAsync>d__10`1.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at Microsoft.VisualStudio.Telemetry.WindowsErrorReporting.WatsonReport.GetClrWatsonExceptionInfo(Exception exceptionObject)

How are you running try convert? I am using the -w argument on the solution directory - I'm not sure if that makes any difference.

Thanks.

[dotnokato]

Hi @tomaustin700
Yes, I am using -w argument on the solution directory.

And I was able to reproduce the issue looking at it again. try-convert finishes successfully, but the output project is not a test project. An attempt to run tests from this output project result with the exception as shown by you in Output > Tests window.

After some debugging I found that this project is identified as ProjectStyle.WindowsDesktop instead of ProjectStyle.MSTest in GetProjectStyle() method in MSBuildWorkspace class.
As a result output project is missing PackageReference to Microsoft.NET.Test.Sdk.

[cartermp]

Yeah, this is a bug. The code currently assumes that it's a desktop app since it has these references https://github.com/tomaustin700/TestConvertIssue/blob/f5a7e30e2bc466171daef63cc0904282d7950058/PatientLedger.PatientLedgerTests/Ledger.PatientLedgerTests.csproj#L70-L75

The correct thing to do would be to adjust this routine to default to test if it has the test references regardless of if it has windows references.

try-convert/src/MSBuild.Abstractions/MSBuildWorkspace.cs

Lines 199 to 215 in 2641e96

	if (MSBuildFacts.PropsConvertibleToSDK.Contains(firstImportFileName, StringComparer.OrdinalIgnoreCase) &&
	MSBuildFacts.TargetsConvertibleToSDK.Contains(lastImportFileName, StringComparer.OrdinalIgnoreCase))
	{
	if (MSBuildHelpers.IsWPF(projectRootElement) || MSBuildHelpers.IsWinForms(projectRootElement) || MSBuildHelpers.IsDesktop(projectRootElement))
	{
	return ProjectStyle.WindowsDesktop;
	}
	else if (MSTestFacts.MSTestProps.Contains(firstImportFileName, StringComparer.OrdinalIgnoreCase) &&
	MSTestFacts.MSTestTargets.Contains(lastImportFileName, StringComparer.OrdinalIgnoreCase))
	{
	return ProjectStyle.MSTest;
	}
	else
	{
	return ProjectStyle.Default;
	}
	}

Probably a pretty simple fix. We look at references and make some assumptions since that's about the best that can be done in conversion tooling, so I suppose a targeted adjustment here is enough.

@tomaustin700 @dotnokato would either of you be interested in submitting a PR? More than happy to review. Also thanks to both of you for investigating this.

[dotnokato]

Hi @cartermp
Thanks for confirmation on what the result should be if there are test references in project file.

I am happy to work on a PR for that.

One additional question:
What should be the correct Project Sdk for such MSTest projects?
Current logic in https://github.com/dotnet/try-convert/blob/9ab68fac00e042daaede4e4486b2390755c4842e/src/MSBuild.Conversion.Project/ProjectRootElementExtensions.cs#L29-31 will set is as <Project Sdk="Microsoft.NET.Sdk.WindowsDesktop">
It seems to me that it should rather be <Project Sdk="Microsoft.NET.Sdk">

[cartermp]

@dotnokato In this case I think the logic you're pointing out here is correct (could be wrong...). The thinking when I wrote that code was that those references will not work correctly for .NET Core-based tests unless the WindowsDesktop SDK is also set. However, the code in MSBuildWorkspace doesn't correctly handle this, so it ends up causing a problem.

[dotnokato]

@cartermp OK, I've removed the change to ProjectRootElementExtensions.cs from the pull request. Please review and let me know if you want anything changed.

[cartermp]

Thanks for resolving this @dotnokato.