Decouple integration tests from implementaion details

[josecelano]

Introduction

There are three integration tests in this repo:

• http_tracker
• udp_tracker
• tracker_api

They all use clients (API client, HTTP client and UDP client) that don't share code with production. And they have independent requests struts.

The UDP tracker uses an announce request from the aquatic crate:

#[tokio::test]
async fn should_return_an_announce_response() {
    let configuration = tracker_configuration();

    let udp_server = start_udp_tracker(&configuration);

    let client = new_udp_tracker_client_connected(&udp_server.bind_address.unwrap()).await;

    let connection_id = send_connection_request(TransactionId(123), &client).await;

    // Send announce request

    let announce_request = AnnounceRequest {
        connection_id: ConnectionId(connection_id.0),
        transaction_id: TransactionId(123i32),
        info_hash: InfoHash([0u8; 20]),
        peer_id: PeerId([255u8; 20]),
        bytes_downloaded: NumberOfBytes(0i64),
        bytes_uploaded: NumberOfBytes(0i64),
        bytes_left: NumberOfBytes(0i64),
        event: AnnounceEvent::Started,
        ip_address: Some(Ipv4Addr::new(0, 0, 0, 0)),
        key: PeerKey(0u32),
        peers_wanted: NumberOfPeers(1i32),
        port: Port(client.udp_client.socket.local_addr().unwrap().port()),
    };

    client.send(announce_request.into()).await;

    let response = client.receive().await;

    assert!(is_ipv4_announce_response(&response));
}

The HTTP tracker:

#[tokio::test]
async fn should_return_the_list_of_previously_announced_peers() {
    let http_tracker_server = start_public_http_tracker(Version::Axum).await;

    let info_hash = InfoHash::from_str("9c38422213e30bff212b30c360d26f9a02136422").unwrap();

    // Peer 1
    let previously_announced_peer = PeerBuilder::default()
        .with_peer_id(&peer::Id(*b"-qB00000000000000001"))
        .build();

    // Add the Peer 1
    http_tracker_server.add_torrent(&info_hash, &previously_announced_peer).await;

    // Announce the new Peer 2. This new peer is non included on the response peer list
    let response = Client::new(http_tracker_server.get_connection_info())
        .announce(
            &QueryBuilder::default()
                .with_info_hash(&info_hash)
                .with_peer_id(&peer::Id(*b"-qB00000000000000002"))
                .query(),
        )
        .await;

    // It should only contain the previously announced peer
    assert_announce_response(
        response,
        &Announce {
            complete: 2,
            incomplete: 0,
            interval: http_tracker_server.tracker.config.announce_interval,
            min_interval: http_tracker_server.tracker.config.min_announce_interval,
            peers: vec![DictionaryPeer::from(previously_announced_peer)],
        },
    )
    .await;
}

That was on purpose to keep tests decoupled from the implementation.

But I think there is a place where we break that isolation.

When we need to set the previous SUT state in the test, we usually call the tracker under the hood. All server implementations have a method to add a torrent:

API server:

pub struct Server {
    pub tracker: Arc<tracker::Tracker>,
    pub connection_info: ConnectionInfo,
}

impl Server {
    pub fn get_connection_info(&self) -> ConnectionInfo {
        self.connection_info.clone()
    }

    pub fn get_bind_address(&self) -> String {
        self.connection_info.bind_address.clone()
    }

    /// Add a torrent to the tracker
    pub async fn add_torrent(&self, info_hash: &InfoHash, peer: &Peer) {
        self.tracker.update_torrent_with_peer_and_get_stats(info_hash, peer).await;
    }
}

HTTP tracker server:

pub struct Server {
    pub tracker: Arc<tracker::Tracker>,
    pub connection_info: ConnectionInfo,
}

impl Server {
    pub fn get_connection_info(&self) -> ConnectionInfo {
        self.connection_info.clone()
    }

    pub async fn add_torrent(&self, info_hash: &InfoHash, peer: &Peer) {
        self.tracker.update_torrent_with_peer_and_get_stats(info_hash, peer).await;
    }
}

The UDP tracker is slightly different. Integration tests are very simple. The test for the announce request only checks that the response is a correct announce response:

#[tokio::test]
async fn should_return_an_announce_response() {
    let configuration = tracker_configuration();

    let udp_server = start_udp_tracker(&configuration);

    let client = new_udp_tracker_client_connected(&udp_server.bind_address.unwrap()).await;

    let connection_id = send_connection_request(TransactionId(123), &client).await;

    // Send announce request

    let announce_request = AnnounceRequest {
        connection_id: ConnectionId(connection_id.0),
        transaction_id: TransactionId(123i32),
        info_hash: InfoHash([0u8; 20]),
        peer_id: PeerId([255u8; 20]),
        bytes_downloaded: NumberOfBytes(0i64),
        bytes_uploaded: NumberOfBytes(0i64),
        bytes_left: NumberOfBytes(0i64),
        event: AnnounceEvent::Started,
        ip_address: Some(Ipv4Addr::new(0, 0, 0, 0)),
        key: PeerKey(0u32),
        peers_wanted: NumberOfPeers(1i32),
        port: Port(client.udp_client.socket.local_addr().unwrap().port()),
    };

    client.send(announce_request.into()).await;

    let response = client.receive().await;

    assert!(is_ipv4_announce_response(&response));
}

And we have units tests for most cases:

#[tokio::test]
async fn an_announced_peer_should_be_added_to_the_tracker() {
    let tracker = initialized_public_tracker();

    let client_ip = Ipv4Addr::new(126, 0, 0, 1);
    let client_port = 8080;
    let info_hash = AquaticInfoHash([0u8; 20]);
    let peer_id = AquaticPeerId([255u8; 20]);

    let remote_addr = SocketAddr::new(IpAddr::V4(client_ip), client_port);

    let request = AnnounceRequestBuilder::default()
        .with_connection_id(into_connection_id(&make(&remote_addr)))
        .with_info_hash(info_hash)
        .with_peer_id(peer_id)
        .with_ip_address(client_ip)
        .with_port(client_port)
        .into();

    handle_announce(remote_addr, &request, tracker.clone()).await.unwrap();

    let peers = tracker.get_all_torrent_peers(&info_hash.0.into()).await;

    let expected_peer = TorrentPeerBuilder::default()
        .with_peer_id(peer::Id(peer_id.0))
        .with_peer_addr(SocketAddr::new(IpAddr::V4(client_ip), client_port))
        .into();

    assert_eq!(peers[0], expected_peer);
}

As you can see in this case, we use the tracker directly, tracker.get_all_torrent_peers, to get the final state. I do not think this is a problem here because is a public tracker method. But my point is:

In E2E tests (or high-level integration tests, depending on what you want to call them), we should not access the tracker method directly.

If we are somehow trying to duplicate the production environment, we should not get data from the database directly or use internal data structures.

I do not consider these tests E2E tests because we setup the tracker at a low-level for testing. To write real E2E tests, we should run the tracker as an external process. I decided to do it so because it is much easier to set the initial state you want to test because you can do it programmatically whiteout having to deal with even variables to inject the tracker configuration you want to use.

NOTE: maybe we should write at least one real E2E test for each service (UDP and HTTP trackers and API). We are not testing how the application gets its configuration yet!!.

Proposal

I think the solution is different for each case.

For the API, I think we could add an endpoint to announce torrent peers manually. That could be useful for other purposes too. Then, we could use the API endpoint to pre-set the state we want to test without accessing implementation details.

For the HTTP, we should use the public HTTP specification to announce a peer instead of accessing the tracker.

#[tokio::test]
async fn should_return_the_list_of_previously_announced_peers() {
    let http_tracker_server = start_public_http_tracker(Version::Warp).await;

    let info_hash = InfoHash::from_str("9c38422213e30bff212b30c360d26f9a02136422").unwrap();

    // Peer 1
    let previously_announced_peer = PeerBuilder::default()
        .with_peer_id(&peer::Id(*b"-qB00000000000000001"))
        .build();

    // Add the Peer 1
    http_tracker_server.add_torrent(&info_hash, &previously_announced_peer).await;

    // Announce the new Peer 2. This new peer is non included on the response peer list
    let response = Client::new(http_tracker_server.get_connection_info())
        .announce(
            &QueryBuilder::default()
                .with_info_hash(&info_hash)
                .with_peer_id(&peer::Id(*b"-qB00000000000000002"))
                .query(),
        )
        .await;

    // It should only contain the previously announced peer
    assert_announce_response(
        response,
        &Announce {
            complete: 2,
            incomplete: 0,
            interval: http_tracker_server.tracker.config.announce_interval,
            min_interval: http_tracker_server.tracker.config.min_announce_interval,
            peers: vec![DictionaryPeer::from(previously_announced_peer)],
        },
    )
    .await;
}

The line http_tracker_server.add_torrent(&info_hash, &previously_announced_peer).await; should be replaced with another announce request using the client.

But there are other cases like this:

  #[tokio::test]
  async fn should_increase_the_number_of_tcp4_connections_handled_in_statistics() {
      let http_tracker_server = start_public_http_tracker(Version::Warp).await;

      Client::new(http_tracker_server.get_connection_info())
          .announce(&QueryBuilder::default().query())
          .await;

      let stats = http_tracker_server.tracker.get_stats().await;

      assert_eq!(stats.tcp4_connections_handled, 1);
  }

After announcing a new peer, we check the stats by accessing the tracker's public API but not the public HTTP tracker API. In this case, there is no way to get the stats from the public HTTP tracker API because it's not an HTTP tracker feature. This makes me think that maybe tests should not be there. In one of my latest PRs I have started extracting services. This way you only need to test once (in the service) if the announce request increases the stats.

We should start cleaning the tests. I think we should fix these problems and also remove duplicate tests. Different services (UDP and HTTP tracker) are, for example, testing that statistics are increased. Since we were doing a lot of refactors I've added a lot of tests to have a safety net for refactors, but now that we start having a good test coverage and extracting new services and adding layers to the app, we should write more units tests and fewer integrations tests. Or even convert some integration tests into unit tests.

Links

There is an interesting video about E2E testing talking about these topics:

• Don’t Do E2E Testing!

What do you think @da2ce7 @WarmBeer? WE do not have to do it right now, but we should take it into consideration from now on when we add or update new tests.

[josecelano]

This is another article:

J.B. Rainsberger - Integrated Tests Are A Scam

Where he talks about integration vs unit tests.

I see the point that you can't test the system (for example, the HTTP tracker) in 100% of corner cases, but I like the idea of thoroughly testing the public contract of the system (HTTP tracker). For example, we could check that the tracker is assigning the correct IP to the peer with an E2E test or with a unit test. I think we have to include the unit test anyway, and the E2E test would be duplicated, but I would like to run the E2E suite against any public tracker so that you can check whether the tracker is fulfilling the standard or not.

On the other hand, we could test the HTTP tracker as an independent piece, as David Farlye proposes n the "Don’t Do E2E Testing!" video. We could mock the tracker: HTTP Tracker -> Core Tracker.

I think the "integration" (as Rust calls them) tests we have right now are more like the first type: a black-box test suite to check if a tracker fulfils a BitTorrent specification (BEP). We do not have the first type of test yet. They would be necessary if we decided to extract the HTTP or UDP tracker into their own packages decoupled from the core tracker package.