Early testhost startup performance work

[cvpoienaru]

Description

Early testhost startup performance improvements

[azure-pipelines]

No pipelines are associated with this pull request.

[azure-pipelines]

No pipelines are associated with this pull request.

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[Haplois]

LGTM with some nit.

[drognanar]

is there any part of the code from CreateTestRunRequest or CreateDiscoveryRequest that's common?
it would be nice if the RunAll() scenario would essentially do PrepareForRunAll() -> Execute()

[cvpoienaru]

Yes, there's indeed a large portion in the beginning that is repeated for both CreateDiscoveryRequest and CreateTestRunRequest.

We can encapsulate this code in a method along the lines of PrepareForRunAll as you suggested, but both CreateDiscoveryRequest and CreateTestRunRequest receive different argument types that don't implement a common interface, i.e. DiscoveryCriteria vs TestRunCriteria. We can introduce a common interface for them to implement, but I'd leave this out of the scope of this PR for now and introduce it with the next PR that will enable testhost sharing.

[drognanar]

under what circumstances do we need to restart sessions?

[cvpoienaru]

Restarting sessions shouldn't be a thing.

The current PR enables spawning a session that has one or more testhosts attached to it and only one request per session can be served. This is because the default behavior is to terminate the testhost(s) once the request (discovery/run) was successfully completed.

The next round of changes should enable testhost sharing between discovery and run and should disable the default testhost termination behavior. This means the testhost(s) will be kept alive until the session is terminated with an explicit call from the API user (TW for example).

The only possible need for session restarting would be when the runsettings/list of sources change between discovery and execution. This will require restarting the testhosts, but at this point I think we're better off with terminating the current session and just starting a new session with the updated settings/list of sources. From the performance standpoint the cost of those operations should be about the same.

[drognanar]

I meant stopping a session and creating a new one.

besides the settings/list of sources would changing between debugging and profiling also require us to create a new session?

[cvpoienaru]

@drognanar Debugging doesn't use the old workflow anymore (i.e. vstest.console asks VS to launch the testhost process and attach to it) unless we're in compatibility mode. Instead the new workflow launches the testhost as a standalone process and only asks VS to attach to it just before starting the test execution, provided IsDebug flag is set of course. No debugging takes place for test discovery though.

For profiling though we still use the old workflow outlined above. As such we ask VS to launch the testhost process on our behalf because there's some specific setup that needs to take place first (setting env vars, etc.). Now this behavior is achievable by passing along an object implementing ITestHostLauncher to the StartTestSession call, the downside being profiling will be set up for test discovery too in addition to test execution. If we wish to avoid this behavior though, the only way to do that would be to start the test session with no ITestHostLauncher (in this way test discovery won't be profiled) and initiate test execution by re-launching the testhost process in profiling mode.

To answer the question in a concise manner: we should know if we intend to debug/profile when starting the test session. In case we didn't anticipate this we'll have to create a new session.

[drognanar]

it looks like the CreateStartTestSessionRequest isn't necessarily an immediate operation. does this mean that test sessions on our side can only be spawned sequentially? I noticed that there is just a global TestRequestManager instance.

[cvpoienaru]

That's correct, there's only one instance of the TestRequestManager and all operations are sequential, including test discovery and test runs. They're not immediate operations either, meaning we cannot go ahead and execute another request while the current one is running. This has been a design limitation for quite some time now and I think it's worth exploring changing this behavior in a PR of its own as I anticipate it'll require significant changes.

However, if we take only test session spawning into consideration this shouldn't be much of an issue as the plan is to spawn the test session(s) during build. Regarding multiple test sessions, is there any real use case for this on the TW side ?

[drognanar]

our extensibility let's users specify a runsettings file to use separately for each csproj. because of that it's possible that different projects will have a different runsettings

hopefully we'll be good with these starting in sequence and we'll need to be smart about stopping/starting sessions to minimize the cases where the session isn't active at the time we need to do a discovery

[Haplois]