Test projects do not convert correctly and result in TestContainer was not found error about try-convert HOT 10 CLOSED

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?

from try-convert.

@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.

from try-convert.

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

from try-convert.

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

Thanks

from try-convert.

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.

from try-convert.

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.

from try-convert.

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.

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.

from try-convert.

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">

from try-convert.

@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.

from try-convert.

Thanks for resolving this @dotnokato.

from try-convert.