Merge pull request #736 from chrisglass/more-tests-2

Add tests to the SimpleConfig object (resubmit)

.travis.yml

@@ -2,4 +2,4 @@ language: python
 python:
     - "2.7"
 install: "pip install slowaes==0.1a1 ecdsa>=0.9 pbkdf2 requests pyasn1 pyasn1-modules tlslite>=0.4.5 qrcode"
-script: nosetests lib
+script: nosetests -e gui

electrum

@@ -103,16 +103,15 @@ def print_help_cb(self, opt, value, parser):
     print_help(parser)
 
 
-def run_command(cmd, password=None, args=[]):
-    import socket
+def run_command(cmd, password=None, args=None):
+    if args is None:
+        args = []  # Do not use mutables as default values!
     if cmd.requires_network and not options.offline:
         network = NetworkProxy(config)
         if not network.start(start_daemon= (True if cmd.name!='daemon' else False)):
             print "Daemon not running"
             sys.exit(1)
 
-
-
         if wallet:
             wallet.start_threads(network)
             wallet.update()

lib/blockchain.py

@@ -241,7 +241,9 @@ class Blockchain(threading.Thread):
                 return h 
 
 
-    def get_target(self, index, chain=[]):
+    def get_target(self, index, chain=None):
+        if chain is None:
+            chain = []  # Do not use mutables as default values!
 
         max_target = 0x00000000FFFF0000000000000000000000000000000000000000000000000000
         if index == 0: return 0x1d00ffff, max_target

lib/daemon.py

@@ -34,7 +34,9 @@ class NetworkProxy(threading.Thread):
     # connects to daemon
     # sends requests, runs callbacks
 
-    def __init__(self, config = {}):
+    def __init__(self, config=None):
+        if config is None:
+            config = {}  # Do not use mutables as default arguments!
         threading.Thread.__init__(self)
         self.daemon = True
         self.config = SimpleConfig(config) if type(config) == type({}) else config

lib/network.py

@@ -72,7 +72,9 @@ from simple_config import SimpleConfig
 
 class Network(threading.Thread):
 
-    def __init__(self, config = {}):
+    def __init__(self, config=None):
+        if config is None:
+            config = {}  # Do not use mutables as default values!
         threading.Thread.__init__(self)
         self.daemon = True
         self.config = SimpleConfig(config) if type(config) == type({}) else config

lib/simple_config.py

@@ -1,62 +1,94 @@
-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
-configuration files.  The constructor reads and stores the system and 
-user configurations from electrum.conf into separate dictionaries within
-a SimpleConfig instance then reads the wallet file.
-"""
-    def __init__(self, options={}):
-        self.lock = threading.Lock()
-
-        # system conf, readonly
-        self.system_config = {}
+    The SimpleConfig class is responsible for handling operations involving
+    configuration files.
+
+    There are 3 different sources of possible configuration values:
+        1. Command line options.
+        2. User configuration (in the user's config directory)
+        3. System configuration (in /etc/)
+    They are taken in order (1. overrides config options set in 2., that
+    override config set in 3.)
+    """
+    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
-        self.options_config = options
+        # update the current options with the command line options last (to
+        # override both others).
+        self.current_options.update(options)
 
         # init path
         self.init_path()
 
-        # user conf, writeable
-        self.user_config = {}
-        self.read_user_config()
-
-        set_config(self)
-
+        # user config.
+        self.user_config = read_user_config_function(self.path)
+        # The user config is overwritten by the current config!
+        self.user_config.update(self.current_options)
+        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')
-
-        # Read electrum path in the system configuration
-        if self.path is None:
-            self.path = self.system_config.get('electrum_path')
+        self.path = self.current_options.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 +96,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 self.options_config.get(key) is not None:
-            print "Warning: not changing '%s' because it was passed as a command-line option"%key
+        if not self.is_modifiable(key):
+            print "Warning: not changing key '%s' because it is not modifiable" \
+                  " (passed as command line option or defined in /etc/electrum.conf)"%key
             return
 
-        elif self.system_config.get(key) is not None:
-            if str(self.system_config[key]) != str(value):
-                print "Warning: not changing '%s' because it was set in the system configuration"%key
-
-        else:
-
-            with self.lock:
-                self.user_config[key] = value
-                if save: 
-                    self.save_user_config()
-
+        with self.lock:
+            self.user_config[key] = value
+            self.current_options[key] = value
+            if save:
+                self.save_user_config()
 
+        return
 
     def get(self, key, default=None):
-
         out = None
-
-        # 1. command-line options always override everything
-        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
-
+        with self.lock:
+            out = self.current_options.get(key, default)
         return out
 
-
     def is_modifiable(self, key):
-        """Check if the config file is modifiable."""
-        if self.options_config.has_key(key):
+        if key in self.command_line_keys:
             return False
-        elif self.user_config.has_key(key):
-            return True
-        elif self.system_config.has_key(key):
+        if key in self.system_config_keys:
             return False
-        else:
-            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
-
+        return True
 
     def save_user_config(self):
         if not self.path: return
@@ -180,3 +134,45 @@ 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(path=SYSTEM_CONFIG_PATH):
+    """Parse and return the system config settings in /etc/electrum.conf."""
+    result = {}
+    if os.path.exists(path):
+        try:
+            import ConfigParser
+        except ImportError:
+            print "cannot parse electrum.conf. please install ConfigParser"
+            return
+
+        p = ConfigParser.ConfigParser()
+        try:
+            p.read(path)
+            for k, v in p.items('client'):
+                result[k] = v
+        except (ConfigParser.NoSectionError, ConfigParser.MissingSectionHeaderError):
+            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 {}  # Return a dict, since we will call update() on it.
+
+    config_path = os.path.join(path, "config")
+    result = {}
+    if os.path.exists(config_path):
+        try:
+
+            with open(config_path, "r") as f:
+                data = f.read()
+            result = ast.literal_eval( data )  #parse raw data from reading wallet file
+
+        except Exception:
+            print_msg("Error: Cannot read config file.")
+            result = {}
+
+        if not type(result) is dict:
+            return {}
+
+    return result

lib/tests/test_simple_config.py

@@ -0,0 +1,222 @@
+import sys
+import os
+import unittest
+import tempfile
+import shutil
+
+from StringIO import StringIO
+from lib.simple_config import (SimpleConfig, read_system_config,
+                               read_user_config)
+
+
+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_system_config_ignored_if_portable(self):
+        """If electrum is started with the "portable" flag, system
+        configuration is completely ignored."""
+        another_path = tempfile.mkdtemp()
+        fake_read_system = lambda : {"electrum_path": self.electrum_dir}
+        fake_read_user = lambda _: {"electrum_path": another_path}
+        read_user_dir = lambda : self.user_dir
+        config = SimpleConfig(options={"portable": True},
+                              read_system_config_function=fake_read_system,
+                              read_user_config_function=fake_read_user,
+                              read_user_dir_function=read_user_dir)
+        self.assertEqual(another_path, config.get("electrum_path"))
+
+    def test_simple_config_user_config_is_used_if_others_arent_specified(self):
+        """If no system-wide configuration and no command-line options are
+        specified, the user configuration is used instead."""
+        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"))
+
+    def test_can_set_options_from_system_config_if_portable(self):
+        """If the "portable" flag is set, the user can overwrite system
+        configuration options."""
+        another_path = tempfile.mkdtemp()
+        fake_read_system = lambda : {"electrum_path": self.electrum_dir}
+        fake_read_user = lambda _: {}
+        read_user_dir = lambda : self.user_dir
+        config = SimpleConfig(options={"portable": True},
+                              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"))
+
+
+class TestSystemConfig(unittest.TestCase):
+
+    sample_conf = """
+[client]
+gap_limit = 5
+
+[something_else]
+everything = 42
+"""
+
+    def setUp(self):
+        super(TestSystemConfig, self).setUp()
+        self.thefile = tempfile.mkstemp(suffix=".electrum.test.conf")[1]
+
+    def tearDown(self):
+        super(TestSystemConfig, self).tearDown()
+        os.remove(self.thefile)
+
+    def test_read_system_config_file_does_not_exist(self):
+        somefile = "/foo/I/do/not/exist/electrum.conf"
+        result = read_system_config(somefile)
+        self.assertEqual({}, result)
+
+    def test_read_system_config_file_returns_file_options(self):
+        with open(self.thefile, "w") as f:
+            f.write(self.sample_conf)
+
+        result = read_system_config(self.thefile)
+        self.assertEqual({"gap_limit": "5"}, result)
+
+    def test_read_system_config_file_no_sections(self):
+
+        with open(self.thefile, "w") as f:
+            f.write("gap_limit = 5")  # The file has no sections at all
+
+        result = read_system_config(self.thefile)
+        self.assertEqual({}, result)
+
+
+class TestUserConfig(unittest.TestCase):
+
+    def setUp(self):
+        super(TestUserConfig, self).setUp()
+        self._saved_stdout = sys.stdout
+        self._stdout_buffer = StringIO()
+        sys.stdout = self._stdout_buffer
+
+        self.user_dir = tempfile.mkdtemp()
+
+    def tearDown(self):
+        super(TestUserConfig, self).tearDown()
+        shutil.rmtree(self.user_dir)
+        sys.stdout = self._saved_stdout
+
+    def test_no_path_means_no_result(self):
+       result = read_user_config(None)
+       self.assertEqual({}, result)
+
+    def test_path_with_reprd_dict(self):
+        thefile = os.path.join(self.user_dir, "config")
+        payload = {"gap_limit": 5}
+        with open(thefile, "w") as f:
+            f.write(repr(payload))
+
+        result = read_user_config(self.user_dir)
+        self.assertEqual(payload, result)
+
+    def test_path_without_config_file(self):
+        """We pass a path but if does not contain a "config" file."""
+        result = read_user_config(self.user_dir)
+        self.assertEqual({}, result)
+
+    def test_path_with_reprd_object(self):
+
+        class something(object):
+            pass
+
+        thefile = os.path.join(self.user_dir, "config")
+        payload = something()
+        with open(thefile, "w") as f:
+            f.write(repr(payload))
+
+        result = read_user_config(self.user_dir)
+        self.assertEqual({}, result)