From 87f3004329ffc3e494fc518ba78a847c85b8f3ea Mon Sep 17 00:00:00 2001 From: Fu Hanxi Date: Fri, 27 Sep 2024 12:12:50 +0200 Subject: [PATCH] fix: unset CLI argument wrongly overwrite config file settings with default value --- idf_build_apps/args.py | 16 ++++--- tests/conftest.py | 3 +- tests/test_args.py | 97 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 110 insertions(+), 6 deletions(-) diff --git a/idf_build_apps/args.py b/idf_build_apps/args.py index b4eb80f..565a02e 100644 --- a/idf_build_apps/args.py +++ b/idf_build_apps/args.py @@ -141,8 +141,9 @@ def validate_by_validate_methods(cls, v: t.Any, info: ValidationInfo): if info.field_name and info.field_name in cls.model_fields: f = cls.model_fields[info.field_name] meta = get_meta(f) - if meta and meta.validate_method and ValidateMethod.TO_LIST in meta.validate_method: - return to_list(v) + if meta and meta.validate_method: + if ValidateMethod.TO_LIST in meta.validate_method: + return to_list(v) return v @@ -269,7 +270,7 @@ class DependencyDrivenBuildArguments(GlobalArguments): ) compare_manifest_sha_filepath: t.Optional[str] = field( None, - description='Path to the file containing the sha256 hash of the manifest rules. ' + description='Path to the file containing the hash of the manifest rules. ' 'Compare the hash with the current manifest rules. ' 'All matched apps will be built if the corresponding manifest rule is modified', default=None, @@ -800,14 +801,19 @@ def add_args_to_parser(argument_cls: t.Type[BaseArguments], parser: argparse.Arg kwargs['required'] = True if f_meta.action: kwargs['action'] = f_meta.action + # to make the CLI override config file work + if f_meta.action == 'store_true': + kwargs['default'] = None + if f_meta.nargs: kwargs['nargs'] = f_meta.nargs if f_meta.choices: kwargs['choices'] = f_meta.choices if f_meta.default: kwargs['default'] = f_meta.default - if 'default' not in kwargs: - kwargs['default'] = f.default + + # here in CLI arguments, don't set the default to field.default + # otherwise it will override the config file settings parser.add_argument( *names, diff --git a/tests/conftest.py b/tests/conftest.py index a92d068..4dd0a09 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -10,13 +10,14 @@ setup_logging, ) from idf_build_apps.args import apply_config_file -from idf_build_apps.constants import IDF_BUILD_APPS_TOML_FN +from idf_build_apps.constants import IDF_BUILD_APPS_TOML_FN, SUPPORTED_TARGETS from idf_build_apps.manifest.manifest import FolderRule @pytest.fixture(autouse=True) def clean_cls_attr(tmp_path): App.MANIFEST = None + FolderRule.DEFAULT_BUILD_TARGETS = SUPPORTED_TARGETS idf_build_apps.SESSION_ARGS.clean() apply_config_file(IDF_BUILD_APPS_TOML_FN) os.chdir(tmp_path) diff --git a/tests/test_args.py b/tests/test_args.py index 39c942a..aa32245 100644 --- a/tests/test_args.py +++ b/tests/test_args.py @@ -1,7 +1,11 @@ # SPDX-FileCopyrightText: 2024 Espressif Systems (Shanghai) CO LTD # SPDX-License-Identifier: Apache-2.0 +from xml.etree import ElementTree import pytest +from conftest import ( + create_project, +) from idf_build_apps import App from idf_build_apps.args import ( @@ -197,3 +201,96 @@ def test_ignore_extra_fields(self): args = FindArguments() assert not hasattr(args, 'dry_run') + + def test_config_file(self, tmp_path, monkeypatch): + create_project('foo', tmp_path) + + with open(IDF_BUILD_APPS_TOML_FN, 'w') as fw: + fw.write("""paths = ["foo"] +target = "esp32" +build_dir = "build_@t" +junitxml = "test.xml" +keep_going = true +""") + + # test basic config + with monkeypatch.context() as m: + m.setenv('PATH', 'foo') # let build fail + m.setattr('sys.argv', ['idf-build-apps', 'build']) + with pytest.raises(SystemExit): + main() + + with open('test.xml') as f: + xml = ElementTree.fromstring(f.read()) + test_suite = xml.findall('testsuite')[0] + assert test_suite.attrib['failures'] == '1' + assert test_suite.attrib['errors'] == '0' + assert test_suite.attrib['skipped'] == '0' + assert test_suite.findall('testcase')[0].attrib['name'] == 'foo/build_esp32' + + # test cli overrides config + with monkeypatch.context() as m: + m.setenv('PATH', 'foo') # let build fail + m.setattr('sys.argv', ['idf-build-apps', 'build', '--build-dir', 'build_hi_@t']) + with pytest.raises(SystemExit): + main() + + with open('test.xml') as f: + xml = ElementTree.fromstring(f.read()) + test_suite = xml.findall('testsuite')[0] + assert test_suite.attrib['failures'] == '1' + assert test_suite.attrib['errors'] == '0' + assert test_suite.attrib['skipped'] == '0' + assert test_suite.findall('testcase')[0].attrib['name'] == 'foo/build_hi_esp32' + + # test cli action_true + with monkeypatch.context() as m: + m.setattr('sys.argv', ['idf-build-apps', 'build', '--dry-run']) + main() + + with open('test.xml') as f: + xml = ElementTree.fromstring(f.read()) + test_suite = xml.findall('testsuite')[0] + assert test_suite.attrib['failures'] == '0' + assert test_suite.attrib['errors'] == '0' + assert test_suite.attrib['skipped'] == '1' + assert test_suite.findall('testcase')[0].attrib['name'] == 'foo/build_esp32' + + # test config store_true set to true + with open(IDF_BUILD_APPS_TOML_FN, 'a') as fw: + fw.write('\ndry_run = true\n') + + with monkeypatch.context() as m: + m.setattr('sys.argv', ['idf-build-apps', 'build']) + main() + + with open('test.xml') as f: + xml = ElementTree.fromstring(f.read()) + test_suite = xml.findall('testsuite')[0] + assert test_suite.attrib['failures'] == '0' + assert test_suite.attrib['errors'] == '0' + assert test_suite.attrib['skipped'] == '1' + assert test_suite.findall('testcase')[0].attrib['name'] == 'foo/build_esp32' + + # test config store_true set to false, but CLI set to true + with open(IDF_BUILD_APPS_TOML_FN, 'w') as fw: + fw.write("""paths = ["foo"] +build_dir = "build_@t" +junitxml = "test.xml" +dry_run = false +""") + + with monkeypatch.context() as m: + m.setattr( + 'sys.argv', ['idf-build-apps', 'build', '--default-build-targets', 'esp32', 'esp32s2', '--dry-run'] + ) + main() + + with open('test.xml') as f: + xml = ElementTree.fromstring(f.read()) + test_suite = xml.findall('testsuite')[0] + assert test_suite.attrib['failures'] == '0' + assert test_suite.attrib['errors'] == '0' + assert test_suite.attrib['skipped'] == '2' + assert test_suite.findall('testcase')[0].attrib['name'] == 'foo/build_esp32' + assert test_suite.findall('testcase')[1].attrib['name'] == 'foo/build_esp32s2'