Simplify agent communication in preparation for new wire protocol #762
Conversation
jnm2
force-pushed
the
simplify_agent_communication
branch
from
74e3fdf
to
3779fc2
Compare
| internal class RemoteTestAgentProxy : ITestAgent | ||
| public partial class TestAgency |
There was a problem hiding this comment.
Nice, it even followed it.
jnm2
force-pushed
the
simplify_agent_communication
branch
from
3779fc2
to
82ede1e
Compare
jnm2
force-pushed
the
simplify_agent_communication
branch
from
82ede1e
to
3b3b2e4
Compare
jnm2
force-pushed
the
simplify_agent_communication
branch
from
3b3b2e4
to
815db91
Compare
| /// <summary> | ||
| /// Gets a Guid that uniquely identifies this agent. | ||
| /// </summary> | ||
| Guid Id { get; } | ||
|
|
||
| /// <summary> | ||
| /// Starts the agent, performing any required initialization | ||
| /// </summary> | ||
| /// <returns>True if successful, otherwise false</returns> | ||
| bool Start(); |
There was a problem hiding this comment.
Do I understand correctly that all public nunit.engine and nunit.engine.core types are changeable in every release because consumers of the engine should only be referencing the nunit.engine.api assembly?
There was a problem hiding this comment.
Yes, that's my opinion. I want to enforce that in v4. 馃檪
There was a problem hiding this comment.
In V3, we could at least tell people this.
There was a problem hiding this comment.
A fair point. I went to take a look at what our current docs say. I think the first paragraph of this page covers that the API is the supported way to integrate with the engine?
https://github.com/nunit/docs/wiki/Test-Engine-API
I'd love to spin out the engine and console docs at some point, and build a little more structure around them. One day...
There was a problem hiding this comment.
@ChrisMaddock your last comment showed up twice, so I deleted one of the copies.
There was a problem hiding this comment.
I think that doc page says it well. I was only reacting to the "modern" view sometimes expressed that any public method being changed constitutes a breaking change.
|
Leave it with you as to whether you want to merge now, Joseph, or if you want other people to look over it first. 馃檪 |
| @@ -56,8 +57,9 @@ public class RemoteTestAgent : TestAgent, ITestEngineRunner | |||
| /// Construct a RemoteTestAgent | |||
| /// </summary> | |||
| public RemoteTestAgent(Guid agentId, string agencyUrl, IServiceLocator services) | |||
| : base(agentId, services) | |||
| : base(services) | |||
There was a problem hiding this comment.
If I understand you, you are saying that only Remote agents need an agent ID, as apposed to all agents. Of course, it makes no difference since we have not yet created any other agent types. Are you willing to give up the option of other agents?
There was a problem hiding this comment.
That's not quite it. All agents still need an ID and still use it to register with the agency. I moved the ID out of the ITestAgent interface so that ITestAgent members can represent remote procedure calls. I'm not planning for the protocol to have a "Start" message to the agent or a "Get ID" message. But even though its storage location moved, the ID is still being used in all the same ways as before this PR, I think.
There was a problem hiding this comment.
(And the reason to make ITestAgent look just like the wire protocol is to enable incremental changes to get there, rather than doing the whole thing in one PR and implementing our current ITestAgent interface on top of that.)
This cuts down on the amount of work needed to build the new protocol and it enhances the separation between ProcessRunner and all agency/agent concerns.
Review a commit at a time to follow the incremental changes.
Removes the ITestAgent.Start and ITestAgent.Id properties since they will not be remote procedure calls in the new protocol
Reinvents RemoteTestAgentProxy as IAgentLease so that TestAgency is in full control both of starting and stopping of agents and is the only thing with access to ITestAgent instances in the console process.
Looks like it nets six lines fewer 馃槉