Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-783 Fix tests failing on non standard environment #432

Merged
merged 17 commits into from
Apr 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 11 additions & 17 deletions node/src/daemon/setup_reporter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1226,7 +1226,6 @@ mod tests {
};
use crate::test_utils::{assert_string_contains, rate_pack};
use core::option::Option;
use dirs::home_dir;
use masq_lib::blockchains::chains::Chain as Blockchain;
use masq_lib::blockchains::chains::Chain::PolyAmoy;
use masq_lib::constants::{DEFAULT_CHAIN, DEFAULT_GAS_PRICE};
Expand All @@ -1239,6 +1238,7 @@ mod tests {
use std::convert::TryFrom;
#[cfg(not(target_os = "windows"))]
use std::default::Default;
use std::env::current_dir;
use std::fs::{create_dir_all, File};
use std::io::Write;
use std::net::IpAddr;
Expand Down Expand Up @@ -2046,21 +2046,16 @@ mod tests {
}

#[test]
fn get_modified_setup_tilde_in_config_file_path() {
fn get_modified_setup_handles_tilde_in_config_file_and_data_directory_path() {
let _guard = EnvironmentGuard::new();
let base_dir = ensure_node_home_directory_exists(
"setup_reporter",
"get_modified_setup_tilde_in_data_directory",
"get_modified_setup_handles_tilde_in_config_file_and_data_directory_path",
);
let data_dir = base_dir.join("data_dir");
std::fs::create_dir_all(home_dir().expect("expect home dir").join("masqhome")).unwrap();
let mut config_file = File::create(
home_dir()
.expect("expect home dir")
.join("masqhome")
.join("config.toml"),
)
.unwrap();
std::fs::create_dir_all(base_dir.join("masqhome")).unwrap();
let config_file_path = base_dir.join("masqhome").join("config.toml");
let mut config_file = File::create(&config_file_path).unwrap();
config_file
.write_all(b"blockchain-service-url = \"https://www.mainnet.com\"\n")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mentioned it at the other test already, please try eliminating the DirsWrapperMock.

.unwrap();
Expand All @@ -2082,12 +2077,11 @@ mod tests {
.collect_vec();

let expected_config_file_data = "https://www.mainnet.com";
let dirs_wrapper = Box::new(
DirsWrapperMock::new()
.data_dir_result(Some(data_dir))
.home_dir_result(Some(base_dir)),
);
let subject = SetupReporterReal::new(dirs_wrapper);
let dirs_wrapper = DirsWrapperMock {
data_dir_result: Some(PathBuf::from(current_dir().unwrap().join(&data_dir))),
home_dir_result: Some(PathBuf::from(current_dir().unwrap().join(&base_dir))),
};
let subject = SetupReporterReal::new(Box::new(dirs_wrapper));

let result = subject
.get_modified_setup(existing_setup, incoming_setup)
Expand Down
13 changes: 8 additions & 5 deletions node/src/node_configurator/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -119,15 +119,16 @@ fn get_data_directory_from_mc(
}
}

fn replace_tilde(config_path: PathBuf) -> PathBuf {
fn replace_tilde(config_path: PathBuf, dirs_wrapper: &dyn DirsWrapper) -> PathBuf {
match config_path.starts_with("~") {
true => PathBuf::from(
config_path.display().to_string().replacen(
'~',
home_dir()
.expect("expected users home_dir")
dirs_wrapper
.home_dir()
.expect("expected users home dir")
.to_str()
.expect("expected str home_dir"),
.expect("expected home dir"),
1,
),
),
Expand Down Expand Up @@ -166,12 +167,13 @@ fn get_config_file_from_mc(
multi_config: &MultiConfig,
data_directory: &Path,
data_directory_def: bool,
dirs_wrapper: &dyn DirsWrapper,
) -> FieldPair<PathBuf> {
let mut panic: bool = false;
let config_file = value_m!(multi_config, "config-file", PathBuf);
match config_file {
Some(config_path) => {
let config_path = replace_tilde(config_path);
let config_path = replace_tilde(config_path, dirs_wrapper);
let config_path = replace_dots(config_path);
let config_path =
replace_relative_path(config_path, data_directory_def, data_directory, &mut panic);
Expand Down Expand Up @@ -216,6 +218,7 @@ fn config_file_data_dir_real_user_chain_from_mc(
&multi_config,
&initialization_data.data_directory.item,
initialization_data.data_directory.user_specified,
dirs_wrapper,
);
initialization_data
}
Expand Down
42 changes: 25 additions & 17 deletions node/src/node_configurator/node_configurator_standard.rs
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,6 @@ mod tests {
make_pre_populated_mocked_directory_wrapper, make_simplified_multi_config,
};
use crate::test_utils::{assert_string_contains, main_cryptde, ArgsBuilder};
use dirs::home_dir;
use masq_lib::blockchains::chains::Chain;
use masq_lib::constants::DEFAULT_CHAIN;
use masq_lib::multi_config::VirtualCommandLine;
Expand Down Expand Up @@ -1075,14 +1074,16 @@ mod tests {
}

#[test]
fn server_initializer_collected_params_handle_tilde_in_path_config_file_from_commandline_and_real_user_from_config_file(
) {
fn tilde_in_config_file_path_from_commandline_and_args_uploaded_from_config_file() {
running_test();
let _guard = EnvironmentGuard::new();
let _clap_guard = ClapGuard::new();
let home_dir = home_dir().expect("expectexd home dir");
let data_dir = &home_dir.join("masqhome");
let _create_data_dir = create_dir_all(data_dir);
let home_dir = ensure_node_home_directory_exists(
"node_configurator_standard",
"tilde_in_config_file_path_from_commandline_and_args_uploaded_from_config_file",
);
let data_dir = home_dir.join("masqhome");
let _dir = create_dir_all(&data_dir);
let config_file_relative = File::create(data_dir.join("config.toml")).unwrap();
fill_up_config_file(config_file_relative);
let env_vec_array = vec![
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot write it lower than here but I continue to think that the DirsWrapperMock doesn't belong in here, it should only be the real one. The code should figure out home_dir and data_dir on its own, genuinely. It would be possible that the test wouldn't pass for that reason of what the tests claims but just because you determined it by these supplied values in the Mock. If I'm wrong and the test fails then... I'd be disappointed... but I think we'll have to think that out even further.

Expand All @@ -1105,16 +1106,30 @@ mod tests {
.param("--config-file", "~\\masqhome\\config.toml")
.param("--data-directory", "~\\masqhome");
let args_vec: Vec<String> = args.into();
let dir_wrapper = DirsWrapperMock::new()
.home_dir_result(Some(home_dir.to_path_buf()))
.data_dir_result(Some(data_dir.to_path_buf()));
let dir_wrapper = DirsWrapperMock {
data_dir_result: Some(PathBuf::from(current_dir().unwrap().join(&data_dir))),
home_dir_result: Some(PathBuf::from(current_dir().unwrap().join(&home_dir))),
};

let result = server_initializer_collected_params(&dir_wrapper, args_vec.as_slice());
let multiconfig = result.unwrap();

assert_eq!(
value_m!(multiconfig, "data-directory", String).unwrap(),
data_dir.to_string_lossy().to_string()
current_dir()
.unwrap()
.join(&data_dir)
.to_string_lossy()
.to_string()
);
assert_eq!(
value_m!(multiconfig, "config-file", String).unwrap(),
current_dir()
.unwrap()
.join(data_dir)
.join(PathBuf::from("config.toml"))
.to_string_lossy()
.to_string()
);
#[cfg(not(target_os = "windows"))]
{
Expand All @@ -1123,13 +1138,6 @@ mod tests {
"9999:9999:booga"
);
}
assert_eq!(
value_m!(multiconfig, "config-file", String).unwrap(),
data_dir
.join(PathBuf::from("config.toml"))
.to_string_lossy()
.to_string()
);
assert_eq!(
value_m!(multiconfig, "blockchain-service-url", String).unwrap(),
"https://www.mainnet1.com"
Expand Down
13 changes: 7 additions & 6 deletions node/src/test_utils/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,13 @@ use std::collections::btree_set::BTreeSet;
use std::collections::HashSet;
use std::convert::From;
use std::fmt::Debug;

use std::hash::Hash;
use std::io::ErrorKind;
use std::io::Read;
use std::iter::repeat;
use std::net::{Shutdown, TcpStream};

use std::str::FromStr;
use std::sync::{Arc, Mutex};
use std::thread;
Expand Down Expand Up @@ -1221,18 +1223,17 @@ pub mod unshared_test_utils {

#[cfg(test)]
mod tests {
use std::borrow::BorrowMut;
use std::iter;
use std::sync::{Arc, Mutex};
use std::thread;
use std::time::Duration;

use crate::sub_lib::cryptde::CryptData;
use crate::sub_lib::hop::LiveHop;
use crate::sub_lib::neighborhood::ExpectedService;
use crate::test_utils::unshared_test_utils::arbitrary_id_stamp::{
ArbitraryIdStamp, FirstTraitMock, SecondTraitMock, TestSubject,
};
use std::borrow::BorrowMut;
use std::iter;
use std::sync::{Arc, Mutex};
use std::thread;
use std::time::Duration;

use super::*;

Expand Down