From 7e1ea7b360b6ac33b10a646fdea1d6b6558bf4fa Mon Sep 17 00:00:00 2001 From: Sebastiaan Huber Date: Wed, 11 Oct 2023 14:19:23 +0200 Subject: [PATCH 1/3] Tests: Print stack trace if CLI command excepts with `run_cli_command` Before, the test would just fail and say that an exception was raised but it would not display the actual exception, making it difficult to debug the problem. In the case of a non-zero exit code, the stderr is printed as well. --- tests/conftest.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index f4feb4ceff..69f172be56 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -536,8 +536,9 @@ def factory( assert result.exception is not None, result.output assert result.exit_code != 0, result.exit_code else: - assert result.exception is None, result.output - assert result.exit_code == 0, result.exit_code + import traceback + assert result.exception is None, ''.join(traceback.format_exception(*result.exc_info)) + assert result.exit_code == 0, (result.exit_code, result.stderr) return result From c4cea87d9e158b26f2ce3641473389af365bd6bb Mon Sep 17 00:00:00 2001 From: Sebastiaan Huber Date: Wed, 11 Oct 2023 14:37:26 +0200 Subject: [PATCH 2/3] Config: Remove use of `NO_DEFAULT` for `Option.default` The `Option.default` property would return the global constant `NO_DEFAULT` in case the option does not specify a default. The idea was that it could be used to distinguish between an option not defining a default and one defining the default to be `None`. The problem is that in the various methods that return a config option value, such as `Config.get_option` could also return this value. This would be problematic for certain CLI command options that used the `aiida.manage.configuration.get_config_option` function to set the default. If the config option was not defined, the function would return `()` which is the value of the `NO_DEFAULT` constant. When the option accepts string values, this value would often even silently be accepted although it almost certainly is not what the user intended. This would actually happen for the tests of `verdi setup`, which has the options `--email`, `--first-name`, `--last-name` and `--institution` that all define a default through `get_config_option` and therefore the default would be actually set to `()` in case the config did not specify these global config options. Since for config options there is no current use-case for actually setting a default to `None`, there is no need to distinguish between this case and a default never having been defined, and so the `NO_DEFAULT` global constant is removed and replaced by `None`. --- aiida/cmdline/commands/cmd_config.py | 3 +-- aiida/manage/configuration/config.py | 8 +++----- aiida/manage/configuration/options.py | 4 +--- 3 files changed, 5 insertions(+), 10 deletions(-) diff --git a/aiida/cmdline/commands/cmd_config.py b/aiida/cmdline/commands/cmd_config.py index 80ce61a15a..c788cff1d4 100644 --- a/aiida/cmdline/commands/cmd_config.py +++ b/aiida/cmdline/commands/cmd_config.py @@ -77,7 +77,6 @@ def _join(val): def verdi_config_show(ctx, option): """Show details of an AiiDA option for the current profile.""" from aiida.manage.configuration import Config, Profile - from aiida.manage.configuration.options import NO_DEFAULT config: Config = ctx.obj.config profile: Profile | None = ctx.obj.profile @@ -85,7 +84,7 @@ def verdi_config_show(ctx, option): dct = { 'schema': option.schema, 'values': { - 'default': '' if option.default is NO_DEFAULT else option.default, + 'default': '' if option.default is None else option.default, 'global': config.options.get(option.name, ''), } } diff --git a/aiida/manage/configuration/config.py b/aiida/manage/configuration/config.py index 103c4b1705..9db56fc43e 100644 --- a/aiida/manage/configuration/config.py +++ b/aiida/manage/configuration/config.py @@ -20,7 +20,7 @@ from aiida.common.exceptions import ConfigurationError from . import schema as schema_module -from .options import NO_DEFAULT, Option, get_option, get_option_names, parse_option +from .options import Option, get_option, get_option_names, parse_option from .profile import Profile __all__ = ('Config', 'config_schema', 'ConfigValidationError') @@ -407,7 +407,7 @@ def set_option(self, option_name, option_value, scope=None, override=True): if parsed_value is not None: value = parsed_value - elif option.default is not NO_DEFAULT: + elif option.default is not None: value = option.default else: return @@ -442,9 +442,7 @@ def get_option(self, option_name, scope=None, default=True): :return: the option value or None if not set for the given scope """ option = get_option(option_name) - - # Default value is `None` unless `default=True` and the `option.default` is not `NO_DEFAULT` - default_value = option.default if default and option.default is not NO_DEFAULT else None + default_value = option.default if default else None if scope is not None: value = self.get_profile(scope).get_option(option.name, default_value) diff --git a/aiida/manage/configuration/options.py b/aiida/manage/configuration/options.py index 9779642605..0240609e7b 100644 --- a/aiida/manage/configuration/options.py +++ b/aiida/manage/configuration/options.py @@ -14,8 +14,6 @@ __all__ = ('get_option', 'get_option_names', 'parse_option', 'Option') -NO_DEFAULT = () - class Option: """Represent a configuration option schema.""" @@ -41,7 +39,7 @@ def valid_type(self) -> Any: @property def default(self) -> Any: - return self._schema.get('default', NO_DEFAULT) + return self._schema.get('default', None) @property def description(self) -> str: From 60758c2c669653a19c6ae7cdcbe2761f44bcc127 Mon Sep 17 00:00:00 2001 From: Sebastiaan Huber Date: Wed, 11 Oct 2023 17:30:59 +0200 Subject: [PATCH 3/3] Tests: Fix failing `tests/cmdline/commands/test_setup.py` The previous commit fixed a bug in the evaluation of defaults for various options of the `verdi setup` command. Due to the bug, these options would set a default even if the corresponding config option was not defined. Instead of no default being defined, the empty tuple `()` would be set as string value. As soon as the bug was fixed, the `test_setup_profile_uuid` test started failing since it doesn't explicitly defined values for the options `--email`, `--first-name`, `--last-name` and `--institution`. --- tests/cmdline/commands/test_setup.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/tests/cmdline/commands/test_setup.py b/tests/cmdline/commands/test_setup.py index 68c1fcd9b8..4818a220ca 100644 --- a/tests/cmdline/commands/test_setup.py +++ b/tests/cmdline/commands/test_setup.py @@ -206,11 +206,15 @@ def test_setup_profile_uuid(self): profile_name = 'profile-copy' user_email = 'some@email.com' - profile_uuid = str(uuid.uuid4) + user_first_name = 'John' + user_last_name = 'Smith' + user_institution = 'ECMA' + profile_uuid = str(uuid.uuid4()) options = [ - '--non-interactive', '--profile', profile_name, '--profile-uuid', profile_uuid, '--db-backend', - self.storage_backend_name, '--db-name', db_name, '--db-username', db_user, '--db-password', db_pass, - '--db-port', self.pg_test.dsn['port'], '--email', user_email + '--non-interactive', '--email', user_email, '--first-name', user_first_name, '--last-name', user_last_name, + '--institution', user_institution, '--db-name', db_name, '--db-username', db_user, '--db-password', db_pass, + '--db-port', self.pg_test.dsn['port'], '--db-backend', self.storage_backend_name, '--profile', profile_name, + '--profile-uuid', profile_uuid ] self.cli_runner(cmd_setup.setup, options, use_subprocess=False) @@ -219,4 +223,4 @@ def test_setup_profile_uuid(self): assert profile_name in config.profile_names profile = config.get_profile(profile_name) - profile.uuid = profile_uuid + assert profile.uuid == profile_uuid