Re: [PATCH v5 1/3] dts: rework arguments framework

[Luca Vizzarro <Luca.Vizzarro@arm.com>]

On 30/05/2024 16:30, Juraj Linkeš wrote:
> There is a difference in behavior when I pass no arguments and then I
> either have or don't have an env var set:
> ./main.py
> usage: main.py [-h] [--config-file FILE_PATH] ...
> ...
> 
> -----------------------------
> DTS_SKIP_SETUP=Y ./main.py
> main.py: error: one of the arguments --tarball/--snapshot
> --revision/--rev/--git-ref is required
> 
> For help and usage, run the command with the --help flag.
> 
> I think this is because we're adding the env vars as actions (the
> second scenario thus has at least one) and the first scenario doesn't
> have any actions, which leads to the different output. I don't know
> whether we want to unify these, but it's a bit confusing, as the error
> applies to both cases.

The behaviour is just as expected. You get the same exact results if you 
were to run:
   ./main.py -s
which is equivalent to:
   DTS_SKIP_SETUP= ./main.py

The true equivalent to your example would be:
   ./main.py -s Y

The only anomaly is that no matter the value passed (true, false, y, 
n...) it's always going to be set because the action is `store_true`.
If we care to interpret a possible value then we need to implement a new 
action to cater for this.

>> diff --git a/dts/framework/settings.py b/dts/framework/settings.py
>> index 688e8679a7..b19f274f9d 100644
>> --- a/dts/framework/settings.py
>> +++ b/dts/framework/settings.py
> 
>> @@ -109,104 +116,224 @@ class Settings:
>>
>>   SETTINGS: Settings = Settings()
>>
>> +P = ParamSpec("P")
>>
>> -def _get_parser() -> argparse.ArgumentParser:
>> -    """Create the argument parser for DTS.
>> +#: Attribute name representing the env variable name to augment :class:`~argparse.Action` with.
>> +_ENV_VAR_NAME_ATTR = "env_var_name"
>> +#: Attribute name representing the value origin to augment :class:`~argparse.Action` with.
>> +_IS_FROM_ENV_ATTR = "is_from_env"
>>
>> -    Command line options take precedence over environment variables, which in turn take precedence
>> -    over default values.
>> +#: The prefix to be added to all of the environment variables.
>> +ENV_PREFIX = "DTS_"
>> +
>> +
>> +def is_action_in_args(action: Action) -> bool:
> 
> I don't think there's any expectation that these functions will be
> used outside this module, so I'd make them all private, in which case
> the short docstring would be fine.
> 
> The ParamSpec also maybe should be private. Same goes for ENV_PREFIX.

Ack.

> I'm also looking at the order of the various functions, classes and
> variables in the module and it looks all over the place. Maybe we can
> tidy it up a bit.

Could you please elaborate on this? It is also important to define 
what's the ordering expectation. At the moment they are mostly in order 
of usage, except for the Settings which I purposefully left on top as 
they are easier to find and probably the most "needed" for a user. As 
for the env var helper functions, most of them are used rightaway, but I 
left a couple that are used later there so that they are all grouped 
together (and related).

>> +class ArgumentParser(argparse.ArgumentParser):
> 
> I'd rename this to DTSArgumentParser and maybe also make the classes
> (this one and the formatter) private.
> 

Ack.
>> +    Instead of printing usage on every error, it prints instructions
>> +    on how to do it.
> 
> This sentence is confusing - how to do what?

This could do without the sentence to be honest, I'll just remove it.

>> +    def find_action(
>> +        self, action_name: str, filter_fn: Callable[[Action], bool] | None = None
>> +    ) -> Action | None:
>> +        """Find and return an action by its name.
>> +
>> +        Arguments:
>> +            action_name: the name of the action to find.
>> +            filter_fn: a filter function to use in the search.
> 
> It's not very clear from the description how this filter is applied.
> We should mention that the found action (and not action_name) is going
> to be passed to filter_fn.

Ack.

>> -        return os.environ.get(env_var) or default
>> +        it = (action for action in self._actions if action_name == _get_action_name(action))
>> +        action = next(it, None)
>> +
>> +        if action is not None and filter_fn is not None:
> 
> Would a simplified condition "if action and filter_fn" work?

Should actually work in this case. Will change it.


>> +class EnvVarHelpFormatter(ArgumentDefaultsHelpFormatter):
>> +    """Custom formatter to add environment variables in the help page."""
> 
> to the help page

Ack.

>> +        if env_var_name is not None:
>> +            help = f"[{env_var_name}] {help}"
>> +
>> +            env_var_value = os.environ.get(env_var_name)
>> +            if env_var_value is not None:
>> +                help += f" (env value: {env_var_value})"
> 
> Let's do this the same way as four lines above: help = f"{help} (env
> value: {env_var_value})"

Ack.

>> -    parser.add_argument(
>> +    add_argument_to_parser_with_env = augment_add_argument_with_env(parser.add_argument)
> 
> I'm wondering whether we could modify the actual add_argument methods
> of ArgumentParser and _MutuallyExclusiveGroup, either the class
> methods or instance methods. Have you tried that? It could be easier
> to understand.

I tried this, but it becomes overly complicated because add_argument is 
implemented in _ActionsContainer which in turn is inherited by several 
classes, which we ultimately use. The easiest and clearest approach is 
just a wrapper.

> Or, alternatively, we could do:
> 
> action = parser.add_argument(
>      "--config-file",
>      default=SETTINGS.config_file_path,
>      type=Path,
>      help="The configuration file that describes the test cases, SUTs
> and targets.",
>      metavar="FILE_PATH",
> )
> add_env_var_to_action(action, env_var_name="CFG_FILE")
> 
> This makes what we're trying to do a bit clearer, but requires two
> calls instead of one, so maybe it's not better. I'm not sure.

I can drop the HOF and use it as `_add_env_var_to_action` as a clearer 
replacement.

>> +
>> +    add_argument_to_parser_with_env(
>>           "--config-file",
> 
> We should rename Settings.config_file_path to correspond with this.
> Otherwise it's going to be ignored by get_settings().

This was actually a miss from me. Nice catch! I've solved this issue by 
adding `dest` to the other arguments, but clearly forgot about this one.

>> @@ -240,17 +369,12 @@ def _process_test_suites(args: str | list[list[str]]) -> list[TestSuiteConfig]:
>>       Returns:
>>           A list of test suite configurations to execute.
>>       """
>> -    if isinstance(args, str):
>> -        # Environment variable in the form of "suite case case, suite case, suite, ..."
>> -        args = [suite_with_cases.split() for suite_with_cases in args.split(",")]
>> +    test_suites = parser.find_action("test_suites", is_from_env)
>> +    if test_suites is not None:
>> +        # Environment variable in the form of "SUITE1 CASE1 CASE2, SUITE2 CASE1, SUITE3, ..."
>> +        args = [suite_with_cases.split() for suite_with_cases in args[0][0].split(",")]
>>
>> -    test_suites_to_run = []
>> -    for suite_with_cases in args:
>> -        test_suites_to_run.append(
>> -            TestSuiteConfig(test_suite=suite_with_cases[0], test_cases=suite_with_cases[1:])
>> -        )
>> -
>> -    return test_suites_to_run
>> +    return [TestSuiteConfig(test_suite, test_cases) for [test_suite, *test_cases] in args]
> 
> This doesn't work properly, if I use:
> DTS_TEST_SUITES="hello_world test_hello_world_single_core, os_udp test_os_udp"
> I'm getting:
> execution_setup - dts - ERROR - Invalid test suite configuration
> found: [TestSuiteConfig(test_suite='hello_world
> test_hello_world_single_core, os_udp test_os_udp', test_cases=[])].
> ...
> ModuleNotFoundError: No module named 'tests.TestSuite_hello_world
> test_hello_world_single_core, os_udp test_os_udp'

yes, nice catch! Must have forgot to test this bit again. The culprit is 
find_action as it's meant to find by dest and not action name.