GH 731 - review by czarte · Pull Request #362 · MASQ-Project/Node

czarte · 2023-11-10T12:00:56Z

No description provided.

…roperly

…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

…r_specific_data

…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

dnwiebe

Resume at node_configurator/mod.rs

dnwiebe · 2023-11-10T13:01:57Z


+    #[test]
+    fn command_line_vcl_return_value_from_vcl_args_by_name() {
+        let command_line: Vec<String> = vec![


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?

dnwiebe · 2023-11-10T13:08:18Z

+        .collect();
+
+        let subject = CommandLineVcl::new(command_line.clone());
+        let existing_value = match subject


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.

dnwiebe

Resume at node_configurator_standard.rs:1001

dnwiebe · 2023-11-14T12:52:01Z

-
-        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();


Are you creating a slice, converting to a Vec, and then back to a slice? I don't know what ArgsBuilder::new() produces.

this is some machinery I took from other tests, should I investigate it?

try this on one place and then create card out of it (good first issue)

dnwiebe · 2023-11-14T13:05:35Z

+        {
+            assert_eq!(
+                value_m!(env_multiconfig, "data-directory", String).unwrap(),
+                "booga/data_dir/MASQ/polygon-mainnet".to_string()


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.

data_dir is the placeholder in tests for .local/share directory

dnwiebe · 2023-11-14T13:18:12Z

+        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);


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?

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

dnwiebe

Resume at node_configurator_standard.rs:253

dnwiebe · 2023-11-15T13:02:18Z

+        fill_up_config_file(config_file);
+
+        let env_vec_array = vec![
+            ("MASQ_CONFIG_FILE", "config.toml"),


config.toml is a default. Can you specify something different here to prove that the default isn't leaking through somehow?

dnwiebe · 2023-11-15T13:06:27Z

+    environment_vcl: Box<dyn VirtualCommandLine>,
+    commandline_vcl: Box<dyn VirtualCommandLine>,
+    user_specific_data: UserSpecifiedData,
+) -> (Vec<String>, Vec<String>) {


I still think these _vcls could be replaced with a MultiConfig. ...sorry...

dnwiebe · 2023-11-15T13:11:36Z

+    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()];


Could you move these down to nearer where they're used?

dnwiebe · 2023-11-15T13:16:47Z

+        |key: &str, value: &str, specified: bool| match value.is_empty() {
+            true => (),
+            false => match specified {
+                true => match specified_vec.contains(&key.to_string()) {


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.

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.

dnwiebe · 2023-11-15T13:24:27Z

+        user_specific_data.real_user_spec,
+    );
+
+    let (cmd_real_user, cmd_real_user_specified) = extract_value_from_vcl(


Three operations on real-user might be unnecessary here if you pass in a MultiConfig instead of three vectors.

dnwiebe · 2023-11-16T11:52:19Z

+        Box::new(environment_vcl),
+        Box::new(commandline_vcl),
+    ];
+    //TODO write test for MultiConfig "try_new" merge line 76


This should probably either be done or removed.

dnwiebe · 2023-11-16T11:58:32Z

+        Box::new(commandline_vcl),
+    ];
+    //TODO write test for MultiConfig "try_new" merge line 76
+    if unspecified_vec.len() > 1 {


I think you can get rid of these if statements. If a Vcl is empty, it won't affect the MultiConfig.

dnwiebe · 2023-11-16T12:01:08Z

+        user_specific_data.real_user_spec,
+    );
+
+    let mut fill_specified_or_unspecified_box =


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.

…ry relative

…dles_only_path_in_config_file_param

…uces malfuncioning behaviour

…e_configurator_standard

czarte added 24 commits August 23, 2023 20:33

begining with test server_initializer_collected_params_combine_vlcs_p…

cedb1b3

…roperly

removed GatheredParams from server_initializer_collected_params, adde…

32dd2cb

…d bools for data_dir, config_file and real_user to MultiConfig, fixing the test

add ComputedVcl struct and its implementation, add VclType enum, to b…

690b099

…e able to push into Vcl Vector, add processing in MultiConfig, add processing in server_initializer_collected_params

back to VirtualCommandLine type without enum, passing different types…

0d7427b

… to MultiConfig and selecting them to computed_value_names on behalf of their type

fixed data flow with Evironment and Command line arguments, need to f…

22e607b

…ix config file arguments data

changes in test to access config.file for various scenarios, add mach…

b9a6e85

…inery to determine if real-user is specified in config-file

try to get rid of redundant MultiConfig construction in determine_use…

08349e2

…r_specific_data

handle computed arguments in multiconfig, closure for server_initiali…

8aead2a

…zer_collected_params, closure for keeping in node_configurator, final removal of redundant construction of multiconfig

computing env_args and cmd_args in create_preorientation_args fn!

6167856

fix passing test server_initializer_collected_params_can_read_paramet…

7286ee8

…ers_from_config_file, some optimization

fixed VclArg problem with withdrawing data from VirtualCommandline, s…

fce6fe1

…orted server_initializer_collected_params and determine_user_specific_data fncs, new fns value_opt for VclArg and tests for them

finalize preparing guts for multiconfig in server_initializer_collect…

4dd0932

…ed_params, added handling for tilde in data_directory and config_file paths

fix windows test of real_user soecified

413ffc5

renaming, adding TODOs, removing resolved TODOS

18d0cc1

fixing tests for windows os

efd3fbb

fixed on windows

cf6da59

formatting

34b8fea

fixed real user for windows

5895879

removed filling specified and unspecified boxes from server_initializ…

62e99e3

…er_collected_params, fixed formatting for get_real_user_from_vcl

last fixes of extract_values_and_fill_boxes, and test server_initiali…

8369ff9

…zer_collected_params_handle_config_file_from_commandline_and_real_user_from_config_file_with_data_dir_started_by_tilde

fixed windows test for tilde

5b02822

fix tilde test for windows

0603f70

renaming of tests and functions

2ec7e12

remove GatheredParams (the goal), optimizing is_user_specified in Mul…

fe491ab

…tiConfig, dealing with comments

czarte requested a review from dnwiebe November 10, 2023 12:00

dnwiebe reviewed Nov 10, 2023

View reviewed changes

dnwiebe reviewed Nov 13, 2023

View reviewed changes

dnwiebe reviewed Nov 14, 2023

View reviewed changes

dnwiebe reviewed Nov 15, 2023

View reviewed changes

dnwiebe reviewed Nov 16, 2023

View reviewed changes

czarte and others added 27 commits December 4, 2023 12:51

fixed the config_path reference bin replace_dots fn

0c8a866

solving dereferencing path in replace_dots fn

3a46f00

solving config_path in replace_dots fn

8205800

fixed get_config_file_from_mc fn - optimized for readability

151ab18

formatting

dd290ea

removed println, and formated

08930b9

fixed windows tests

15e66e6

formatting

1391842

workaround for actual_server_drop test

6973082

forgotten u64 for another test in connection_termination_test.rs

9ad7413

add one more request to actual_serve_drop

ccdb0ad

remove forgotten piece of code

d50c740

fixing comments from review2

2606ebd

remove redundant lifetime, and remove convertion to PathBuf

553ac0b

fixing absolute path for tests of handle config-file and data directo…

a3bce20

…ry relative

run the actions

e7fdf8e

merge master into GH-731

e5b8a6e

Merge branch 'master' into GH-731

09f84ce

fix ENV and CLAP Guards in tests

1877513

fix Windows panic message for server_initializer_collected_params_han…

29a1c3d

…dles_only_path_in_config_file_param

formatting

4bae1dd

removing debugging println from accountant

4a75406

fix CLI UI issue with config file

ee900f9

remove canonicalize() from test uses home_dir() from dirs crate. Prod…

3b829b7

…uces malfuncioning behaviour

Reformatted some code, added a comment

cb58d97

formatting, fix order of Vcls in setup_reporter, add assertion to nod…

7110f4f

…e_configurator_standard

Merge branch 'master' into GH-731

9fb3296

dnwiebe approved these changes Feb 7, 2024

View reviewed changes

Merge branch 'master' into GH-731

177cc88

czarte merged commit a50e526 into master Feb 7, 2024

Uh oh!

Conversation

czarte commented Nov 10, 2023

Uh oh!

dnwiebe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dnwiebe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnwiebe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees