Added a lot of tests for SimpleConfig

Refactored the SImpleConfig class a lot to make sure the behavior is
always defined.

lib/simple_config.py

@@ -1,62 +1,95 @@
-import json
 import ast
 import threading
 import os
 
 from util import user_dir, print_error, print_msg
 
+SYSTEM_CONFIG_PATH = "/etc/electrum.conf"
 
 config = None
+
+
 def get_config():
     global config
     return config
 
+
 def set_config(c):
     global config
     config = c
 
 
-class SimpleConfig:
+class SimpleConfig(object):
     """
-The SimpleConfig class is responsible for handling operations involving
+    The SimpleConfig class is responsible for handling operations involving
-configuration files.  The constructor reads and stores the system and 
+    configuration files.
-user configurations from electrum.conf into separate dictionaries within
+
-a SimpleConfig instance then reads the wallet file.
+    There are 3 different sources of possible configuration values:
-"""
+        1. Command line options.
-    def __init__(self, options={}):
+        2. User configuration (in the user's config directory)
-        self.lock = threading.Lock()
+        3. System configuration (in /etc/)
-
+    They are taken in order (1. overrides config options set in 2., that
-        # system conf, readonly
+    override config set in 3.)
-        self.system_config = {}
+    """
+    def __init__(self, options=None, read_system_config_function=None,
+                 read_user_config_function=None, read_user_dir_function=None):
+
+        # This is the holder of actual options for the current user.
+        self.current_options = {}
+        # This lock needs to be acquired for updating and reading the config in
+        # a thread-safe way.
+        self.lock = threading.RLock()
+        # The path for the config directory. This is set later by init_path()
+        self.path = None
+
+        if options is None:
+            options = {}  # Having a mutable as a default value is a bad idea.
+
+        # The following two functions are there for dependency injection when
+        # testing.
+        if read_system_config_function is None:
+            read_system_config_function = read_system_config
+        if read_user_config_function is None:
+            read_user_config_function = read_user_config
+        if read_user_dir_function is None:
+            self.user_dir = user_dir
+        else:
+            self.user_dir = read_user_dir_function
+
+        # Save the command-line keys to make sure we don't override them.
+        self.command_line_keys = options.keys()
+        # Save the system config keys to make sure we don't override them.
+        self.system_config_keys = []
+
         if options.get('portable') is not True:
-            self.read_system_config()
+            # system conf
+            system_config = read_system_config_function()
+            self.system_config_keys = system_config.keys()
+            self.current_options.update(system_config)
 
-        # command-line options
+        # update the current options with the command line options last (to
-        self.options_config = options
+        # override both others).
+        self.current_options.update(options)
 
         # init path
         self.init_path()
 
-        # user conf, writeable
+        # user config.
-        self.user_config = {}
+        self.user_config = read_user_config_function(self.path)
-        self.read_user_config()
+        # The user config is overwritten by the current config!
-
+        self.user_config.update(self.current_options)
-        set_config(self)
+        self.current_options = self.user_config
-
 
+        set_config(self)  # Make a singleton instance of 'self'
 
     def init_path(self):
 
         # Read electrum path in the command line configuration
-        self.path = self.options_config.get('electrum_path')
+        self.path = self.current_options.get('electrum_path')
-
-        # Read electrum path in the system configuration
-        if self.path is None:
-            self.path = self.system_config.get('electrum_path')
 
         # If not set, use the user's default data directory.
         if self.path is None:
-            self.path = user_dir()
+            self.path = self.user_dir()
 
         # Make directory if it does not yet exist.
         if not os.path.exists(self.path):
@@ -64,110 +97,32 @@ a SimpleConfig instance then reads the wallet file.
 
         print_error( "electrum directory", self.path)
 
-        # portable wallet: use the same directory for wallet and headers file
-        #if options.get('portable'):
-        #    self.wallet_config['blockchain_headers_path'] = os.path.dirname(self.path)
-            
     def set_key(self, key, value, save = True):
-        # find where a setting comes from and save it there
+        if not self.is_modifiable(key):
-        if self.options_config.get(key) is not None:
+            print "Warning: not changing key '%s' because it is not modifiable" \
-            print "Warning: not changing '%s' because it was passed as a command-line option"%key
+                  " (passed as command line option or defined in /etc/electrum.conf)"%key
             return
 
-        elif self.system_config.get(key) is not None:
+        with self.lock:
-            if str(self.system_config[key]) != str(value):
+            self.user_config[key] = value
-                print "Warning: not changing '%s' because it was set in the system configuration"%key
+            self.current_options[key] = value
-
+            if save:
-        else:
+                self.save_user_config()
-
-            with self.lock:
-                self.user_config[key] = value
-                if save: 
-                    self.save_user_config()
-
 
+        return
 
     def get(self, key, default=None):
-
         out = None
-
+        with self.lock:
-        # 1. command-line options always override everything
+            out = self.current_options.get(key, default)
-        if self.options_config.has_key(key) and self.options_config.get(key) is not None:
-            out = self.options_config.get(key)
-
-        # 2. user configuration 
-        elif self.user_config.has_key(key):
-            out = self.user_config.get(key)
-
-        # 2. system configuration
-        elif self.system_config.has_key(key):
-            out = self.system_config.get(key)
-
-        if out is None and default is not None:
-            out = default
-
-        # try to fix the type
-        if default is not None and type(out) != type(default):
-            import ast
-            try:
-                out = ast.literal_eval(out)
-            except Exception:
-                print "type error for '%s': using default value"%key
-                out = default
-
         return out
 
-
     def is_modifiable(self, key):
-        """Check if the config file is modifiable."""
+        if key in self.command_line_keys:
-        if self.options_config.has_key(key):
             return False
-        elif self.user_config.has_key(key):
+        if key in self.system_config_keys:
-            return True
-        elif self.system_config.has_key(key):
             return False
-        else:
+        return True
-            return True
-
-
-    def read_system_config(self):
-        """Parse and store the system config settings in electrum.conf into system_config[]."""
-        name = '/etc/electrum.conf'
-        if os.path.exists(name):
-            try:
-                import ConfigParser
-            except ImportError:
-                print "cannot parse electrum.conf. please install ConfigParser"
-                return
-                
-            p = ConfigParser.ConfigParser()
-            p.read(name)
-            try:
-                for k, v in p.items('client'):
-                    self.system_config[k] = v
-            except ConfigParser.NoSectionError:
-                pass
-
-
-    def read_user_config(self):
-        """Parse and store the user config settings in electrum.conf into user_config[]."""
-        if not self.path: return
-
-        path = os.path.join(self.path, "config")
-        if os.path.exists(path):
-            try:
-                with open(path, "r") as f:
-                    data = f.read()
-            except IOError:
-                return
-            try:
-                d = ast.literal_eval( data )  #parse raw data from reading wallet file
-            except Exception:
-                print_msg("Error: Cannot read config file.")
-                return
-
-            self.user_config = d
-
 
     def save_user_config(self):
         if not self.path: return
@@ -180,3 +135,41 @@ a SimpleConfig instance then reads the wallet file.
         if self.get('gui') != 'android':
             import stat
             os.chmod(path, stat.S_IREAD | stat.S_IWRITE)
+
+def read_system_config():
+    """Parse and return the system config settings in /etc/electrum.conf."""
+    if os.path.exists(SYSTEM_CONFIG_PATH):
+        try:
+            import ConfigParser
+        except ImportError:
+            print "cannot parse electrum.conf. please install ConfigParser"
+            return
+
+        p = ConfigParser.ConfigParser()
+        p.read(SYSTEM_CONFIG_PATH)
+        result = {}
+        try:
+            for k, v in p.items('client'):
+                result[k] = v
+        except ConfigParser.NoSectionError:
+            pass
+        return result
+
+def read_user_config(path):
+    """Parse and store the user config settings in electrum.conf into user_config[]."""
+    if not path: return
+
+    config_path = os.path.join(path, "config")
+    if os.path.exists(config_path):
+        try:
+            with open(config_path, "r") as f:
+                data = f.read()
+        except IOError:
+            return
+        try:
+            d = ast.literal_eval( data )  #parse raw data from reading wallet file
+        except Exception:
+            print_msg("Error: Cannot read config file.")
+            return
+
+        return d

lib/tests/test_simple_config.py

@@ -0,0 +1,107 @@
+import sys
+import unittest
+import tempfile
+import shutil
+
+from StringIO import StringIO
+from lib.simple_config import SimpleConfig
+
+
+class Test_SimpleConfig(unittest.TestCase):
+
+    def setUp(self):
+        super(Test_SimpleConfig, self).setUp()
+        # make sure "read_user_config" and "user_dir" return a temporary directory.
+        self.electrum_dir = tempfile.mkdtemp()
+        # Do the same for the user dir to avoid overwriting the real configuration
+        # for development machines with electrum installed :)
+        self.user_dir = tempfile.mkdtemp()
+
+        self.options = {"electrum_path": self.electrum_dir}
+        self._saved_stdout = sys.stdout
+        self._stdout_buffer = StringIO()
+        sys.stdout = self._stdout_buffer
+
+    def tearDown(self):
+        super(Test_SimpleConfig, self).tearDown()
+        # Remove the temporary directory after each test (to make sure we don't
+        # pollute /tmp for nothing.
+        shutil.rmtree(self.electrum_dir)
+        shutil.rmtree(self.user_dir)
+
+        # Restore the "real" stdout
+        sys.stdout = self._saved_stdout
+
+    def test_simple_config_command_line_overrides_everything(self):
+        """Options passed by command line override all other configuration
+        sources"""
+        fake_read_system = lambda : {"electrum_path": "a"}
+        fake_read_user = lambda _: {"electrum_path": "b"}
+        read_user_dir = lambda : self.user_dir
+        config = SimpleConfig(options=self.options,
+                              read_system_config_function=fake_read_system,
+                              read_user_config_function=fake_read_user,
+                              read_user_dir_function=read_user_dir)
+        self.assertEqual(self.options.get("electrum_path"),
+                         config.get("electrum_path"))
+
+    def test_simple_config_system_config_overrides_user_config(self):
+        """Options passed in system config override user config."""
+        fake_read_system = lambda : {"electrum_path": self.electrum_dir}
+        fake_read_user = lambda _: {"electrum_path": "b"}
+        read_user_dir = lambda : self.user_dir
+        config = SimpleConfig(options=None,
+                              read_system_config_function=fake_read_system,
+                              read_user_config_function=fake_read_user,
+                              read_user_dir_function=read_user_dir)
+        self.assertEqual(self.options.get("electrum_path"),
+                         config.get("electrum_path"))
+
+    def test_simple_config_user_config_is_used_if_others_arent_specified(self):
+        """Options passed by command line override all other configuration
+        sources"""
+        fake_read_system = lambda : {}
+        fake_read_user = lambda _: {"electrum_path": self.electrum_dir}
+        read_user_dir = lambda : self.user_dir
+        config = SimpleConfig(options=None,
+                              read_system_config_function=fake_read_system,
+                              read_user_config_function=fake_read_user,
+                              read_user_dir_function=read_user_dir)
+        self.assertEqual(self.options.get("electrum_path"),
+                         config.get("electrum_path"))
+
+    def test_cannot_set_options_passed_by_command_line(self):
+        fake_read_system = lambda : {}
+        fake_read_user = lambda _: {"electrum_path": "b"}
+        read_user_dir = lambda : self.user_dir
+        config = SimpleConfig(options=self.options,
+                              read_system_config_function=fake_read_system,
+                              read_user_config_function=fake_read_user,
+                              read_user_dir_function=read_user_dir)
+        config.set_key("electrum_path", "c")
+        self.assertEqual(self.options.get("electrum_path"),
+                         config.get("electrum_path"))
+
+    def test_cannot_set_options_from_system_config(self):
+        fake_read_system = lambda : {"electrum_path": self.electrum_dir}
+        fake_read_user = lambda _: {}
+        read_user_dir = lambda : self.user_dir
+        config = SimpleConfig(options={},
+                              read_system_config_function=fake_read_system,
+                              read_user_config_function=fake_read_user,
+                              read_user_dir_function=read_user_dir)
+        config.set_key("electrum_path", "c")
+        self.assertEqual(self.options.get("electrum_path"),
+                         config.get("electrum_path"))
+
+    def test_can_set_options_set_in_user_config(self):
+        another_path = tempfile.mkdtemp()
+        fake_read_system = lambda : {}
+        fake_read_user = lambda _: {"electrum_path": self.electrum_dir}
+        read_user_dir = lambda : self.user_dir
+        config = SimpleConfig(options={},
+                              read_system_config_function=fake_read_system,
+                              read_user_config_function=fake_read_user,
+                              read_user_dir_function=read_user_dir)
+        config.set_key("electrum_path", another_path)
+        self.assertEqual(another_path, config.get("electrum_path"))