From 67dddc299275731ceef0883d4b1b4a84927f84c2 Mon Sep 17 00:00:00 2001 From: Cleber Rosa Date: Thu, 22 Aug 2024 01:35:19 -0400 Subject: [PATCH 1/7] avocado/core/nrunner/runnable.py: fix typo Signed-off-by: Cleber Rosa --- avocado/core/nrunner/runnable.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/avocado/core/nrunner/runnable.py b/avocado/core/nrunner/runnable.py index 606330239e..876758c2cf 100644 --- a/avocado/core/nrunner/runnable.py +++ b/avocado/core/nrunner/runnable.py @@ -201,7 +201,7 @@ def config(self, config): LOG.warning( "The runnable config should have only values " "essential for its runner. In the next version of " - "avocado, this will raise a Value Error. Please " + "avocado, this will raise a ValueError. Please " "use avocado.core.nrunner.runnable.Runnable.filter_runnable_config " "or avocado.core.nrunner.runnable.Runnable.from_avocado_config" ) From bf8189bf2a66aa79632856cd95cc078589f6ae93 Mon Sep 17 00:00:00 2001 From: Cleber Rosa Date: Thu, 22 Aug 2024 01:35:19 -0400 Subject: [PATCH 2/7] Runnable: make list of used configuration more predictable The Runnable.get_configuration_used_by_kind() is the right place to return all configuration used by a runnable, and that includes the global configuration used by all runners. Signed-off-by: Cleber Rosa --- avocado/core/nrunner/runnable.py | 4 ++-- selftests/check.py | 2 +- selftests/unit/runnable.py | 14 ++++++++++++++ 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/avocado/core/nrunner/runnable.py b/avocado/core/nrunner/runnable.py index 876758c2cf..0d15692031 100644 --- a/avocado/core/nrunner/runnable.py +++ b/avocado/core/nrunner/runnable.py @@ -327,7 +327,7 @@ def get_configuration_used_by_kind(cls, kind): if command is not None: command = " ".join(command) configuration_used = STANDALONE_EXECUTABLE_CONFIG_USED.get(command) - return configuration_used + return configuration_used + CONFIGURATION_USED @classmethod def filter_runnable_config(cls, kind, config): @@ -349,7 +349,7 @@ def filter_runnable_config(cls, kind, config): """ whole_config = settings.as_dict() filtered_config = {} - config_items = cls.get_configuration_used_by_kind(kind) + CONFIGURATION_USED + config_items = cls.get_configuration_used_by_kind(kind) for config_item in config_items: filtered_config[config_item] = config.get( config_item, whole_config.get(config_item) diff --git a/selftests/check.py b/selftests/check.py index 52368ce2ca..76553aa74e 100755 --- a/selftests/check.py +++ b/selftests/check.py @@ -27,7 +27,7 @@ "job-api-7": 1, "nrunner-interface": 70, "nrunner-requirement": 28, - "unit": 672, + "unit": 673, "jobs": 11, "functional-parallel": 307, "functional-serial": 7, diff --git a/selftests/unit/runnable.py b/selftests/unit/runnable.py index 824ea0d333..1bab827e37 100644 --- a/selftests/unit/runnable.py +++ b/selftests/unit/runnable.py @@ -146,6 +146,20 @@ def test_config(self): runnable.config.get("runner.identifier_format"), "{uri}-{args[0]}" ) + def test_config_at_init(self): + runnable = Runnable("exec-test", "/bin/sh") + self.assertEqual( + set(runnable.config.keys()), + set( + [ + "run.keep_tmp", + "runner.exectest.exitcodes.skip", + "runner.exectest.clear_env", + "runner.identifier_format", + ] + ), + ) + def test_identifier(self): open_mocked = unittest.mock.mock_open( read_data=( From 90919507e19bc789fffc2bf5bf068a5ce270194f Mon Sep 17 00:00:00 2001 From: Cleber Rosa Date: Thu, 22 Aug 2024 01:35:19 -0400 Subject: [PATCH 3/7] Runnable: refactor the warning on unused config There's a warning that should be given to users passing configuration that is not used by runnables of a specific kind. This refactors that into its own method, and adds tests for it. Signed-off-by: Cleber Rosa --- avocado/core/nrunner/runnable.py | 21 ++++++++++++--------- selftests/check.py | 2 +- selftests/unit/runnable.py | 12 ++++++++++++ 3 files changed, 25 insertions(+), 10 deletions(-) diff --git a/avocado/core/nrunner/runnable.py b/avocado/core/nrunner/runnable.py index 0d15692031..af2c13a533 100644 --- a/avocado/core/nrunner/runnable.py +++ b/avocado/core/nrunner/runnable.py @@ -185,6 +185,17 @@ def identifier(self): def config(self): return self._config + def _config_setter_warning(self, config): + configuration_used = Runnable.get_configuration_used_by_kind(self.kind) + if not set(configuration_used).issubset(set(config.keys())): + LOG.warning( + "The runnable config should have only values " + "essential for its runner. In the next version of " + "avocado, this will raise a ValueError. Please " + "use avocado.core.nrunner.runnable.Runnable.filter_runnable_config " + "or avocado.core.nrunner.runnable.Runnable.from_avocado_config" + ) + @config.setter def config(self, config): """Sets the config values based on the runnable kind. @@ -196,15 +207,7 @@ def config(self, config): :param config: A config dict with new values for Runnable. :type config: dict """ - configuration_used = Runnable.get_configuration_used_by_kind(self.kind) - if not set(configuration_used).issubset(set(config.keys())): - LOG.warning( - "The runnable config should have only values " - "essential for its runner. In the next version of " - "avocado, this will raise a ValueError. Please " - "use avocado.core.nrunner.runnable.Runnable.filter_runnable_config " - "or avocado.core.nrunner.runnable.Runnable.from_avocado_config" - ) + self._config_setter_warning(config) self._config = config @classmethod diff --git a/selftests/check.py b/selftests/check.py index 76553aa74e..97d4a67582 100755 --- a/selftests/check.py +++ b/selftests/check.py @@ -27,7 +27,7 @@ "job-api-7": 1, "nrunner-interface": 70, "nrunner-requirement": 28, - "unit": 673, + "unit": 675, "jobs": 11, "functional-parallel": 307, "functional-serial": 7, diff --git a/selftests/unit/runnable.py b/selftests/unit/runnable.py index 1bab827e37..9452fe6571 100644 --- a/selftests/unit/runnable.py +++ b/selftests/unit/runnable.py @@ -1,5 +1,6 @@ import unittest.mock +import avocado.core.nrunner.runnable as runnable_mod from avocado.core.nrunner.runnable import Runnable @@ -160,6 +161,17 @@ def test_config_at_init(self): ), ) + def test_config_warn_if_not_used(self): + runnable = Runnable("exec-test", "/bin/sh") + with unittest.mock.patch.object(runnable_mod.LOG, "warning") as log_mock: + runnable.config = {"some-unused-config": "foo"} + log_mock.assert_called_once() + + def test_config_dont_warn_if_used(self): + with unittest.mock.patch.object(runnable_mod.LOG, "warning") as log_mock: + Runnable("noop", "noop", config={"runner.identifier_format": "noop"}) + log_mock.assert_not_called() + def test_identifier(self): open_mocked = unittest.mock.mock_open( read_data=( From b4a3631e37c5948202cbc58054a32aee1eb27c06 Mon Sep 17 00:00:00 2001 From: Cleber Rosa Date: Thu, 22 Aug 2024 01:35:19 -0400 Subject: [PATCH 4/7] Runnable: set filtered configuration in every instatiation The behavior of setting the filtered configuration should be applied at every single instantiation. Let's unify that at the class' __init__ method. Signed-off-by: Cleber Rosa --- avocado/core/nrunner/runnable.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/avocado/core/nrunner/runnable.py b/avocado/core/nrunner/runnable.py index af2c13a533..10393bf6ec 100644 --- a/avocado/core/nrunner/runnable.py +++ b/avocado/core/nrunner/runnable.py @@ -102,8 +102,8 @@ def __init__(self, kind, uri, *args, config=None, identifier=None, **kwargs): #: attr:`avocado.core.nrunner.runner.BaseRunner.CONFIGURATION_USED` self._config = {} if config is None: - config = self.filter_runnable_config(kind, {}) - self.config = config or {} + config = {} + self.config = self.filter_runnable_config(kind, config) self.args = args self.tags = kwargs.pop("tags", None) self.dependencies = self.read_dependencies(kwargs.pop("dependencies", None)) @@ -307,9 +307,6 @@ def from_avocado_config( cls, kind, uri, *args, config=None, identifier=None, **kwargs ): """Creates runnable with only essential config for runner of specific kind.""" - if not config: - config = {} - config = cls.filter_runnable_config(kind, config) return cls(kind, uri, *args, config=config, identifier=identifier, **kwargs) @classmethod From 990b052f6de4a27d2ae77c9c2bb723b1bddecc74 Mon Sep 17 00:00:00 2001 From: Cleber Rosa Date: Thu, 22 Aug 2024 10:11:26 -0400 Subject: [PATCH 5/7] Runnable: deprecate from_avocado_config() Since it provides the same signature as the main class initilization method. Internally, let's switch to it, while still allowing people that is creating their own runnables time to switch. Signed-off-by: Cleber Rosa --- avocado/core/nrunner/runnable.py | 13 +++++++++---- avocado/plugins/sysinfo.py | 6 +++--- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/avocado/core/nrunner/runnable.py b/avocado/core/nrunner/runnable.py index 10393bf6ec..8284d85ef2 100644 --- a/avocado/core/nrunner/runnable.py +++ b/avocado/core/nrunner/runnable.py @@ -5,6 +5,7 @@ import os import subprocess import sys +import warnings import pkg_resources @@ -192,8 +193,7 @@ def _config_setter_warning(self, config): "The runnable config should have only values " "essential for its runner. In the next version of " "avocado, this will raise a ValueError. Please " - "use avocado.core.nrunner.runnable.Runnable.filter_runnable_config " - "or avocado.core.nrunner.runnable.Runnable.from_avocado_config" + "use avocado.core.nrunner.runnable.Runnable.filter_runnable_config" ) @config.setter @@ -214,7 +214,7 @@ def config(self, config): def from_args(cls, args): """Returns a runnable from arguments""" decoded_args = [_arg_decode_base64(arg) for arg in args.get("arg", ())] - return cls.from_avocado_config( + return cls( args.get("kind"), args.get("uri"), *decoded_args, @@ -280,7 +280,7 @@ def from_dict(cls, recipe_dict): """ cls._validate_recipe(recipe_dict) config = ConfigDecoder.decode_set(recipe_dict.get("config", {})) - return cls.from_avocado_config( + return cls( recipe_dict.get("kind"), recipe_dict.get("uri"), *recipe_dict.get("args", ()), @@ -307,6 +307,11 @@ def from_avocado_config( cls, kind, uri, *args, config=None, identifier=None, **kwargs ): """Creates runnable with only essential config for runner of specific kind.""" + warnings.warn( + "from_avocado_config() is deprecated, please use the regular " + "class initialization as it has the same behavior.", + DeprecationWarning, + ) return cls(kind, uri, *args, config=config, identifier=identifier, **kwargs) @classmethod diff --git a/avocado/plugins/sysinfo.py b/avocado/plugins/sysinfo.py index 515d7e4469..a6ee797cfc 100644 --- a/avocado/plugins/sysinfo.py +++ b/avocado/plugins/sysinfo.py @@ -220,7 +220,7 @@ def pre_test_runnables(self, test_runnable, suite_config=None): return [] sysinfo_config = sysinfo.gather_collectibles_config(suite_config) return [ - Runnable.from_avocado_config( + Runnable( "sysinfo", "pre", config=suite_config, @@ -237,7 +237,7 @@ def post_test_runnables(self, test_runnable, suite_config=None): sysinfo_config = sysinfo.gather_collectibles_config(suite_config) return [ ( - Runnable.from_avocado_config( + Runnable( "sysinfo", "post", config=suite_config, @@ -249,7 +249,7 @@ def post_test_runnables(self, test_runnable, suite_config=None): ["pass"], ), ( - Runnable.from_avocado_config( + Runnable( "sysinfo", "post", config=suite_config, From e7d6f333887912a96cc39f4707ad853e2c8a1acd Mon Sep 17 00:00:00 2001 From: Cleber Rosa Date: Thu, 22 Aug 2024 01:35:19 -0400 Subject: [PATCH 6/7] Runnable: introduce default config Right now, there's a conflict of interest on the "config" attribute of a runnable: it serves as both a convenience holder of default values from Avocado's configuration (default values set by the settings API itself or in current configuration files), and the actual configuration defined in the runnable API. This introduces a different config holder, one with the default configuration known at the time the class is istantiated. At this point, it's a separate entity, but it will cooperate with config, so that the config attribute will provide either the value set in the runnable itself or in the default config. Signed-off-by: Cleber Rosa --- avocado/core/nrunner/runnable.py | 25 +++++++++++++++++++++++++ selftests/check.py | 2 +- selftests/unit/runnable.py | 13 +++++++++++++ 3 files changed, 39 insertions(+), 1 deletion(-) diff --git a/avocado/core/nrunner/runnable.py b/avocado/core/nrunner/runnable.py index 8284d85ef2..800675e78d 100644 --- a/avocado/core/nrunner/runnable.py +++ b/avocado/core/nrunner/runnable.py @@ -97,6 +97,10 @@ def __init__(self, kind, uri, *args, config=None, identifier=None, **kwargs): #: test or being the test, or an actual URI with multiple #: parts self.uri = uri + #: This attributes holds default configuration values that the + #: runner has determined that has interest in by setting it in + #: attr:`avocado.core.nrunner.runner.BaseRunner.CONFIGURATION_USED` + self._default_config = self.filter_runnable_config(kind, {}) #: This attributes holds configuration from Avocado proper #: that is passed to runners, as long as a runner declares #: its interest in using them with @@ -186,6 +190,10 @@ def identifier(self): def config(self): return self._config + @property + def default_config(self): + return self._default_config + def _config_setter_warning(self, config): configuration_used = Runnable.get_configuration_used_by_kind(self.kind) if not set(configuration_used).issubset(set(config.keys())): @@ -210,6 +218,23 @@ def config(self, config): self._config_setter_warning(config) self._config = config + @default_config.setter + def default_config(self, config): + """Sets the default config values based on the runnable kind. + + This is not avocado config, it is a runnable config which is a subset + of avocado config based on `STANDALONE_EXECUTABLE_CONFIG_USED` which + describes essential configuration values for each runner kind. + + These values are used as convenience if other values are not set + in the actual :attr:`config` itself. + + :param config: A config dict with default values for this Runnable. + :type config: dict + """ + self._config_setter_warning(config) + self._default_config = config + @classmethod def from_args(cls, args): """Returns a runnable from arguments""" diff --git a/selftests/check.py b/selftests/check.py index 97d4a67582..e88b0ac898 100755 --- a/selftests/check.py +++ b/selftests/check.py @@ -27,7 +27,7 @@ "job-api-7": 1, "nrunner-interface": 70, "nrunner-requirement": 28, - "unit": 675, + "unit": 677, "jobs": 11, "functional-parallel": 307, "functional-serial": 7, diff --git a/selftests/unit/runnable.py b/selftests/unit/runnable.py index 9452fe6571..303dce5354 100644 --- a/selftests/unit/runnable.py +++ b/selftests/unit/runnable.py @@ -172,6 +172,19 @@ def test_config_dont_warn_if_used(self): Runnable("noop", "noop", config={"runner.identifier_format": "noop"}) log_mock.assert_not_called() + def test_default_config(self): + runnable = Runnable("noop", "noop") + self.assertEqual( + runnable.default_config.get("runner.identifier_format"), "{uri}" + ) + + def test_default_and_actual_config(self): + runnable = Runnable("noop", "noop", config={"runner.identifier_format": "noop"}) + self.assertEqual(runnable.config.get("runner.identifier_format"), "noop") + self.assertEqual( + runnable.default_config.get("runner.identifier_format"), "{uri}" + ) + def test_identifier(self): open_mocked = unittest.mock.mock_open( read_data=( From 7608215109c7baaefe51f9872af473b48fc32067 Mon Sep 17 00:00:00 2001 From: Cleber Rosa Date: Thu, 22 Aug 2024 01:35:19 -0400 Subject: [PATCH 7/7] Runnable and Suite configuration support This change allows for the a runnable configuration and the suite and default configuration to coexist with the correct behavior. In short, if the suite has a configuration, it will become the new default configuration for a runnable, while its own configuration will not be touched. Because the purpose of the default_config and config are different, the checks for the values they may contain are different. The default_config should contain the exact keys as in the used config. The config itself, on the other hand, may contain nothing, or some values, but it should never be more than a subset of the config. Note: there's an opportunity here for changing the data structure used for the "config" attribute, one that looks up the value in the "default_config" if it doesn't exist in the actual "config". But, given that the size of the "default_config" will always be very small (given that it's filtered to contain only the used configuration by the runner) it seemed an unnecessary optimization at this time. Fixes: https://github.com/avocado-framework/avocado/issues/5998 Signed-off-by: Cleber Rosa --- avocado/core/nrunner/runnable.py | 35 +++++++++++-------- avocado/core/suite.py | 4 ++- .../nrunner/recipes/runnable/noop_config.json | 1 + selftests/check.py | 2 +- selftests/functional/resolver.py | 2 +- selftests/unit/suite.py | 11 ++++++ 6 files changed, 38 insertions(+), 17 deletions(-) create mode 100644 examples/nrunner/recipes/runnable/noop_config.json diff --git a/avocado/core/nrunner/runnable.py b/avocado/core/nrunner/runnable.py index 800675e78d..7669f5835b 100644 --- a/avocado/core/nrunner/runnable.py +++ b/avocado/core/nrunner/runnable.py @@ -1,5 +1,6 @@ import base64 import collections +import copy import json import logging import os @@ -105,10 +106,7 @@ def __init__(self, kind, uri, *args, config=None, identifier=None, **kwargs): #: that is passed to runners, as long as a runner declares #: its interest in using them with #: attr:`avocado.core.nrunner.runner.BaseRunner.CONFIGURATION_USED` - self._config = {} - if config is None: - config = {} - self.config = self.filter_runnable_config(kind, config) + self.config = config or {} self.args = args self.tags = kwargs.pop("tags", None) self.dependencies = self.read_dependencies(kwargs.pop("dependencies", None)) @@ -188,21 +186,30 @@ def identifier(self): @property def config(self): - return self._config + if not self._config: + return self._default_config + config_with_defaults = copy.copy(self._default_config) + config_with_defaults.update(self._config) + return config_with_defaults @property def default_config(self): return self._default_config - def _config_setter_warning(self, config): + def _config_setter_warning(self, config, default_config=False): configuration_used = Runnable.get_configuration_used_by_kind(self.kind) - if not set(configuration_used).issubset(set(config.keys())): - LOG.warning( - "The runnable config should have only values " - "essential for its runner. In the next version of " - "avocado, this will raise a ValueError. Please " - "use avocado.core.nrunner.runnable.Runnable.filter_runnable_config" - ) + if default_config: + if set(configuration_used) == (set(config.keys())): + return + else: + if set(config.keys()).issubset(set(configuration_used)): + return + LOG.warning( + "The runnable config should have only values " + "essential for its runner. In the next version of " + "avocado, this will raise a ValueError. Please " + "use avocado.core.nrunner.runnable.Runnable.filter_runnable_config" + ) @config.setter def config(self, config): @@ -232,7 +239,7 @@ def default_config(self, config): :param config: A config dict with default values for this Runnable. :type config: dict """ - self._config_setter_warning(config) + self._config_setter_warning(config, True) self._default_config = config @classmethod diff --git a/avocado/core/suite.py b/avocado/core/suite.py index 88949f526d..286c532406 100644 --- a/avocado/core/suite.py +++ b/avocado/core/suite.py @@ -76,7 +76,9 @@ def resolutions_to_runnables(resolutions, config): if resolution.result != ReferenceResolutionResult.SUCCESS: continue for runnable in resolution.resolutions: - runnable.config = runnable.filter_runnable_config(runnable.kind, config) + runnable.default_config = runnable.filter_runnable_config( + runnable.kind, config + ) result.append(runnable) return result diff --git a/examples/nrunner/recipes/runnable/noop_config.json b/examples/nrunner/recipes/runnable/noop_config.json new file mode 100644 index 0000000000..009060233a --- /dev/null +++ b/examples/nrunner/recipes/runnable/noop_config.json @@ -0,0 +1 @@ +{"kind": "noop", "uri": "noop", "config": {"runner.identifier_format": "nothing-op"}} diff --git a/selftests/check.py b/selftests/check.py index e88b0ac898..ea81932dfa 100755 --- a/selftests/check.py +++ b/selftests/check.py @@ -27,7 +27,7 @@ "job-api-7": 1, "nrunner-interface": 70, "nrunner-requirement": 28, - "unit": 677, + "unit": 678, "jobs": 11, "functional-parallel": 307, "functional-serial": 7, diff --git a/selftests/functional/resolver.py b/selftests/functional/resolver.py index 6b0eea5ee8..f9b64449db 100644 --- a/selftests/functional/resolver.py +++ b/selftests/functional/resolver.py @@ -183,7 +183,7 @@ def test_runnables_recipe(self): ================== asset: 1 exec-test: 3 -noop: 2 +noop: 3 package: 1 python-unittest: 1 sysinfo: 1""" diff --git a/selftests/unit/suite.py b/selftests/unit/suite.py index 51c967e8b9..af91128439 100644 --- a/selftests/unit/suite.py +++ b/selftests/unit/suite.py @@ -85,6 +85,17 @@ def test_config_runnable(self): runnable = suite.tests[0] self.assertEqual(runnable.config.get("runner.identifier_format"), "NOT FOO") + def test_config_runnable_and_suite(self): + config = { + "resolver.references": [ + "examples/nrunner/recipes/runnable/noop_config.json", + ], + "runner.identifier_format": "NOT FOO", + } + suite = TestSuite.from_config(config) + runnable = suite.tests[0] + self.assertEqual(runnable.config.get("runner.identifier_format"), "nothing-op") + def tearDown(self): self.tmpdir.cleanup()