Agent protocol design: allow concurrent runners for the same agent?
@jnm2 is already working on this.
Since May 4, 2020.
- Dominant language
- C#
- Stars
- 234
- Forks
- 163
- Avg merge
- 13h 32m
- Merged PRs (30d)
- 25
Description
/cc @nunit/engine-team
/cc @CharliePoole because I want to make sure we're not blocking a UI scenario
/cc @oznetmaster because you also had a use case that I'd like to be aware of
The current design of ITestAgent has ITestEngineRunner CreateRunner(TestPackage package). This means that you can create as many runner instances as you like for the same agent instance and use them all at the same time.
We do not make use of this functionality. ProcessRunner calls ITestAgent.CreateRunner immediately upon getting an agent. It proceeds to make all its calls through that single runner instance before calling ITestAgent.Stop().
Implementing this functionality in our new wire protocol seems like a lot of work for something we currently don't use. It would require either 1) a new TCP connection per runner or 2) multiplexing all the commands and test results between multiple runners in the same connection. With multiplexing, we'd have to manage a header on every runner message in our protocol specifying which runner session it's talking about. We'd need a system to be sure one runner didn't hog the whole TCP connection.
Allowing the same agent to run more than one runner does sound like it has potential to me, but mainly from the perspective of inverting the whole thing so that the agent is the listener/server and the console is the client. One runner per client connection is much more natural; clients can open up multiple connections if they want multiple runners. However, we weren't interested in inverting the server/client relationship between the console and the agent. See https://github.com/nunit/nunit-console/pull/576 for that implementation and discussion.
If we decide not to support concurrent runners in the same timeframe as moving off of remoting, I will do a simplification PR while we're still using PR before moving forward. It will make reviews significantly easier to follow if we make the remoting interfaces match the design of the replacement protocol before actually replacing it.
It would look something very roughly like the following. Before:
public interface ITestAgent
{
void Stop();
ITestEngineRunner CreateRunner(TestPackage package);
}
After:
public interface ITestAgent
{
void ShutDown(); // Modified, was: void Stop();
// Copied from from ITestEngineRunner:
TestEngineResult Load(TestPackage package); // Modified, was: TestEngineResult Load();
void Unload();
TestEngineResult Reload();
int CountTestCases(TestFilter filter);
TestEngineResult Run(ITestEventListener listener, TestFilter filter);
TestEngineResult Explore(TestFilter filter);
// *Eventually* would likely disappear or change form:
AsyncTestEngineResult RunAsync(ITestEventListener listener, TestFilter filter);
void StopRun(bool force);
}
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Assessment
This issue has not been assessed yet.