Merge pull request #679 from boriel/bugfix/crash_on_new_config_file

fix: prevents crash on config load/save errors

Author: boriel

src/api/config.py

@@ -93,11 +93,16 @@ class OPTION(str, Enum):
 }
 
 
-def load_config_from_file(filename: str, section: str, options_: options.Options = None, stop_on_error=True) -> bool:
+def load_config_from_file(
+    filename: str, section: ConfigSections, options_: options.Options | None = None, *, stop_on_error: bool = True
+) -> bool:
     """Opens file and read options from the given section. If stop_on_error is set,
-    the program stop if any error is found. Otherwise the result of the operation will be
+    the program stop if any error is found. Otherwise, the result of the operation will be
     returned (True on success, False on failure)
     """
+    assert isinstance(section, ConfigSections)
+    section_ = section.value
+
     if options_ is None:
         options_ = OPTIONS
 
@@ -115,25 +120,30 @@ def load_config_from_file(filename: str, section: str, options_: options.Options
             sys.exit(1)
         return False
 
-    if section not in cfg.sections():
-        errmsg.msg_output(f"Section '{section}' not found in config file '{filename}'")
+    if section_ not in cfg.sections():
+        errmsg.msg_output(f"Section '{section_}' not found in config file '{filename}'")
         if stop_on_error:
             sys.exit(1)
         return False
 
     parsing: Dict[type, Callable] = {int: cfg.getint, float: cfg.getfloat, bool: cfg.getboolean}
 
-    for opt in cfg.options(section):
+    for opt in cfg.options(section_):
         options_[opt].value = parsing.get(options_[opt].type, cfg.get)(section=section, option=opt)
 
     return True
 
 
-def save_config_into_file(filename: str, section: str, options_: options.Options = None, stop_on_error=True) -> bool:
+def save_config_into_file(
+    filename: str, section: ConfigSections, options_: options.Options | None = None, *, stop_on_error: bool = True
+) -> bool:
     """Save config into config ini file into the given section. If stop_on_error is set,
-    the program stop. Otherwise the result of the operation will be
+    the program stop. Otherwise, the result of the operation will be
     returned (True on success, False on failure)
     """
+    assert isinstance(section, ConfigSections)
+    section_ = section.value
+
     if options_ is None:
         options_ = OPTIONS
 
@@ -147,16 +157,16 @@ def save_config_into_file(filename: str, section: str, options_: options.Options
                 sys.exit(1)
             return False
 
-    cfg[section] = {}
+    cfg[section_] = {}
     for opt_name, opt in options_().items():
         if opt_name.startswith("__") or opt.value is None or opt_name in OPTIONS_NOT_SAVED:
             continue
 
         if opt.type == bool:
-            cfg[section][opt_name] = str(opt.value).lower()
+            cfg[section_][opt_name] = str(opt.value).lower()
             continue
 
-        cfg[section][opt_name] = str(opt.value)
+        cfg[section_][opt_name] = str(opt.value)
 
     try:
         with open(filename, "wt", encoding="utf-8") as f:
@@ -170,7 +180,7 @@ def save_config_into_file(filename: str, section: str, options_: options.Options
     return True
 
 
-def init():
+def init() -> None:
     """Default Options and Compilation Flags"""
     OPTIONS(Action.CLEAR)
 

src/zxbc/args_config.py

@@ -7,15 +7,15 @@
 import src.api.config
 import src.api.global_ as gl
 from src import arch
-from src.api import debug, errmsg
+from src.api import errmsg
 from src.api.config import OPTIONS
 from src.api.utils import open_file
 from src.zxbc import args_parser
 
 if TYPE_CHECKING:
     from argparse import Namespace
 
-__all__ = ["FileType", "parse_options", "set_option_defines"]
+__all__ = "FileType", "parse_options", "set_option_defines"
 
 
 class FileType:
@@ -101,7 +101,6 @@ def parse_options(args: list[str] | None = None) -> Namespace:
         OPTIONS.case_insensitive = True
 
     OPTIONS.case_insensitive = options.ignore_case
-    debug.ENABLED = OPTIONS.debug_level > 0
 
     if options.basic and not options.tzx and not options.tap:
         parser.error("Option --BASIC and --autorun requires --tzx or tap format")

tests/api/test_config.py

@@ -1,8 +1,10 @@
 #!/usr/bin/env python
 # -*- coding: utf-8 -*-
-
 import sys
 import unittest
+from unittest import mock
+
+import pytest
 
 from src.api import config, global_
 
@@ -100,3 +102,49 @@ def test_autorun_ignore_none(self):
         config.OPTIONS.autorun = True
         config.OPTIONS.autorun = None
         self.assertEqual(config.OPTIONS.autorun, True)
+
+    @mock.patch("os.path.exists", lambda x: False)
+    def test_save_config_for_the_1st_time(self):
+        m = mock.mock_open()
+        with mock.patch("builtins.open", m):
+            config.save_config_into_file("dummy_filename", config.ConfigSections.ZXBC)
+
+        m().write.assert_has_calls([mock.call("[zxbc]\n")], any_order=True)
+
+    def test_save_config_does_not_accept_invalid_section(self):
+        with pytest.raises(AssertionError):
+            config.save_config_into_file("dummy_filename", "invalid_section")
+
+    def test_load_config_from_file(self):
+        m = mock.mock_open(read_data="[zxbc]\nheap_size=1234\n")
+        config.OPTIONS(config.Action.ADD_IF_NOT_DEFINED, name="heap_size", type=int, default=4768, ignore_none=True)
+
+        with mock.patch("builtins.open", m):
+            config.load_config_from_file("dummy_filename", config.ConfigSections.ZXBC)
+
+        self.assertEqual(config.OPTIONS.heap_size, 1234)
+
+    def test_load_config_from_file_fails_if_no_section(self):
+        m = mock.mock_open(read_data="[zxbasm]\norg=1234\n")
+
+        with mock.patch("builtins.open", m):
+            result = config.load_config_from_file("dummy_filename", config.ConfigSections.ZXBC, stop_on_error=False)
+
+        self.assertFalse(result)
+
+    @mock.patch("os.path.exists", lambda x: False)
+    def test_load_config_from_file_fails_if_no_file(self):
+        result = config.load_config_from_file("dummy_filename", config.ConfigSections.ZXBC, stop_on_error=False)
+        self.assertFalse(result)
+
+    def test_load_config_from_file_fails_if_duplicated_fields(self):
+        m = mock.mock_open(read_data="[zxbc]\nheap_size=1234\nheap_size=5678\n")
+
+        with mock.patch("builtins.open", m):
+            result = config.load_config_from_file("dummy_filename", config.ConfigSections.ZXBC, stop_on_error=False)
+
+        self.assertFalse(result)
+
+    def test_load_config_does_not_accept_invalid_section(self):
+        with pytest.raises(AssertionError):
+            config.load_config_from_file("dummy_filename", "invalid_section", stop_on_error=False)