arti: Fix TODO: Add test for onion_proxy::build_list
Resolves a TODO mentioning the need for a test for arti::onion_proxy::build_list
Merge request reports
Activity
requested review from @opara
392 /// builders, producing a `Vec` of the results. 393 /// 394 /// # Panics 395 /// 396 /// Will panic if any of the builders supplied are invalid. 397 macro_rules! build_all { 398 ($builders:expr) => ( 399 $builders 400 .into_iter() 401 .map(|builder| builder.build().unwrap()) 402 .collect::<Vec<_>>() 403 ); 404 ($($builder:expr),+ $(,)?) => ( 405 vec![ $($builder.build().unwrap()),+ ] 406 ); 407 } 391 /// Takes either a `Vec` or a comma-separated list of 392 /// builders, producing a `Vec` of the results. 393 /// 394 /// # Panics 395 /// 396 /// Will panic if any of the builders supplied are invalid. 397 macro_rules! build_all { 398 ($builders:expr) => ( 399 $builders 400 .into_iter() 401 .map(|builder| builder.build().unwrap()) 402 .collect::<Vec<_>>() 403 ); 404 ($($builder:expr),+ $(,)?) => ( 405 vec![ $($builder.build().unwrap()),+ ] 406 ); changed this line in version 2 of the diff
70 70 #[cfg(feature = "onion-service-service")] 71 71 pub(crate) type OnionServiceProxyConfigMap = BTreeMap<HsNickname, OnionServiceProxyConfig>; 72 72 73 /// The serialized format of an OnionServiceProxyConfigListBuilder: 74 /// a map from nickname to `OnionServiceConfigBuilder` 73 /// The serialized format of an `OnionServiceProxyConfigMapBuilder`: 74 /// a map from `HsNickname` to `OnionServiceConfigBuilder` changed this line in version 2 of the diff
407 } 408 409 #[test] 410 fn fn_build_list() { 411 let nick_1 = HsNickname::new("nick_1".to_string()).unwrap(); 412 let nick_2 = HsNickname::new("nick_2".to_string()).unwrap(); 413 414 let proxy_configs = build_all![ 415 onion_service_proxy_config_builder!(nick_1), 416 onion_service_proxy_config_builder!(nick_2), 417 ]; 418 let actual = build_list(proxy_configs.clone()); 419 420 assert!(actual.is_ok()); 421 422 let actual = actual.unwrap(); - Comment on lines +418 to +422
changed this line in version 2 of the diff
402 .collect::<Vec<_>>() 403 ); 404 ($($builder:expr),+ $(,)?) => ( 405 vec![ $($builder.build().unwrap()),+ ] 406 ); 407 } 408 409 #[test] 410 fn fn_build_list() { 411 let nick_1 = HsNickname::new("nick_1".to_string()).unwrap(); 412 let nick_2 = HsNickname::new("nick_2".to_string()).unwrap(); 413 414 let proxy_configs = build_all![ 415 onion_service_proxy_config_builder!(nick_1), 416 onion_service_proxy_config_builder!(nick_2), 417 ]; - Comment on lines +414 to +417
I feel like this would be simpler as just the following, no helper macros/functions needed. It's about as short, and you don't need to look around at helper functions to know what it's doing.
let proxy_configs: Vec<_> = [&nick_1, &nick_2].into_iter().map(|nick| { let mut builder = OnionServiceProxyConfigBuilder::default(); builder.service().nickname(nick.clone()); builder.build().unwrap() }).collect();And the same for the duplicate case below.
changed this line in version 2 of the diff
395 /// 396 /// Will panic if any of the builders supplied are invalid. 397 macro_rules! build_all { 398 ($builders:expr) => ( 399 $builders 400 .into_iter() 401 .map(|builder| builder.build().unwrap()) 402 .collect::<Vec<_>>() 403 ); 404 ($($builder:expr),+ $(,)?) => ( 405 vec![ $($builder.build().unwrap()),+ ] 406 ); 407 } 408 409 #[test] 410 fn fn_build_list() { - Comment on lines +409 to +410
Can you add a comment to this
fn_build_listfunction explaining what it's testing? Something like "Testing [`build_list`] with and without duplicate service nicknames".Edited by opara
13 13 // part of the configuration was invalid. 14 14 // 15 15 // This is part of the public API. 16 #[derive(Debug, Clone, thiserror::Error)] 16 #[derive(Debug, Clone, thiserror::Error, PartialEq, Eq)] 17 17 #[non_exhaustive] 18 18 pub enum ConfigBuildError { - Comment on lines 8 to 18
I'm hesitant to derive
PartialEqandEqforConfigBuildError, since it means that we can never put non-PartialEqtypes here in the future, and we only need this functionality for a test.Can you change
fn_build_listto do something like:let ConfigBuildError::Inconsistent { fields, problem } = actual else { panic!("Unexpected error from `build_list`: {expected}"); }; assert_eq!(fields, vec!["nickname".to_string()]); assert_eq!(problem, format!("Multiple onion services with the nickname {nick_dup}"));and remove the
PartialEq, Eqhere?Edited by opara
added 1 commit
- ec674740 - arti: Fix TODO: Rework test: onion_proxy::build_list
I'm afraid the
maint-check-cbindgentest job won't pass, !2901 (merged) fixes this, but it was merged after I opened this MR, so I don't have those changes on branchtodo1. LMK if I should update my local branch.if we restart it enough times it will pass
I tried quite a few times hours ago then just gave up :(
-
I'm afraid the
maint-check-cbindgentest job won't pass, !2901 (merged) fixes this, but it was merged after I opened this MR, so I don't have those changes on branchtodo1. LMK if I should update my local branch.Okay, yeah please rebase this MR on the latest changes on main then, and then force push here. If you have any questions about this lmk.
if we restart it enough times it will pass
I tried quite a few times hours ago then just gave up :(
Yeah, it's painful. I'll take care of restarting the jobs to make sure they run.
-
Rebased and pushed. Latest commit from
mainintodo1is now8f85ef297dfff71ad586b7784a39ea25a133505bEdited by playbahn -
if we restart it enough times it will pass
I tried quite a few times hours ago then just gave up :(
I think what's happening here is that the proxy is trying to download the container image from Dockerhub, but has reached its rate limit. It might take a few hours for the rate limit to reset.
added 29 commits
-
ec674740...8f85ef29 - 27 commits from branch
tpo/core:main - b70fc14b - arti: onion_proxy::build_list: fix TODO: Add test
- a535c1b9 - arti: Fix TODO: Rework test: onion_proxy::build_list
-
ec674740...8f85ef29 - 27 commits from branch
mentioned in commit ebae86a9