feat: dynamic runner parameters (#345)

context: codecov/engineering-team#407

Consider this a 1st iteration of this feature.
A compromise between "this is easy to implement and helpful to users"
and "users don't have to think to use this".

The benefit is that now you can add dynamic params to your runners,
the downside is that you need to know the runner configuration AND
format these params accordingly.

Otherwise it would be very hard to figure out exactly what parameter
group a given parameter is. So, compromise.

More details on whoe to actually use the feature are documented in
the code.

Author: giovanni-guidini

codecov_cli/commands/labelanalysis.py

@@ -2,7 +2,7 @@
 import logging
 import pathlib
 import time
-from typing import List, Optional
+from typing import Dict, List, Optional
 
 import click
 import requests
@@ -70,6 +70,11 @@
     help="Format in which --dry-run data is printed. Default is JSON.",
     default="json",
 )
+@click.option(
+    "--runner-param",
+    "runner_params",
+    multiple=True,
+)
 @click.pass_context
 def label_analysis(
     ctx: click.Context,
@@ -80,6 +85,7 @@ def label_analysis(
     max_wait_time: str,
     dry_run: bool,
     dry_run_format: str,
+    runner_params: List[str],
 ):
     enterprise_url = ctx.obj.get("enterprise_url")
     logger.debug(
@@ -113,7 +119,8 @@ def label_analysis(
     codecov_yaml = ctx.obj["codecov_yaml"] or {}
     cli_config = codecov_yaml.get("cli", {})
     # Raises error if no runner is found
-    runner = get_runner(cli_config, runner_name)
+    parsed_runner_params = _parse_runner_params(runner_params)
+    runner = get_runner(cli_config, runner_name, parsed_runner_params)
     logger.debug(
         f"Selected runner: {runner}",
         extra=dict(extra_log_attributes=dict(config=runner.params)),
@@ -232,6 +239,38 @@ def label_analysis(
         time.sleep(5)
 
 
+def _parse_runner_params(runner_params: List[str]) -> Dict[str, str]:
+    """Parses the structured list of dynamic runner params into a dictionary.
+    Structure is `key=value`. If value is a list make it comma-separated.
+    If the list item doesn't have '=' we consider it the key and set to None.
+
+    EXAMPLE:
+    runner_params = ['key=value', 'null_item', 'list=item1,item2,item3']
+    _parse_runner_params(runner_params) == {
+        'key': 'value',
+        'null_item': None,
+        'list': ['item1', 'item2', 'item3']
+    }
+    """
+    final_params = {}
+    for param in runner_params:
+        # Emit warning if param is not well formatted
+        # Using == 0 rather than != 1 because there might be
+        # a good reason for the param to include '=' in the value.
+        if param.count("=") == 0:
+            logger.warning(
+                f"Runner param {param} is not well formated. Setting value to None. Use '--runner-param key=value' to set value"
+            )
+            final_params[param] = None
+        else:
+            key, value = param.split("=", 1)
+            # For list values we need to split the list too
+            if "," in value:
+                value = value.split(",")
+            final_params[key] = value
+    return final_params
+
+
 def _potentially_calculate_absent_labels(
     request_result, requested_labels
 ) -> LabelAnalysisRequestResult:

codecov_cli/runners/__init__.py

@@ -15,7 +15,9 @@ class UnableToFindRunner(Exception):
     pass
 
 
-def _load_runner_from_yaml(plugin_dict: typing.Dict) -> LabelAnalysisRunnerInterface:
+def _load_runner_from_yaml(
+    plugin_dict: typing.Dict, dynamic_params: typing.Dict
+) -> LabelAnalysisRunnerInterface:
     try:
         module_obj = import_module(plugin_dict["module"])
         class_obj = getattr(module_obj, plugin_dict["class"])
@@ -32,16 +34,21 @@ def _load_runner_from_yaml(plugin_dict: typing.Dict) -> LabelAnalysisRunnerInter
         )
         raise
     try:
-        return class_obj(**plugin_dict["params"])
+        final_params = {**plugin_dict["params"], **dynamic_params}
+        return class_obj(**final_params)
     except TypeError:
         click.secho(
-            f"Unable to instantiate {class_obj} with parameters {plugin_dict['params']}",
+            f"Unable to instantiate {class_obj} with parameters {final_params}",
             err=True,
         )
         raise
 
 
-def get_runner(cli_config, runner_name) -> LabelAnalysisRunnerInterface:
+def get_runner(
+    cli_config, runner_name: str, dynamic_params: typing.Dict = None
+) -> LabelAnalysisRunnerInterface:
+    if dynamic_params is None:
+        dynamic_params = {}
     if runner_name == "pytest":
         config_params = cli_config.get("runners", {}).get("pytest", {})
         # This is for backwards compatibility with versions <= 0.3.4
@@ -52,10 +59,12 @@ def get_runner(cli_config, runner_name) -> LabelAnalysisRunnerInterface:
                 logger.warning(
                     "Using 'python' to configure the PytestStandardRunner is deprecated. Please change to 'pytest'"
                 )
-        return PytestStandardRunner(config_params)
+        final_params = {**config_params, **dynamic_params}
+        return PytestStandardRunner(final_params)
     elif runner_name == "dan":
         config_params = cli_config.get("runners", {}).get("dan", {})
-        return DoAnythingNowRunner(config_params)
+        final_params = {**config_params, **dynamic_params}
+        return DoAnythingNowRunner(final_params)
     logger.debug(
         f"Trying to load runner {runner_name}",
         extra=dict(
@@ -65,5 +74,7 @@ def get_runner(cli_config, runner_name) -> LabelAnalysisRunnerInterface:
         ),
     )
     if cli_config and runner_name in cli_config.get("runners", {}):
-        return _load_runner_from_yaml(cli_config["runners"][runner_name])
+        return _load_runner_from_yaml(
+            cli_config["runners"][runner_name], dynamic_params=dynamic_params
+        )
     raise UnableToFindRunner(f"Can't find runner {runner_name}")

codecov_cli/runners/pytest_standard_runner.py

@@ -1,3 +1,4 @@
+import inspect
 import logging
 import random
 import subprocess
@@ -42,6 +43,18 @@ def coverage_root(self) -> str:
         """
         return self.get("coverage_root", "./")
 
+    @classmethod
+    def get_available_params(cls) -> List[str]:
+        """Lists all the @property attribute names of this class.
+        These attributes are considered the 'valid config options'
+        """
+        klass_methods = [
+            x
+            for x in dir(cls)
+            if (inspect.isdatadescriptor(getattr(cls, x)) and not x.startswith("__"))
+        ]
+        return klass_methods
+
 
 class PytestStandardRunner(LabelAnalysisRunnerInterface):
 
@@ -52,8 +65,20 @@ def __init__(self, config_params: Optional[dict] = None) -> None:
         super().__init__()
         if config_params is None:
             config_params = {}
+        # Before we create the config params we emit warnings if any param is unknown
+        # So the user knows something is wrong with their config
+        self._possibly_warn_bad_config(config_params)
         self.params = PytestStandardRunnerConfigParams(config_params)
 
+    def _possibly_warn_bad_config(self, config_params: dict):
+        available_config_params = (
+            PytestStandardRunnerConfigParams.get_available_params()
+        )
+        provided_config_params = config_params.keys()
+        for provided_param in provided_config_params:
+            if provided_param not in available_config_params:
+                logger.warning(f"Config parameter '{provided_param}' is unknonw.")
+
     def parse_captured_output_error(self, exp: CalledProcessError) -> str:
         result = ""
         for out_stream in [exp.stdout, exp.stderr]:

tests/commands/test_invoke_labelanalysis.py

@@ -13,6 +13,7 @@
     _dry_run_json_output,
     _dry_run_list_output,
     _fallback_to_collected_labels,
+    _parse_runner_params,
     _potentially_calculate_absent_labels,
     _send_labelanalysis_request,
 )
@@ -169,6 +170,20 @@ def test__dry_run_space_separated_list_output(self):
             == "TESTS_TO_RUN='--option=1' '--option=2' 'label_1' 'label_2'\nTESTS_TO_SKIP='--option=1' '--option=2' 'label_3' 'label_4'\n"
         )
 
+    def test_parse_dynamic_runner_options(self):
+        params = [
+            "wrong_param",
+            "key=value",
+            "list_key=val1,val2,val3",
+            "point=somethingwith=sign",
+        ]
+        assert _parse_runner_params(params) == {
+            "wrong_param": None,
+            "key": "value",
+            "list_key": ["val1", "val2", "val3"],
+            "point": "somethingwith=sign",
+        }
+
 
 class TestLabelAnalysisCommand(object):
     def test_invoke_label_analysis_missing_token(self, mocker, fake_ci_provider):

tests/runners/test_pytest_standard_runner.py

@@ -5,7 +5,10 @@
 import pytest
 from pytest import ExitCode
 
-from codecov_cli.runners.pytest_standard_runner import PytestStandardRunner
+from codecov_cli.runners.pytest_standard_runner import (
+    PytestStandardRunner,
+    PytestStandardRunnerConfigParams,
+)
 from codecov_cli.runners.pytest_standard_runner import logger as runner_logger
 from codecov_cli.runners.pytest_standard_runner import stdout as pyrunner_stdout
 from codecov_cli.runners.types import LabelAnalysisRequestResult
@@ -39,9 +42,33 @@ def test_execute_pytest(self, mock_subprocess):
         )
         assert result == output
 
+    @patch("codecov_cli.runners.pytest_standard_runner.logger.warning")
+    def test_warning_bad_config(self, mock_warning):
+        available_config = PytestStandardRunnerConfigParams.get_available_params()
+        assert "python_path" in available_config
+        assert "collect_tests_options" in available_config
+        assert "some_missing_option" not in available_config
+        params = dict(
+            python_path="path_to_python",
+            collect_tests_options=["option1", "option2"],
+            some_missing_option="option",
+        )
+        runner = PytestStandardRunner(params)
+        # Adding invalid config options emits a warning
+        assert mock_warning.called_with(
+            "Config parameter 'some_missing_option' is unknonw."
+        )
+        # Warnings don't change the config
+        assert runner.params == {**params, "some_missing_option": "option"}
+        # And we can still access the config as usual
+        assert runner.params.python_path == "path_to_python"
+        assert runner.params.collect_tests_options == ["option1", "option2"]
+
     @pytest.mark.parametrize("python_path", ["/usr/bin/python", "venv/bin/python"])
     @patch("codecov_cli.runners.pytest_standard_runner.subprocess")
-    def test_execute_pytest_user_provided_command(self, mock_subprocess, python_path):
+    def test_execute_pytest_user_provided_python_path(
+        self, mock_subprocess, python_path
+    ):
         output = "Output in stdout"
         return_value = MagicMock(stdout=output.encode("utf-8"))
         mock_subprocess.run.return_value = return_value

tests/runners/test_runners.py

@@ -14,7 +14,7 @@ def test_get_standard_runners(self):
         assert isinstance(get_runner({}, "dan"), DoAnythingNowRunner)
         # TODO: Extend with other standard runners once we create them (e.g. JS)
 
-    def test_pytest_standard_runner_with_options_backwards_compatible(self):
+    def test_pytest_standard_runner_with_options(self):
         config_params = dict(
             collect_tests_options=["--option=value", "-option"],
         )
@@ -26,6 +26,23 @@ def test_pytest_standard_runner_with_options_backwards_compatible(self):
         )
         assert runner_instance.params.coverage_root == "./"
 
+    def test_pytest_standard_runner_with_options_and_dynamic_options(self):
+        config_params = dict(
+            collect_tests_options=["--option=value", "-option"],
+        )
+        runner_instance = get_runner(
+            {"runners": {"pytest": config_params}},
+            "pytest",
+            {"python_path": "path/to/python"},
+        )
+        assert isinstance(runner_instance, PytestStandardRunner)
+        assert (
+            runner_instance.params.collect_tests_options
+            == config_params["collect_tests_options"]
+        )
+        assert runner_instance.params.python_path == "path/to/python"
+        assert runner_instance.params.coverage_root == "./"
+
     def test_pytest_standard_runner_with_options_backwards_compatible(self):
         config_params = dict(
             collect_tests_options=["--option=value", "-option"],
@@ -56,7 +73,9 @@ def test_get_runner_from_yaml(self, mock_load_runner):
         config = {"runners": {"my_runner": {"path": "path_to_my_runner"}}}
         mock_load_runner.return_value = "MyRunner()"
         assert get_runner(config, "my_runner") == "MyRunner()"
-        mock_load_runner.assert_called_with({"path": "path_to_my_runner"})
+        mock_load_runner.assert_called_with(
+            {"path": "path_to_my_runner"}, dynamic_params={}
+        )
 
     def test_load_runner_from_yaml(self, mocker):
         fake_module = mocker.MagicMock(FakeRunner=FakeRunner)
@@ -66,7 +85,8 @@ def test_load_runner_from_yaml(self, mocker):
                 "module": "mymodule.runner",
                 "class": "FakeRunner",
                 "params": {"collect_tests_response": ["list", "of", "labels"]},
-            }
+            },
+            {},
         )
         assert isinstance(res, FakeRunner)
         assert res.collect_tests() == ["list", "of", "labels"]
@@ -83,7 +103,8 @@ def side_effect(*args, **kwargs):
                     "module": "mymodule.runner",
                     "class": "FakeRunner",
                     "params": {"collect_tests_response": ["list", "of", "labels"]},
-                }
+                },
+                {},
             )
 
     def test_load_runner_from_yaml_class_not_found(self, mocker):
@@ -97,7 +118,8 @@ def test_load_runner_from_yaml_class_not_found(self, mocker):
                     "module": "mymodule.runner",
                     "class": "WrongClassName",
                     "params": {"collect_tests_response": ["list", "of", "labels"]},
-                }
+                },
+                {},
             )
 
     def test_load_runner_from_yaml_fail_instantiate_class(self, mocker):
@@ -109,5 +131,6 @@ def test_load_runner_from_yaml_fail_instantiate_class(self, mocker):
                     "module": "mymodule.runner",
                     "class": "FakeRunner",
                     "params": {"wrong_params": ["list", "of", "labels"]},
-                }
+                },
+                {},
             )