Use InMemoryEventBus as scoped and use InMemoryEventBus in integration tests

[anddrzejb]

Changes:

• InMemoryEventBus changed to scoped
• Rename incorrect class name (file name not matching class name)
• Fix integration tests to use the InMemoryEventBus from DI (still rely on FakeInMemoryBus for asserting if the event was sent)

[meaboutsoftware]

It introduces quite a lot of boilerplate code when adding it in TestEngine. Still, we agree that it would make sense to use MediatR but maybe with a slightly different approach. What do you think to use MediatR but fake the method responsible for publishing?

[anddrzejb]

The problem here is that there is a test Given_valid_mark_pass_as_expired_request_Then_should_return_no_content where:

1. The pass is registered in the notification handler (ContractSignedEvent is published and needs to be handled by ContractSignedEventHandler, where the pass is persisted) -> here you had this intricate IntegrationEventHandlerScope flow in previous approach
2. The expiration endpoint is called which is served IMediator based InMemoryEventBus. The PassExpiredEvent is using the bus.
3. PassExpiredEventHandler is marking the pass as expired in the database. Also publishes OfferPreparedEvent.

If I am going to fake PublishAsync instead of using built-in MediatR, I am afraid that:
Ad.1. The ContractSignedEventHandler won't be called
Ad.3. The PassExpiredEventHandler won't be called
I believe there are more tests like that.

On top of that, if Mediator won't run its internals, then the bug #96 that sparked this change will hide again, as it surfaced because Mediator actually executed the Publish method.

[meaboutsoftware]

@anddrzejb to be honest I wouldn't be afraid that it won't be called - this is an internal mechanism of MediatR that should not be tested. IMO it would be enough to know that the event was published and that's it.

@kamilbaczek can you add your opinion on it as well? I do not want to be "that one badass who destroys dreams" :D

If you disagree, there is still a chance to simplify the above code:

internal static WebApplicationFactory<T> WithFakeEventBus<T>(this WebApplicationFactory<T> webApplicationFactory,
        IEventBus eventBusMock)
        where T : class => webApplicationFactory.WithWebHostBuilder(builder =>
    {
        builder.ConfigureTestServices(services =>
        {
            ReplaceNotificationHandlers(eventBusMock, services);
        });
    });

private static void ReplaceNotificationHandlers(IEventBus eventBusMock, IServiceCollection services)
    {
        var notificationHandlerTypes = GetNotificationHandlerTypes(services);

        foreach (var handlerType in notificationHandlerTypes)
        {
            var decoratorType = typeof(NotificationDecorator<>).MakeGenericType(handlerType.GetGenericArguments()[0]);

            services.AddTransient(handlerType, serviceProvider =>
            {
                var innerHandler = serviceProvider.GetService(handlerType);
                var decorator = Activator.CreateInstance(decoratorType, eventBusMock, innerHandler);

                return decorator!;
            });
        }
    }

    private static List<Type> GetNotificationHandlerTypes(IServiceCollection services) => services
            .Where(descriptor => descriptor.ServiceType.IsGenericType &&
                                 descriptor.ServiceType.GetGenericTypeDefinition() == typeof(INotificationHandler<>))
            .Select(descriptor => descriptor.ServiceType)
            .ToList();

[meaboutsoftware]

One more comment from my side - in such case it would be acceptable to introduce the E2E test that would check the business process. Unfortunately (on purpose) we do not have it in this project but unfortunately we need to decide for some trade-offs because of that

[anddrzejb]

Let me first address the example test I was mentioning: Given_valid_mark_pass_as_expired_request_Then_should_return_no_content.

1. This test is expecting a PassExpiredEvent to be published.
2. Based on the code, it should be published once api/passes/{id} endpoint is called - given the pass exists. Because this is an integration test, then I presume it should wire the application in such a way, so the event is indeed published in the endpoint handler.
3. For the endpoint to find the pass, it needs to exist in database. The arrange section of the test is responsible for that.
   a. Publishes ContractSignedEvent
   b. The handler is resolved based on the event and called.
   c. The handler creates the pass in the database and publishes PassRegisteredEvent. We do not want to reallyy publish it, this is not part of the test, after all we are still in the arrange section.
4. The pass id is retrieved from the database so it can be used later in the test (still in the arrange section of the test)
5. The endpoint api/passes/{id}, where {id} is the value retrieved in point 4 (act)
6. The mock is checked if the event PassExpiredEvent was indeed called (assert).

Here we have 2 scenarios - I: use mediator or II: fake it.
I. Mediator. The tricky part is to catch the 2nd event published in 3c. Otherwise, decorator does its thing. This is where the fragility of my solution comes out - what if we have a scenario when we need to stop 3rd event, or 4th? Currently, such scenario is not handled, unless manually issued in the arrange/act section. But also seems that at this point is not needed. But it is not handled by II either.
II. Fake the mediator. Then we have to deal with the 3b. We can easily do it using fake, but it adds complexity to the solution by introducing IntegrationEventHandlerScope, which under the hood seems to be doing what mediator does. Without its shortcomings. Which is not necessarily good given that the test did not discover the bug.

Here is where we come to the bottom of the issue. Integration tests should be testing all the components that are involved in the testing scenario. When they are not tested, then bugs may creep in (I rest my case?). The argument that Mediator is an internal mechanism and should not be tested - if it was a unit test I wholeheartedly agree. But in this case it is not Mediator being tested but its correct/incorrect integration.

Regarding the proposed change - it goes into infinite loop. When mediator is trying to resolve INotification

• it goes into the delegate passed in services.AddTransient. Delegate Inside is trying to get from the services the handlerType, that is the same type as the mediator was trying to get. So...
  • it goes into the delegate passed in services.AddTransient. Delegate Inside is trying to get from the services the handlerType, that is the same type as the mediator was trying to get. So...
    • it goes into the delegate passed in services.AddTransient. Delegate Inside is trying to get from the services the handlerType, that is the same type as the mediator was trying to get. So...
      (....) surprisingly I never run into stack overflow.

"that one badass who destroys dreams"

No worries, I am not that fragile 😄

[meaboutsoftware]

Let's discuss this together on the next sync