Merge pull request #166 from espressif/fix/unset_cli_args_overwrite_c…

…onfig_files_with_default_value

fix: unset CLI argument wrongly overwrite config file settings with default value

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,

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)

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'