Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only allow one runner at a time per connection to the agent by the protocol design #773

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 19 additions & 5 deletions src/NUnitEngine/nunit.engine.core/Agents/RemoteTestAgent.cs
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,13 @@ namespace NUnit.Engine.Agents
/// to it. Rather, it reports back to the sponsoring TestAgency upon
/// startup so that the agency may in turn provide it to clients for use.
/// </summary>
public class RemoteTestAgent : TestAgent, ITestEngineRunner
public class RemoteTestAgent : MarshalByRefObject, ITestAgent, IDisposable, ITestEngineRunner
{
private static readonly Logger log = InternalTrace.GetLogger(typeof(RemoteTestAgent));

private readonly Guid _agentId;
private readonly string _agencyUrl;
private readonly IServiceLocator _services;

private ITestEngineRunner _runner;
private TestPackage _package;
Expand All @@ -57,21 +58,34 @@ public class RemoteTestAgent : TestAgent, ITestEngineRunner
/// Construct a RemoteTestAgent
/// </summary>
public RemoteTestAgent(Guid agentId, string agencyUrl, IServiceLocator services)
: base(services)
{
_agentId = agentId;
_agencyUrl = agencyUrl;
_services = services;
}

/// <summary>
/// Overridden to cause object to live indefinitely
/// </summary>
public override object InitializeLifetimeService()
{
return null;
}

public void Dispose()
{
ShutDown();
}

public int ProcessId
{
get { return System.Diagnostics.Process.GetCurrentProcess().Id; }
}

public override ITestEngineRunner CreateRunner(TestPackage package)
public ITestEngineRunner CreateRunner(TestPackage package)
{
_package = package;
_runner = Services.GetService<ITestRunnerFactory>().MakeTestRunner(_package);
_runner = _services.GetService<ITestRunnerFactory>().MakeTestRunner(_package);
return this;
}

Expand Down Expand Up @@ -107,7 +121,7 @@ public bool Start()
return true;
}

public override void Stop()
public void ShutDown()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agents used Stop to indicate a distinction with ShutDown, which has a special meaning for Services. Services youse StartUp and ShutDown, whereas communications elements have used Start and Stop. Where a comm element is also a service, Start and Stop are typically called as part of StartUp and ShutDown. Having these as separate methods makes some unit-testing easier.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will say, I've never been aware of that design intent! 😅

I have no strong opinion on the final naming. Joseph - re: Charlie's comment about unit testing, do you think that will cause an issue, based on your final goal here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ChrisMaddock Well, to put it differently, the intent is seen in the definition of IService, which has StartUp and Shutdown methods. There is no rule against using the same method names outside of the interface, but it's potentially confusing... or maybe only confusing to me. 😄

{
log.Info("Stopping");

Expand Down
98 changes: 0 additions & 98 deletions src/NUnitEngine/nunit.engine.core/Agents/TestAgent.cs

This file was deleted.

2 changes: 1 addition & 1 deletion src/NUnitEngine/nunit.engine.core/ITestAgent.cs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ public interface ITestAgent
/// <summary>
/// Stops the agent, releasing any resources
/// </summary>
void Stop();
void ShutDown();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Others may not be confused, but I guess I find it confusing to see an interface that implements ShutDown even though it's not an IService and which implements it without also implementing StartUp.


/// <summary>
/// Creates a test runner
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public bool Start()
throw new NotImplementedException();
}

public void Stop()
public void ShutDown()
{
throw new NotImplementedException();
}
Expand Down
2 changes: 1 addition & 1 deletion src/NUnitEngine/nunit.engine/Services/TestAgency.cs
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ private void Release(Guid agentId, ITestAgent agent)
try
{
log.Debug("Stopping remote agent");
agent.Stop();
agent.ShutDown();
}
catch (SocketException ex)
{
Expand Down