Consider defining public interfaces in order to ease the unit testing on client side

[adrianiftode]

Given the following example, I need to test the CancelOrder method.

public void CancelOrder(string referenceId)
{ 
  var order = _ordersRepository.GetByReference(referenceId);
  if (order == null)
  {
     _logger.LogWarning("Order with {referenceId} does not exist", referenceId)
     return;
  }
  
  using (var audit = AuditScope.Create("Order:Update", () => order))
  {
    audit.SetCustomField("ReferenceId", referenceId);
    order.Status = -1;
    order = _ordersRepository.OrderUpdate(order);
    audit.Comment("Status Updated to Cancelled");
  }
}

I can think of several different unit tests here:

• Cancelling_Existing_Order_Sets_Status_To_Minus_One()
• Cancelling_Existing_Order_Creates_An_AuditEvent_Having_OrderRefence_A_Comment_The_Order_ItSelf_And_A_Specific_EventType()

I could probably write Cancelling_Existing_Order_Sets_Status_To_Minus_One as

[Fact]
public void Cancelling_Existing_Order_Sets_Status_To_Minus_One()
{
    var orderReference = "IL-0001";
    var ordersRepositoryMock = new Mock<OrdersRepository>();
    var ordersRepositoryMock.Setup(...).Returns(new Order());
    var sut  = new OrdersService(ordersRepositoryMock.Object, ...);

     sut.CancelOrder(orderReference);

     ordersRepositoryMock.Verify(repo => repo.OrderUpdate(It.Is<Order>(o => o.Status == -1));
}

One of the issues with this test is it calls AuditScope.Create, which is a shared dependency due to its static nature, and because of this then this test is not a unit test anymore, but an integration test, even the intention is to be a unit test. Maybe this doesn't affect too much, as usually the interaction between the tests and Audit.NET does not carry state, but I still have to be aware I need to change the default data provider to the NullDataProvider to avoid having lots of files in the tests execution folder and also reduce the execution time of the overall test suite.

For the following test, I don't have any way to verify the interaction with Audit.NET as usually mocking static classes is not very user friendly. Also, there is no way to interact with the created AuditScope, unless we expose it in a certain way. For this, I could define some interfaces, their default implementations will be simply adapters to Audit.NET classes as they currently are, so I can use them in the production code, but in the testing code I can do something like:

[Fact]
public void Cancelling_Existing_Order_Creates_An_AuditEvent_Having_OrderRefence_A_Comment_The_Order_ItSelf_And_A_Specific_EventType()
{
    var orderReference = "IL-0001";

    var auditScopeFactoryMock = new Mock<IAuditScopeFactory>();
    var sut  = new OrdersService(..., auditScopeFactoryMock.Object);

     sut.CancelOrder(orderReference);

     // verify event type is "Order:Update"
     auditScopeFactoryMock.Verify(c => c.Create("Order:Update", It.Is<Func<object>>(func => func.Invoke().OrderRefence == "IL-0001")));

    // verify how AuditScope is used
     auditScope.Verify(c => c.SetCustomField("ReferenceId", orderReference));
}

So what can be done is Audit.NET to define these two interfaces: IAuditScopeFactory and IAuditScope. The default implementation (adapters) also should be provided and let the Audit.NET consumers to chose if they want to use the static factories and the AuditScope itself or the interfaces and their default implementations, based on their needs. For .NET Core Audit.NET could even have an extension method over the IServiceCollection method, something like AddAuditNET, that registers the default implementation. I think the implementation could be public to give the possibility to let other IoCs register them.

There could be also some other questions like should these interfaces/default implementations be integrated with the Audit.NET's internal implementation? Maybe not for start and maybe is not the scope of it. The main scope here is to enable testability in business classes like OrdersService.

This issue diverged from #315

[thepirat000]

Maybe I'm missing something, but I think exposing the IAuditScopeFactory should be enough, or could you provide some sample test code where you use IAuditScope?

[adrianiftode]

From a business perspective, there can be a very strong requirement that we audit a certain property, like Order Reference in this case. Order Reference is important to the business, as it is an identifier recognized in different contexts (aka, different systems, different micro-services, so on). A test has to document and assert this business requirement.

I understand the reticence of having the IAuditScope interface when AuditScope is more like a dto object with a state that can be easily inspected (via Event property).

So I foresee these two ways:

Having the interface: IAuditScope. It can be mocked and IAuditScopeFactory mock could be set up to return the IAuditScope mock

public interface IAuditScopeFactory
{
	IAuditScope Create(string eventType, Func<object> target);
}

public interface IAuditScope : IDisposable, IAsyncDisposable
{
	void SetCustomField<TC>(string fieldName, TC value);
}

public class OrderService
{
	private readonly IAuditScopeFactory _auditScopeFactory;

	public OrderService(IAuditScopeFactory auditScopeFactory) => _auditScopeFactory = auditScopeFactory;

	public void CancelOrder(string referenceId)
	{
		var order = new { };
		using var audit = _auditScopeFactory.Create("Order:Update", () => order);
		audit.SetCustomField("ReferenceId", referenceId);
	}
}

public class OrderServiceTests
{
	[Fact]
	public void OrderReference_Is_Audited()
	{
		// Arrange
		var auditScopeFactoryMock = new Mock<IAuditScopeFactory>();
		var auditScopeMock = new Mock<IAuditScope>();
		auditScopeFactoryMock.Setup(c => c.Create(It.IsAny<string>(), It.IsAny<Func<object>>()))
			.Returns(auditScopeMock.Object);
		var sut = new OrderService(auditScopeFactoryMock.Object);

		// Act
		sut.CancelOrder("IL-0001");

		// Assert
		auditScopeMock.Verify(c => c.SetCustomField("ReferenceId", "IL-0001"));
	}
}

The problem with the IAuditScope interface is it can get quite big. Maybe the interface can be limited to a subset of the existing API, by excluding any property, and start with SetCustomField, Comment, Discard, Save, etc.

Having no IAuditScope interface. This leaves us on checking the audit scope itself and in order to successfully do so, we will need to create and access it. The creation can be passed on the default implementation of the IAuditScopeFactory. One of the problems to be solved here remains the default data provider. So with these, the code might look like

public interface IAuditScopeFactory
{
	AuditScope Create(string eventType, Func<object> target);
}

public class AuditScopeFactory : IAuditScopeFactory
{
	public AuditScope Create(string eventType, Func<object> target)
		=> AuditScope.Create(eventType, target);
}

public class OrderService
{
	private readonly IAuditScopeFactory _auditScopeFactory;

	public OrderService(IAuditScopeFactory auditScopeFactory) => _auditScopeFactory = auditScopeFactory;

	public void CancelOrder(string referenceId)
	{
		var order = new { };
		using var auditScope = _auditScopeFactory.Create("Order:Update", () => order);
		auditScope.SetCustomField("ReferenceId", referenceId);
	}
}

public class OrderServiceTests
{
	[Fact]
	public void OrderReference_Is_Audited()
	{
		// Arrange
		var auditScopeFactoryMock = new Mock<IAuditScopeFactory>();
		AuditScope auditScope = null;
		auditScopeFactoryMock
			.Setup(c => c.Create(It.IsAny<string>(), It.IsAny<Func<object>>()))
			.Returns((string eventType, Func<object> target) =>
			{                                
				auditScope = new AuditScopeFactory().Create(eventType, target);
				return auditScope;
			});
		var sut = new OrderService(auditScopeFactoryMock.Object);

		// Act
		sut.CancelOrder("IL-0001");

		// Assert
		Assert.True(auditScope.Event.CustomFields.ContainsKey("ReferenceId"));
		Assert.True(auditScope.Event.CustomFields.ContainsValue("IL-0001"));
	}
}

The problem with this approach is it forwards the call to the default factory, which in turn uses the default data provider, which again is the FileSystem one. This can be fixed with the NullProvider as being the default one, but it's a breaking change. The second problem is we can't Verify calls to SetCustomField, Comment, Discard, Save, etc, in a mocking way, but this could be overcome with checking their side effects, which are on the actual state of the audit scope object. Still doesn't feel that right as the developer needs to know a bit more about the internals of the AuditScope object.

So this is the reason why I would personally go with the IAuditScope interface.

[thepirat000]

please check the pull request #322 with the proposed implementation of the factory/interfaces.

[thepirat000]

Included on version 16.0.0