Merge branch 'more-linkspec-tests' into 'main'

Add tests for a bunch of code in tor-linkspec

See merge request tpo/core/arti!867

Cargo.lock

@@ -3896,6 +3896,7 @@ dependencies = [
  "educe",
  "hex",
  "hex-literal",
+ "itertools",
  "safelog",
  "serde",
  "serde_test",

crates/tor-guardmgr/src/bridge/config.rs

@@ -363,7 +363,7 @@ impl FromStr for BridgeConfigBuilder {
             #[cfg(feature = "pt-client")]
             ChannelMethod::Pluggable(target) => {
                 let (transport, addr, settings) = target.into_parts();
-                (transport.into_inner(), vec![addr], settings.into_inner())
+                (transport.to_string(), vec![addr], settings.into_inner())
             }
             other => {
                 return Err(BridgeParseError::UnsupportedChannelMethod {

crates/tor-linkspec/Cargo.toml

@@ -38,6 +38,7 @@ tor-protover = { path = "../tor-protover", version = "0.3.0" }
 
 [dev-dependencies]
 hex-literal = "0.3"
+itertools = "0.10.1"
 serde_test = "1.0.124"
 
 [package.metadata.docs.rs]

crates/tor-linkspec/src/ids/by_id.rs

@@ -116,7 +116,7 @@ impl<H: HasRelayIds> ByRelayIds<H> {
         }
     }
 
-    /// Remove the single value in this set (if any) that has all the same relay ids that
+    /// Remove the single value in this set (if any) that has all the same
     /// relay IDs that `key` does.
     pub fn remove_by_all_ids<T>(&mut self, key: &T) -> Option<H>
     where
@@ -133,6 +133,7 @@ impl<H: HasRelayIds> ByRelayIds<H> {
             None
         }
     }
+
     /// Return a reference to every element in this set that shares _any_ ID
     /// with `key`.
     ///
@@ -201,6 +202,7 @@ mod test {
         // Try exact lookup
         assert_eq!(set.by_all_ids(&keys1), Some(&keys1));
         assert_eq!(set.by_all_ids(&keys2), Some(&keys2));
+        assert_eq!(set.by_all_ids(&RelayIds::empty()), None);
         {
             let search = RelayIdsBuilder::default()
                 .rsa_identity(rsa1)
@@ -268,7 +270,7 @@ mod test {
 
         let mut set = ByRelayIds::new();
         set.insert(keys1.clone());
-        set.insert(keys2);
+        set.insert(keys2.clone());
         assert_eq!(set.len(), 2);
 
         let removed = set.remove_exact(&keys1);
@@ -277,9 +279,27 @@ mod test {
 
         {
             let search = RelayIdsBuilder::default().ed_identity(ed2).build().unwrap();
+            // We're calling remove_exact, but we did not list _all_ the keys in keys2.
             let removed = set.remove_exact(&search);
             assert_eq!(removed, None);
             assert_eq!(set.len(), 1);
+
+            // If we were to use `remove_by_all_ids` with a search that didn't
+            // match, it wouldn't work.
+            let no_match = RelayIdsBuilder::default()
+                .ed_identity(ed2)
+                .rsa_identity(rsa1)
+                .build()
+                .unwrap();
+            let removed = set.remove_by_all_ids(&no_match);
+            assert_eq!(removed, None);
+            assert_eq!(set.len(), 1);
+
+            // If we use `remove_by_all_ids` with the original search, though,
+            // it will remove the element.
+            let removed = set.remove_by_all_ids(&search);
+            assert_eq!(removed, Some(keys2));
+            assert_eq!(set.len(), 0);
         }
     }
 }

crates/tor-linkspec/src/traits.rs

@@ -479,4 +479,11 @@ mod test {
             );
         }
     }
+
+    #[test]
+    fn has_id() {
+        use crate::RelayIds;
+        assert!(example().has_any_identity());
+        assert!(!RelayIds::empty().has_any_identity());
+    }
 }

crates/tor-linkspec/src/transport.rs

@@ -83,19 +83,6 @@ impl TryFrom<String> for PtTransportName {
     }
 }
 
-impl AsRef<str> for PtTransportName {
-    fn as_ref(&self) -> &str {
-        &self.0
-    }
-}
-
-impl PtTransportName {
-    /// Return the name as a `String`
-    pub fn into_inner(self) -> String {
-        self.0
-    }
-}
-
 impl Display for PtTransportName {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         Display::fmt(&self.0, f)
@@ -692,4 +679,134 @@ mod test {
             assert!(matches!(e, BridgeAddrError::BadAddress(_)));
         }
     }
+
+    #[test]
+    fn transport_id() {
+        let id1: TransportId = "<none>".parse().unwrap();
+        assert!(id1.is_builtin());
+        assert_eq!(id1.to_string(), "-".to_string());
+
+        #[cfg(feature = "pt-client")]
+        {
+            let id2: TransportId = "obfs4".parse().unwrap();
+            assert_ne!(id2, id1);
+            assert!(!id2.is_builtin());
+            assert_eq!(id2.to_string(), "obfs4");
+
+            assert!(matches!(
+                TransportId::from_str("==="),
+                Err(TransportIdError::BadId(_))
+            ));
+        }
+
+        #[cfg(not(feature = "pt-client"))]
+        {
+            assert!(matches!(
+                TransportId::from_str("obfs4"),
+                Err(TransportIdError::NoSupport)
+            ))
+        }
+    }
+
+    #[test]
+    fn settings() {
+        let s = PtTargetSettings::try_from(vec![]).unwrap();
+        assert_eq!(Vec::<_>::from(s), vec![]);
+
+        let v = vec![("abc".into(), "def".into()), ("ghi".into(), "jkl".into())];
+        let s = PtTargetSettings::try_from(v.clone()).unwrap();
+        assert_eq!(Vec::<_>::from(s), v);
+
+        let v = vec![("a=b".into(), "def".into())];
+        let s = PtTargetSettings::try_from(v);
+        assert!(matches!(s, Err(PtTargetInvalidSetting::Key(_))));
+
+        let v = vec![("abc".into(), "d ef".into())];
+        let s = PtTargetSettings::try_from(v);
+        assert!(matches!(s, Err(PtTargetInvalidSetting::Value(_))));
+    }
+
+    #[test]
+    fn chanmethod_direct() {
+        let a1 = "127.0.0.1:8080".parse().unwrap();
+        let a2 = "127.0.0.2:8181".parse().unwrap();
+        let a3 = "127.0.0.3:8282".parse().unwrap();
+
+        let m = ChannelMethod::Direct(vec![a1, a2]);
+        assert_eq!(m.socket_addrs(), Some(&[a1, a2][..]));
+        assert_eq!((m.target_addr()), Some(BridgeAddr::IpPort(a1)));
+        assert!(m.is_direct());
+        assert_eq!(m.transport_id(), TransportId::default());
+
+        let m2 = ChannelMethod::Direct(vec![a1, a2, a3]);
+        assert!(m.contained_by(&m));
+        assert!(m.contained_by(&m2));
+        assert!(!m2.contained_by(&m));
+
+        let mut m3 = m2.clone();
+        m3.retain_addrs(|a| a.port() != 8282).unwrap();
+        assert_eq!(m3, m);
+        assert_ne!(m3, m2);
+    }
+
+    #[test]
+    #[cfg(feature = "pt-client")]
+    fn chanmethod_pt() {
+        use itertools::Itertools;
+
+        let transport = "giraffe".parse().unwrap();
+        let addr1 = BridgeAddr::HostPort("pt.example.com".into(), 1234);
+        let target1 = PtTarget::new("giraffe".parse().unwrap(), addr1.clone());
+        let m1 = ChannelMethod::Pluggable(target1);
+
+        let addr2 = BridgeAddr::IpPort("127.0.0.1:567".parse().unwrap());
+        let target2 = PtTarget::new("giraffe".parse().unwrap(), addr2.clone());
+        let m2 = ChannelMethod::Pluggable(target2);
+
+        let addr3 = BridgeAddr::None;
+        let target3 = PtTarget::new("giraffe".parse().unwrap(), addr3.clone());
+        let m3 = ChannelMethod::Pluggable(target3);
+
+        assert_eq!(m1.socket_addrs(), None);
+        assert_eq!(
+            m2.socket_addrs(),
+            Some(&["127.0.0.1:567".parse().unwrap()][..])
+        );
+        assert_eq!(m3.socket_addrs(), None);
+
+        assert_eq!(m1.target_addr(), Some(addr1));
+        assert_eq!(m2.target_addr(), Some(addr2));
+        assert_eq!(m3.target_addr(), Some(addr3));
+
+        assert!(!m1.is_direct());
+        assert!(!m2.is_direct());
+        assert!(!m3.is_direct());
+
+        assert_eq!(m1.transport_id(), transport);
+        assert_eq!(m2.transport_id(), transport);
+        assert_eq!(m3.transport_id(), transport);
+
+        for v in [&m1, &m2, &m3].iter().combinations(2) {
+            let first = v[0];
+            let second = v[1];
+            assert_eq!(first.contained_by(second), first == second);
+        }
+
+        let mut m1new = m1.clone();
+        let mut m2new = m2.clone();
+        let mut m3new = m3.clone();
+        // this will retain the IpPort target, and ignore the other targets.
+        m1new.retain_addrs(|a| a.port() == 567).unwrap();
+        m2new.retain_addrs(|a| a.port() == 567).unwrap();
+        m3new.retain_addrs(|a| a.port() == 567).unwrap();
+        assert_eq!(m1new, m1);
+        assert_eq!(m2new, m2);
+        assert_eq!(m3new, m3);
+
+        // But if we try to remove the ipport target, we get an error.
+        assert!(matches!(
+            m2new.retain_addrs(|a| a.port() == 999),
+            Err(RetainAddrsError::NoAddrsLeft)
+        ));
+    }
 }