Refactor StatsTracker

[josecelano]

This is an experimental refactor of the StatsTracker.

It's still a WIP.

Tracker statistics are optional, and we had problems testing that behaviour.

I decided to try to make that code more testable. My idea is:

• Convert the StatsTracker into a StatsService that only creates the communication channel and the other services (EventSender, EventListener, ...)
• A EventSender to send the events.
• A EventListener that updates the statistics.
• A StatisticsRepository that contains the stats.

The high-level service (StatsService) only creates the StatisticsRepository used by the EventListener and the TorrusTracker. And the communication channel (with the sender and listener).

TODO:

• Extract EventSender
• Extract EventListener
• Extract StatisticsRepository
• Fix StatsEventSenderMock
• Add tests for EnventListener

Once we have the EventListener, we can add tests to check if it updates the stats correctly, which is something we did not test on PR-112.

While working on the refactor, I realized the StatsEventSenderMock does not work correctly. It only works when the event is sent, but it does not fail if no event is sent. The obvious solution would be to store the event sent in the Mock, but the method is not mutable.

    struct StatsEventSenderMock {
        expected_event: Option<TrackerStatisticsEvent>,
    }

    impl StatsEventSenderMock {
        fn new() -> Self {
            Self { expected_event: None }
        }

        fn should_throw_event(&mut self, expected_event: TrackerStatisticsEvent) {
            self.expected_event = Some(expected_event);
        }
    }

    #[async_trait]
    impl TrackerStatisticsEventSender for StatsEventSenderMock {
        async fn send_event(&self, event: TrackerStatisticsEvent) -> Option<Result<(), SendError<TrackerStatisticsEvent>>> {
            if self.expected_event.is_some() {
                assert_eq!(event, *self.expected_event.as_ref().unwrap());
            }
            None
        }
    }

    #[tokio::test]
    async fn it_should_send_the_upd4_connect_event_when_a_client_tries_to_connect_using_a_ip4_socket_address() {
        let tracker_stats_service = Box::new(TrackerStatsServiceMock::new());
        let mut stats_event_sender = Box::new(StatsEventSenderMock::new());

        let client_socket_address = sample_ipv4_socket_address();
        stats_event_sender.should_throw_event(TrackerStatisticsEvent::Udp4Connect);

        let torrent_tracker =
            Arc::new(TorrentTracker::new(default_tracker_config(), tracker_stats_service, Some(stats_event_sender)).unwrap());
        handle_connect(client_socket_address, &sample_connect_request(), torrent_tracker)
            .await
            .unwrap();
    }

I did not want to add a mocking library only for this case. That's why I did it manually. And I do not want to make that function mutable only for the test.

[josecelano]

hi @da2ce7 @WarmBeer, I've fixed the tests that were using my incomplete custom mock. In this commit.

I did not find a way to mock a trait function which is not mutable. This was my mock:

    struct StatsEventSenderMock {
        expected_event: Option<TrackerStatisticsEvent>,
    }

    impl StatsEventSenderMock {
        fn new() -> Self {
            Self { expected_event: None }
        }

        fn should_throw_event(&mut self, expected_event: TrackerStatisticsEvent) {
            self.expected_event = Some(expected_event);
        }
    }

    #[async_trait]
    impl TrackerStatisticsEventSender for StatsEventSenderMock {
        async fn send_event(&self, event: TrackerStatisticsEvent) -> Option<Result<(), SendError<TrackerStatisticsEvent>>> {
            if self.expected_event.is_some() {
                assert_eq!(event, *self.expected_event.as_ref().unwrap());
            }
            None
        }
    }

I needed to know if the method was called. If the method send_event is not called, the test does not fail.

The only way to do it (or the only way I found) is by adding a new field to the struct to store the state. But I could not do that without changing the function signature to:

async fn send_event(&mut self, event: TrackerStatisticsEvent) -> Option<Result<(), SendError<TrackerStatisticsEvent>>>;

I tried with a mocking library, and it works:

let mut stats_event_sender_mock = MockTrackerStatisticsEventSender::new();
            stats_event_sender_mock
                .expect_send_event()
                .with(eq(TrackerStatisticsEvent::Udp4Connect))
                .times(1)
                .returning(|_| Box::pin(future::ready(Some(Ok(())))));
            let stats_event_sender = Box::new(stats_event_sender_mock);

I think sooner or later, we will need it for more complex mocks.

I wonder how they can check that the method is called without changing its signature. I'm curious.

The other tricky thing was the returning value: .returning(|_| Box::pin(future::ready(Some(Ok(())))))

Since the function is an async function, you need to return a "future". But I do not know why they need the "pin".

We can apply this commit in the develop branch even if we do not merge this PR. Although I think all the changes I've made so far are useful.

[josecelano]

hi @da2ce7 @WarmBeer, I've finished the experimental refactor. I think it is worth merging it for two reasons:

1. I fixed some tests with the MockTrackerStatisticsEventSender mock.
2. I've increased test coverage for the statistics mod.

There are a couple of things I do not like:

• I think I can remove the trait TrackerStatisticsEventSender. Now I use mockall and I think I can mock the struct without adding the trait, which is only used in tests. I prefer to avoid adding traits if that's the only reason and we can mock the struct.
• I have isolated the asynchronous part that is hard to test. The infinite loop (event_listener) and launching the thread for the listener (run_event_listener). Those things are not tested, but I do not know how to do it without changing the production code (by introducing things that would be used only by tests).

[josecelano]

hi @da2ce7 @WarmBeer, I've finished the experimental refactor. I think it is worth merging it for two reasons:

1. I fixed some tests with the MockTrackerStatisticsEventSender mock.
2. I've increased test coverage for the statistics mod.

There are a couple of things I do not like:

• I think I can remove the trait TrackerStatisticsEventSender. Now I use mockall and I think I can mock the struct without adding the trait, which is only used in tests. I prefer to avoid adding traits if that's the only reason and we can mock the struct.
• I have isolated the asynchronous part that is hard to test. The infinite loop (event_listener) and launching the thread for the listener (run_event_listener). Those things are not tested, but I do not know how to do it without changing the production code (by introducing things that would be used only by tests).

I forgot to mention that this refactor makes it easier to decouple events from statistics in the future. Statistics would be only one of the consumers of those events. But it's not a reason to merge this PR since we have not decided how to handle events and their relationship with logging.

[da2ce7]

Looks Good to me. Nice work! :)

[da2ce7]

ACK 6f77dfe