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 all 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
38 changes: 20 additions & 18 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
{
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,22 +58,35 @@ 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 TestEngineResult Load(TestPackage package)
{
_package = package;
_runner = Services.GetService<ITestRunnerFactory>().MakeTestRunner(_package);
return this;
_runner = _services.GetService<ITestRunnerFactory>().MakeTestRunner(_package);
return _runner.Load();
}

public bool Start()
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 Expand Up @@ -187,18 +201,6 @@ public TestEngineResult Run(ITestEventListener listener, TestFilter filter)
return _runner.Run(listener, filter);
}

/// <summary>
/// Start a run of the tests in the loaded TestPackage. The tests are run
/// asynchronously and the listener interface is notified as it progresses.
/// </summary>
/// <param name="listener">An ITestEventHandler to receive events</param>
/// <param name="filter">A TestFilter used to select tests</param>
/// <returns>A <see cref="AsyncTestEngineResult"/> that will provide the result of the test execution</returns>
public AsyncTestEngineResult RunAsync(ITestEventListener listener, TestFilter filter)
{
return _runner.RunAsync(listener, filter);
}

/// <summary>
/// Cancel the ongoing test run. If no test is running, the call is ignored.
/// </summary>
Expand Down
98 changes: 0 additions & 98 deletions src/NUnitEngine/nunit.engine.core/Agents/TestAgent.cs

This file was deleted.

9 changes: 9 additions & 0 deletions src/NUnitEngine/nunit.engine.core/AsyncTestEngineResult.cs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,15 @@ public class AsyncTestEngineResult : ITestRun
private volatile TestEngineResult _result;
private readonly ManualResetEvent _waitHandle = new ManualResetEvent(false);

#if !NETSTANDARD1_6
public static AsyncTestEngineResult RunAsync(Func<TestEngineResult> func)
{
var testRun = new AsyncTestEngineResult();
ThreadPool.QueueUserWorkItem(_ => testRun.SetResult(func.Invoke()));
return testRun;
}
#endif

/// <summary>
/// Get the result of this run.
/// </summary>
Expand Down
43 changes: 39 additions & 4 deletions src/NUnitEngine/nunit.engine.core/ITestAgent.cs
Original file line number Diff line number Diff line change
Expand Up @@ -24,18 +24,53 @@
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
{
/// <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
/// 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. The listener interface is notified as the run progresses.
/// </summary>
TestEngineResult Run(ITestEventListener listener, TestFilter filter);

/// <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);
}
}
11 changes: 11 additions & 0 deletions src/NUnitEngine/nunit.engine.core/Polyfills.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
#if NET20

namespace System
{
// This would cause conflicts if it was public in the API assembly. This is an engine implementation detail since
// even though it's public because this assembly should not be compiled against by anyone that references the
// engine.
public delegate TResult Func<TResult>();
}

#endif
16 changes: 1 addition & 15 deletions src/NUnitEngine/nunit.engine.core/Runners/AbstractTestRunner.cs
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,6 @@
// ***********************************************************************

using System;
using System.ComponentModel;
using NUnit.Engine.Services;

namespace NUnit.Engine.Runners
{
Expand Down Expand Up @@ -110,19 +108,7 @@ public virtual void UnloadPackage()
/// <returns>An <see cref="AsyncTestEngineResult"/> that will provide the result of the test execution</returns>
protected virtual AsyncTestEngineResult RunTestsAsync(ITestEventListener listener, TestFilter filter)
{
var testRun = new AsyncTestEngineResult();

using (var worker = new BackgroundWorker())
{
worker.DoWork += (s, ea) =>
{
var result = RunTests(listener, filter);
testRun.SetResult(result);
};
worker.RunWorkerAsync();
}

return testRun;
return AsyncTestEngineResult.RunAsync(() => RunTests(listener, filter));
}
#endif

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,17 +30,42 @@ 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 void Stop()
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 void ShutDown()
{
throw new NotImplementedException();
}

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

public void Unload()
{
throw new NotImplementedException();
}
Expand Down
Loading