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 1 commit
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
6 changes: 3 additions & 3 deletions src/NUnitEngine/nunit.engine.core/Agents/RemoteTestAgent.cs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ 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 : MarshalByRefObject, ITestAgent, IDisposable, ITestEngineRunner
public class RemoteTestAgent : MarshalByRefObject, ITestAgent, IDisposable
{
private static readonly Logger log = InternalTrace.GetLogger(typeof(RemoteTestAgent));

Expand Down Expand Up @@ -82,11 +82,11 @@ public int ProcessId
get { return System.Diagnostics.Process.GetCurrentProcess().Id; }
}

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

public bool Start()
Expand Down
50 changes: 47 additions & 3 deletions src/NUnitEngine/nunit.engine.core/ITestAgent.cs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@
namespace NUnit.Engine
{
/// <summary>
/// The ITestAgent interface is implemented by remote test agents.
/// Defines the current communication protocol between the engine and the agent. This is an implementation detail
/// which is in the process of changing as the dependency on .NET Framework's remoting is removed.
/// </summary>
public interface ITestAgent
{
Expand All @@ -34,8 +35,51 @@ public interface ITestAgent
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
/// Loads a package which will be used for all subsequent calls to the other methods (except
/// <see cref="ShutDown"/>). This method must be called at least once before calling methods that operate on a
/// test package. It may be called any number of times after those methods.
/// </summary>
ITestEngineRunner CreateRunner(TestPackage package);
TestEngineResult Load(TestPackage package);
Copy link
Member

Choose a reason for hiding this comment

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

Reading this, I feel the need to read all the code so as to see if all the methods of ITestEngineResult are present. They are, so why not just inherit from that interface?


/// <summary>
/// Unloads any loaded package. If none is loaded, the call is ignored.
/// </summary>
void Unload();
Copy link
Member

Choose a reason for hiding this comment

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

I expect your working on a minimal-change-basis here, but before we move on from this project entirely, I think we should consider if ignoring an empty Unload call rather than throwing is the behaviour we want here. It feels inconsistent with Reload's behaviour below...

Copy link
Member

@CharliePoole CharliePoole May 26, 2020

Choose a reason for hiding this comment

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

@ChrisMaddock It could have been different, but it's public behavior. I don't think we should change what has been a safe call into one that throws.

UPDATE: Might not be public in this particular interface, but it is what we do in ITestRunner.Unload, which is public.


/// <summary>
/// Reloads the loaded package.
/// </summary>
/// <exception cref="InvalidOperationException">Thrown if no package is loaded.</exception>
TestEngineResult Reload();

/// <summary>
/// Counts the test cases in the loaded package that would be run under the specified filter.
/// </summary>
int CountTestCases(TestFilter filter);

/// <summary>
/// Runs the tests in the loaded package synchronously. The listener interface is notified as the run
/// progresses.
/// </summary>
TestEngineResult Run(ITestEventListener listener, TestFilter filter);

#if !NETSTANDARD1_6
/// <summary>
/// Start an asynchronous run of the tests in the loaded package. The listener interface is notified as the run
/// progresses.
/// </summary>
AsyncTestEngineResult RunAsync(ITestEventListener listener, TestFilter filter);
#endif

/// <summary>
/// Cancel the current test run. If no test is running, the call is ignored.
/// </summary>
/// <param name="force">Indicates whether tests that have not completed should be killed.</param>
void StopRun(bool force);

/// <summary>
/// Returns information about the test cases in the loaded package that would be run under the specified filter.
/// </summary>
TestEngineResult Explore(TestFilter filter);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,32 @@ public partial class AgentStoreTests
{
private sealed class DummyTestAgent : ITestAgent
{
public ITestEngineRunner CreateRunner(TestPackage package)
public int CountTestCases(TestFilter filter)
{
throw new NotImplementedException();
}

public bool Start()
public TestEngineResult Explore(TestFilter filter)
{
throw new NotImplementedException();
}

public TestEngineResult Load(TestPackage package)
{
throw new NotImplementedException();
}

public TestEngineResult Reload()
{
throw new NotImplementedException();
}

public TestEngineResult Run(ITestEventListener listener, TestFilter filter)
{
throw new NotImplementedException();
}

public AsyncTestEngineResult RunAsync(ITestEventListener listener, TestFilter filter)
{
throw new NotImplementedException();
}
Expand All @@ -44,6 +64,16 @@ public void ShutDown()
{
throw new NotImplementedException();
}

public void StopRun(bool force)
{
throw new NotImplementedException();
}

public void Unload()
{
throw new NotImplementedException();
}
}
}
}
Expand Down
52 changes: 48 additions & 4 deletions src/NUnitEngine/nunit.engine/Services/TestAgency.AgentLease.cs
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,17 @@

#if !NETSTANDARD1_6 && !NETSTANDARD2_0
using System;
using System.Threading;

namespace NUnit.Engine.Services
{
public partial class TestAgency
{
private sealed class AgentLease : IAgentLease
private sealed class AgentLease : IAgentLease, ITestEngineRunner
{
private readonly TestAgency _agency;
private readonly ITestAgent _remoteAgent;
private TestEngineResult _createRunnerLoadResult;

public AgentLease(TestAgency agency, Guid id, ITestAgent remoteAgent)
{
Expand All @@ -42,14 +44,56 @@ public AgentLease(TestAgency agency, Guid id, ITestAgent remoteAgent)

public Guid Id { get; }

public void Dispose()
{
_agency.Release(Id, _remoteAgent);
}

public ITestEngineRunner CreateRunner(TestPackage package)
{
return _remoteAgent.CreateRunner(package);
_createRunnerLoadResult = _remoteAgent.Load(package);
return this;
}

public void Dispose()
public int CountTestCases(TestFilter filter)
{
_agency.Release(Id, _remoteAgent);
return _remoteAgent.CountTestCases(filter);
}

public TestEngineResult Explore(TestFilter filter)
{
return _remoteAgent.Explore(filter);
}

public TestEngineResult Load()
{
return Interlocked.Exchange(ref _createRunnerLoadResult, null)
?? _remoteAgent.Reload();
}

public void Unload()
{
_remoteAgent.Unload();
}

public TestEngineResult Reload()
{
return _remoteAgent.Reload();
}

public TestEngineResult Run(ITestEventListener listener, TestFilter filter)
{
return _remoteAgent.Run(listener, filter);
}

public AsyncTestEngineResult RunAsync(ITestEventListener listener, TestFilter filter)
{
return _remoteAgent.RunAsync(listener, filter);
}

public void StopRun(bool force)
{
_remoteAgent.StopRun(force);
}
}
}
Expand Down