From 40182b49e0c221ed85fb78f4243cc180cc96ed1e Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 16 Apr 2024 12:00:28 +0100 Subject: [PATCH] test: add tests for PeerList type --- packages/primitives/src/peer.rs | 39 +++ .../torrent-repository/src/entry/peer_list.rs | 249 ++++++++++++++++-- .../torrent-repository/src/entry/single.rs | 11 +- 3 files changed, 276 insertions(+), 23 deletions(-) diff --git a/packages/primitives/src/peer.rs b/packages/primitives/src/peer.rs index f5b009f2..ab755950 100644 --- a/packages/primitives/src/peer.rs +++ b/packages/primitives/src/peer.rs @@ -362,6 +362,38 @@ pub mod fixture { } impl PeerBuilder { + #[allow(dead_code)] + #[must_use] + pub fn seeder() -> Self { + let peer = Peer { + peer_id: Id(*b"-qB00000000000000001"), + peer_addr: SocketAddr::new(IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), 8080), + updated: DurationSinceUnixEpoch::new(1_669_397_478_934, 0), + uploaded: NumberOfBytes(0), + downloaded: NumberOfBytes(0), + left: NumberOfBytes(0), + event: AnnounceEvent::Completed, + }; + + Self { peer } + } + + #[allow(dead_code)] + #[must_use] + pub fn leecher() -> Self { + let peer = Peer { + peer_id: Id(*b"-qB00000000000000002"), + peer_addr: SocketAddr::new(IpAddr::V4(Ipv4Addr::new(127, 0, 0, 2)), 8080), + updated: DurationSinceUnixEpoch::new(1_669_397_478_934, 0), + uploaded: NumberOfBytes(0), + downloaded: NumberOfBytes(0), + left: NumberOfBytes(10), + event: AnnounceEvent::Started, + }; + + Self { peer } + } + #[allow(dead_code)] #[must_use] pub fn with_peer_id(mut self, peer_id: &Id) -> Self { @@ -390,6 +422,13 @@ pub mod fixture { self } + #[allow(dead_code)] + #[must_use] + pub fn last_updated_on(mut self, updated: DurationSinceUnixEpoch) -> Self { + self.peer.updated = updated; + self + } + #[allow(dead_code)] #[must_use] pub fn build(self) -> Peer { diff --git a/packages/torrent-repository/src/entry/peer_list.rs b/packages/torrent-repository/src/entry/peer_list.rs index 4af9b1d7..3f69edbb 100644 --- a/packages/torrent-repository/src/entry/peer_list.rs +++ b/packages/torrent-repository/src/entry/peer_list.rs @@ -1,7 +1,13 @@ +//! A peer list. use std::net::SocketAddr; use std::sync::Arc; -use torrust_tracker_primitives::peer; +use torrust_tracker_primitives::{peer, DurationSinceUnixEpoch}; + +// code-review: the current implementation uses the peer Id as the ``BTreeMap`` +// key. That would allow adding two identical peers except for the Id. +// For example, two peers with the same socket address but a different peer Id +// would be allowed. That would lead to duplicated peers in the tracker responses. #[derive(Clone, Debug, Default, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct PeerList { @@ -19,31 +25,26 @@ impl PeerList { self.peers.is_empty() } - pub fn insert(&mut self, key: peer::Id, value: Arc) -> Option> { - self.peers.insert(key, value) + pub fn upsert(&mut self, value: Arc) -> Option> { + self.peers.insert(value.peer_id, value) } pub fn remove(&mut self, key: &peer::Id) -> Option> { self.peers.remove(key) } - pub fn retain(&mut self, f: F) - where - F: FnMut(&peer::Id, &mut Arc) -> bool, - { - self.peers.retain(f); + pub fn remove_inactive_peers(&mut self, current_cutoff: DurationSinceUnixEpoch) { + self.peers + .retain(|_, peer| peer::ReadInfo::get_updated(peer) > current_cutoff); } #[must_use] - pub fn seeders_and_leechers(&self) -> (usize, usize) { - let seeders = self.peers.values().filter(|peer| peer.is_seeder()).count(); - let leechers = self.len() - seeders; - - (seeders, leechers) + pub fn get(&self, peer_id: &peer::Id) -> Option<&Arc> { + self.peers.get(peer_id) } #[must_use] - pub fn get_peers(&self, limit: Option) -> Vec> { + pub fn get_all(&self, limit: Option) -> Vec> { match limit { Some(limit) => self.peers.values().take(limit).cloned().collect(), None => self.peers.values().cloned().collect(), @@ -51,13 +52,21 @@ impl PeerList { } #[must_use] - pub fn get_peers_for_client(&self, client: &SocketAddr, limit: Option) -> Vec> { + pub fn seeders_and_leechers(&self) -> (usize, usize) { + let seeders = self.peers.values().filter(|peer| peer.is_seeder()).count(); + let leechers = self.len() - seeders; + + (seeders, leechers) + } + + #[must_use] + pub fn get_peers_excluding_addr(&self, peer_addr: &SocketAddr, limit: Option) -> Vec> { match limit { Some(limit) => self .peers .values() // Take peers which are not the client peer - .filter(|peer| peer::ReadInfo::get_address(peer.as_ref()) != *client) + .filter(|peer| peer::ReadInfo::get_address(peer.as_ref()) != *peer_addr) // Limit the number of peers on the result .take(limit) .cloned() @@ -66,9 +75,215 @@ impl PeerList { .peers .values() // Take peers which are not the client peer - .filter(|peer| peer::ReadInfo::get_address(peer.as_ref()) != *client) + .filter(|peer| peer::ReadInfo::get_address(peer.as_ref()) != *peer_addr) .cloned() .collect(), } } } + +#[cfg(test)] +mod tests { + + mod it_should { + use std::net::{IpAddr, Ipv4Addr, SocketAddr}; + use std::sync::Arc; + + use torrust_tracker_primitives::peer::fixture::PeerBuilder; + use torrust_tracker_primitives::peer::{self}; + use torrust_tracker_primitives::DurationSinceUnixEpoch; + + use crate::entry::peer_list::PeerList; + + #[test] + fn be_empty_when_no_peers_have_been_inserted() { + let peer_list = PeerList::default(); + + assert!(peer_list.is_empty()); + } + + #[test] + fn have_zero_length_when_no_peers_have_been_inserted() { + let peer_list = PeerList::default(); + + assert_eq!(peer_list.len(), 0); + } + + #[test] + fn allow_inserting_a_new_peer() { + let mut peer_list = PeerList::default(); + + let peer = PeerBuilder::default().build(); + + assert_eq!(peer_list.upsert(peer.into()), None); + } + + #[test] + fn allow_updating_a_preexisting_peer() { + let mut peer_list = PeerList::default(); + + let peer = PeerBuilder::default().build(); + + peer_list.upsert(peer.into()); + + assert_eq!(peer_list.upsert(peer.into()), Some(Arc::new(peer))); + } + + #[test] + fn allow_getting_all_peers() { + let mut peer_list = PeerList::default(); + + let peer = PeerBuilder::default().build(); + + peer_list.upsert(peer.into()); + + assert_eq!(peer_list.get_all(None), [Arc::new(peer)]); + } + + #[test] + fn allow_getting_one_peer_by_id() { + let mut peer_list = PeerList::default(); + + let peer = PeerBuilder::default().build(); + + peer_list.upsert(peer.into()); + + assert_eq!(peer_list.get(&peer.peer_id), Some(Arc::new(peer)).as_ref()); + } + + #[test] + fn increase_the_number_of_peers_after_inserting_a_new_one() { + let mut peer_list = PeerList::default(); + + let peer = PeerBuilder::default().build(); + + peer_list.upsert(peer.into()); + + assert_eq!(peer_list.len(), 1); + } + + #[test] + fn decrease_the_number_of_peers_after_removing_one() { + let mut peer_list = PeerList::default(); + + let peer = PeerBuilder::default().build(); + + peer_list.upsert(peer.into()); + + peer_list.remove(&peer.peer_id); + + assert!(peer_list.is_empty()); + } + + #[test] + fn allow_removing_an_existing_peer() { + let mut peer_list = PeerList::default(); + + let peer = PeerBuilder::default().build(); + + peer_list.upsert(peer.into()); + + peer_list.remove(&peer.peer_id); + + assert_eq!(peer_list.get(&peer.peer_id), None); + } + + #[test] + fn allow_getting_all_peers_excluding_peers_with_a_given_address() { + let mut peer_list = PeerList::default(); + + let peer1 = PeerBuilder::default() + .with_peer_id(&peer::Id(*b"-qB00000000000000001")) + .with_peer_addr(&SocketAddr::new(IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), 6969)) + .build(); + peer_list.upsert(peer1.into()); + + let peer2 = PeerBuilder::default() + .with_peer_id(&peer::Id(*b"-qB00000000000000002")) + .with_peer_addr(&SocketAddr::new(IpAddr::V4(Ipv4Addr::new(127, 0, 0, 2)), 6969)) + .build(); + peer_list.upsert(peer2.into()); + + assert_eq!(peer_list.get_peers_excluding_addr(&peer2.peer_addr, None), [Arc::new(peer1)]); + } + + #[test] + fn return_the_number_of_seeders_in_the_list() { + let mut peer_list = PeerList::default(); + + let seeder = PeerBuilder::seeder().build(); + let leecher = PeerBuilder::leecher().build(); + + peer_list.upsert(seeder.into()); + peer_list.upsert(leecher.into()); + + let (seeders, _leechers) = peer_list.seeders_and_leechers(); + + assert_eq!(seeders, 1); + } + + #[test] + fn return_the_number_of_leechers_in_the_list() { + let mut peer_list = PeerList::default(); + + let seeder = PeerBuilder::seeder().build(); + let leecher = PeerBuilder::leecher().build(); + + peer_list.upsert(seeder.into()); + peer_list.upsert(leecher.into()); + + let (_seeders, leechers) = peer_list.seeders_and_leechers(); + + assert_eq!(leechers, 1); + } + + #[test] + fn remove_inactive_peers() { + let mut peer_list = PeerList::default(); + let one_second = DurationSinceUnixEpoch::new(1, 0); + + // Insert the peer + let last_update_time = DurationSinceUnixEpoch::new(1_669_397_478_934, 0); + let peer = PeerBuilder::default().last_updated_on(last_update_time).build(); + peer_list.upsert(peer.into()); + + // Remove peers not updated since one second after inserting the peer + peer_list.remove_inactive_peers(last_update_time + one_second); + + assert_eq!(peer_list.len(), 0); + } + + #[test] + fn not_remove_active_peers() { + let mut peer_list = PeerList::default(); + let one_second = DurationSinceUnixEpoch::new(1, 0); + + // Insert the peer + let last_update_time = DurationSinceUnixEpoch::new(1_669_397_478_934, 0); + let peer = PeerBuilder::default().last_updated_on(last_update_time).build(); + peer_list.upsert(peer.into()); + + // Remove peers not updated since one second before inserting the peer. + peer_list.remove_inactive_peers(last_update_time - one_second); + + assert_eq!(peer_list.len(), 1); + } + + #[test] + fn allow_inserting_two_identical_peers_except_for_the_id() { + let mut peer_list = PeerList::default(); + + let peer1 = PeerBuilder::default() + .with_peer_id(&peer::Id(*b"-qB00000000000000001")) + .build(); + peer_list.upsert(peer1.into()); + + let peer2 = PeerBuilder::default() + .with_peer_id(&peer::Id(*b"-qB00000000000000002")) + .build(); + peer_list.upsert(peer2.into()); + + assert_eq!(peer_list.len(), 2); + } + } +} diff --git a/packages/torrent-repository/src/entry/single.rs b/packages/torrent-repository/src/entry/single.rs index 169ee2fb..a0112445 100644 --- a/packages/torrent-repository/src/entry/single.rs +++ b/packages/torrent-repository/src/entry/single.rs @@ -43,11 +43,11 @@ impl Entry for EntrySingle { } fn get_peers(&self, limit: Option) -> Vec> { - self.swarm.get_peers(limit) + self.swarm.get_all(limit) } fn get_peers_for_client(&self, client: &SocketAddr, limit: Option) -> Vec> { - self.swarm.get_peers_for_client(client, limit) + self.swarm.get_peers_excluding_addr(client, limit) } fn upsert_peer(&mut self, peer: &peer::Peer) -> bool { @@ -58,7 +58,7 @@ impl Entry for EntrySingle { drop(self.swarm.remove(&peer::ReadInfo::get_id(peer))); } AnnounceEvent::Completed => { - let previous = self.swarm.insert(peer::ReadInfo::get_id(peer), Arc::new(*peer)); + let previous = self.swarm.upsert(Arc::new(*peer)); // Don't count if peer was not previously known and not already completed. if previous.is_some_and(|p| p.event != AnnounceEvent::Completed) { self.downloaded += 1; @@ -66,7 +66,7 @@ impl Entry for EntrySingle { } } _ => { - drop(self.swarm.insert(peer::ReadInfo::get_id(peer), Arc::new(*peer))); + drop(self.swarm.upsert(Arc::new(*peer))); } } @@ -74,7 +74,6 @@ impl Entry for EntrySingle { } fn remove_inactive_peers(&mut self, current_cutoff: DurationSinceUnixEpoch) { - self.swarm - .retain(|_, peer| peer::ReadInfo::get_updated(peer) > current_cutoff); + self.swarm.remove_inactive_peers(current_cutoff); } }