GH 731 - review #362
GH 731 - review #362
Conversation
…d bools for data_dir, config_file and real_user to MultiConfig, fixing the test
…e able to push into Vcl Vector, add processing in MultiConfig, add processing in server_initializer_collected_params
… to MultiConfig and selecting them to computed_value_names on behalf of their type
…ix config file arguments data
…inery to determine if real-user is specified in config-file
…zer_collected_params, closure for keeping in node_configurator, final removal of redundant construction of multiconfig
…ers_from_config_file, some optimization
…orted server_initializer_collected_params and determine_user_specific_data fncs, new fns value_opt for VclArg and tests for them
…ed_params, added handling for tilde in data_directory and config_file paths
…er_collected_params, fixed formatting for get_real_user_from_vcl
…zer_collected_params_handle_config_file_from_commandline_and_real_user_from_config_file_with_data_dir_started_by_tilde
…tiConfig, dealing with comments
masq_lib/src/multi_config.rs
Outdated
| @@ -949,6 +1037,40 @@ pub mod tests { | |||
| assert_eq!(subject.args(), command_line); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn command_line_vcl_return_value_from_vcl_args_by_name() { | |||
| let command_line: Vec<String> = vec![ | |||
There was a problem hiding this comment.
Isn't there a utility function with a long name that you could use in place of this? Something like vec_of_slices_to_vec_of_strings() or something?
masq_lib/src/multi_config.rs
Outdated
| .collect(); | ||
|
|
||
| let subject = CommandLineVcl::new(command_line.clone()); | ||
| let existing_value = match subject |
There was a problem hiding this comment.
I think this match at the beginning could be replaced with a .map() at the end. Also, this sequence of code that you write once for existing_value and once for non_existing_value could probably be made into a local function or closure and called twice. If locating a VclArg inside a VirtualCommandLine by name turns out to be useful enough, it could turn into a local utility function that other tests could use.
|
|
||
| let multi_config = gathered_params.multi_config; | ||
| let args = ArgsBuilder::new().param("--data-directory", home_dir.to_str().unwrap()); | ||
| let args_vec: Vec<String> = args.into(); |
There was a problem hiding this comment.
Are you creating a slice, converting to a Vec, and then back to a slice? I don't know what ArgsBuilder::new() produces.
There was a problem hiding this comment.
this is some machinery I took from other tests, should I investigate it?
There was a problem hiding this comment.
try this on one place and then create card out of it (good first issue)
| { | ||
| assert_eq!( | ||
| value_m!(env_multiconfig, "data-directory", String).unwrap(), | ||
| "booga/data_dir/MASQ/polygon-mainnet".to_string() |
There was a problem hiding this comment.
I don't understand where the "data_dir" part of this path came from. If it came from the DirsWrapperMock object, it seems as though it should have the complicated home_dir pasted on the beginning of it.
There was a problem hiding this comment.
data_dir is the placeholder in tests for .local/share directory
| let result = server_initializer_collected_params(&dir_wrapper, args_vec.as_slice()); | ||
| let env_multiconfig = result.unwrap(); | ||
|
|
||
| assert_eq!(env_multiconfig.is_user_specified("--data-directory"), false); |
There was a problem hiding this comment.
These assertions look to me to be very similar--maybe identical--to the assertions in the test above. So I wonder: how are these tests different? Are both required?
There was a problem hiding this comment.
this test is testing the scenario, when we specify the config-file with dot, assertions just proving that we correct the path correctly and we can read from config file - so real-user and dns-servers are asserted right way
| fill_up_config_file(config_file); | ||
|
|
||
| let env_vec_array = vec![ | ||
| ("MASQ_CONFIG_FILE", "config.toml"), |
There was a problem hiding this comment.
config.toml is a default. Can you specify something different here to prove that the default isn't leaking through somehow?
| environment_vcl: Box<dyn VirtualCommandLine>, | ||
| commandline_vcl: Box<dyn VirtualCommandLine>, | ||
| user_specific_data: UserSpecifiedData, | ||
| ) -> (Vec<String>, Vec<String>) { |
There was a problem hiding this comment.
I still think these _vcls could be replaced with a MultiConfig. ...sorry...
| user_specific_data: UserSpecifiedData, | ||
| ) -> (Vec<String>, Vec<String>) { | ||
| let mut unspecified_vec: Vec<String> = vec!["".to_string()]; | ||
| let mut specified_vec: Vec<String> = vec!["".to_string()]; |
There was a problem hiding this comment.
Could you move these down to nearer where they're used?
| |key: &str, value: &str, specified: bool| match value.is_empty() { | ||
| true => (), | ||
| false => match specified { | ||
| true => match specified_vec.contains(&key.to_string()) { |
There was a problem hiding this comment.
I see some code duplication here. You already have some local functions; you can probably pull out the match [un]specified_vec part into another one.
Also, the do-this-if-it-already-exists-or-this-if-it-doesn't logic is already part of HashMap, if you decided to use that. Unfortunately, what you need to make a VCL is a vector. However, there might be an easy way to convert a HashMap into a Vec with pairs. I don't know, but it might be worth looking into to simplify this code.
There was a problem hiding this comment.
Turns out I think you can do it by calling .iter() on the HashMap and then fold() on the result, starting with an empty vector. Every time through the closure you pass to fold(), push the two items in the pair onto the vector; that should do it, maybe in two or three lines.
| user_specific_data.real_user_spec, | ||
| ); | ||
|
|
||
| let (cmd_real_user, cmd_real_user_specified) = extract_value_from_vcl( |
There was a problem hiding this comment.
Three operations on real-user might be unnecessary here if you pass in a MultiConfig instead of three vectors.
| Box::new(environment_vcl), | ||
| Box::new(commandline_vcl), | ||
| ]; | ||
| //TODO write test for MultiConfig "try_new" merge line 76 |
There was a problem hiding this comment.
This should probably either be done or removed.
| Box::new(commandline_vcl), | ||
| ]; | ||
| //TODO write test for MultiConfig "try_new" merge line 76 | ||
| if unspecified_vec.len() > 1 { |
There was a problem hiding this comment.
I think you can get rid of these if statements. If a Vcl is empty, it won't affect the MultiConfig.
| user_specific_data.real_user_spec, | ||
| ); | ||
|
|
||
| let mut fill_specified_or_unspecified_box = |
There was a problem hiding this comment.
I notice that there are a lot of paths through this code. I'm concerned that not all of them may be exercised by tests. If this piece of functionality retains a significant amount of complexity after you switch to MultiConfig, I'd recommend that you run the tests at least once with Coverage and make sure that all the paths through the code are covered.
…dles_only_path_in_config_file_param
…uces malfuncioning behaviour
…e_configurator_standard
No description provided.