Cleanups and test coverage

10 files changed, 45 insertions, 26 deletions

diff --git a/mitmproxy/addons/replace.py b/mitmproxy/addons/replace.py
index 34bb40c2..0d0c3aa5 100644
--- a/mitmproxy/addons/replace.py
+++ b/mitmproxy/addons/replace.py
@@ -57,10 +57,7 @@ class _ReplaceBase:
         if self.optionName in updated:
             lst = []
             for rep in getattr(options, self.optionName):
-                if isinstance(rep, str):
-                    fpatt, rex, s = parse_hook(rep)
-                else:
-                    fpatt, rex, s = rep
+                fpatt, rex, s = parse_hook(rep)
 
                 flt = flowfilter.parse(fpatt)
                 if not flt:
diff --git a/mitmproxy/addons/setheaders.py b/mitmproxy/addons/setheaders.py
index 95cf9a09..9e60eabd 100644
--- a/mitmproxy/addons/setheaders.py
+++ b/mitmproxy/addons/setheaders.py
@@ -54,10 +54,7 @@ class SetHeaders:
         if "setheaders" in updated:
             self.lst = []
             for shead in options.setheaders:
-                if isinstance(shead, str):
-                    fpatt, header, value = parse_setheader(shead)
-                else:
-                    fpatt, header, value = shead
+                fpatt, header, value = parse_setheader(shead)
 
                 flt = flowfilter.parse(fpatt)
                 if not flt:
diff --git a/mitmproxy/optmanager.py b/mitmproxy/optmanager.py
index 319fe622..3768b52c 100644
--- a/mitmproxy/optmanager.py
+++ b/mitmproxy/optmanager.py
@@ -110,7 +110,7 @@ class OptManager:
         self._options[name] = _Option(name, default, typespec, help, choices)
 
     @contextlib.contextmanager
-    def rollback(self, updated):
+    def rollback(self, updated, reraise=False):
         old = copy.deepcopy(self._options)
         try:
             yield
@@ -120,6 +120,8 @@ class OptManager:
             # Rollback
             self.__dict__["_options"] = old
             self.changed.send(self, updated=updated)
+            if reraise:
+                raise e
 
     def subscribe(self, func, opts):
         """
@@ -337,7 +339,7 @@ class OptManager:
                 dest=option,
                 help=o.help
             )
-            parser.set_defaults(**{option: o.default})
+            parser.set_defaults(**{option: None})
         elif o.typespec in (int, typing.Optional[int]):
             parser.add_argument(
                 "--%s" % f,
diff --git a/mitmproxy/test/taddons.py b/mitmproxy/test/taddons.py
index c3e19cc7..8d6baa12 100644
--- a/mitmproxy/test/taddons.py
+++ b/mitmproxy/test/taddons.py
@@ -61,5 +61,6 @@ class context:
             Options object with the given keyword arguments, then calls the
             configure method on the addon with the updated value.
         """
-        self.options.update(**kwargs)
-        addon.configure(self.options, kwargs.keys())
+        with self.options.rollback(kwargs.keys(), reraise=True):
+            self.options.update(**kwargs)
+            addon.configure(self.options, kwargs.keys())
diff --git a/mitmproxy/tools/cmdline.py b/mitmproxy/tools/cmdline.py
index f3c3b6d0..13577997 100644
--- a/mitmproxy/tools/cmdline.py
+++ b/mitmproxy/tools/cmdline.py
@@ -8,10 +8,6 @@ from mitmproxy import version
 CONFIG_PATH = os.path.join(options.CA_DIR, "config.yaml")
 
 
-class ParseException(Exception):
-    pass
-
-
 def basic_options(parser, opts):
     parser.add_argument(
         '--version',
diff --git a/mitmproxy/tools/dump.py b/mitmproxy/tools/dump.py
index 6b862475..f64f2241 100644
--- a/mitmproxy/tools/dump.py
+++ b/mitmproxy/tools/dump.py
@@ -6,10 +6,6 @@ from mitmproxy import master
 from mitmproxy.addons import dumper, termlog
 
 
-class DumpError(Exception):
-    pass
-
-
 class DumpMaster(master.Master):
 
     def __init__(
@@ -38,7 +34,7 @@ class DumpMaster(master.Master):
                 self.load_flows_file(options.rfile)
             except exceptions.FlowReadException as v:
                 self.add_log("Flow file corrupted.", "error")
-                raise DumpError(v)
+                raise exceptions.OptionsError(v)
 
     @controller.handler
     def log(self, e):
diff --git a/mitmproxy/tools/main.py b/mitmproxy/tools/main.py
index a231c421..05877e2c 100644
--- a/mitmproxy/tools/main.py
+++ b/mitmproxy/tools/main.py
@@ -105,7 +105,7 @@ def mitmdump(args=None):  # pragma: no cover
 
         signal.signal(signal.SIGTERM, cleankill)
         master.run()
-    except (dump.DumpError, exceptions.OptionsError) as e:
+    except exceptions.OptionsError as e:
         print("mitmdump: %s" % e, file=sys.stderr)
         sys.exit(1)
     except (KeyboardInterrupt, RuntimeError):
diff --git a/test/mitmproxy/addons/test_core.py b/test/mitmproxy/addons/test_core.py
index 533eb58e..7b9e9614 100644
--- a/test/mitmproxy/addons/test_core.py
+++ b/test/mitmproxy/addons/test_core.py
@@ -13,6 +13,18 @@ def test_simple():
         tctx.configure(sa, body_size_limit = "1m")
         assert tctx.options._processed["body_size_limit"]
 
+        with pytest.raises(exceptions.OptionsError, match="mutually exclusive"):
+            tctx.configure(
+                sa,
+                add_upstream_certs_to_client_chain = True,
+                upstream_cert = False
+            )
+        with pytest.raises(exceptions.OptionsError, match="Invalid mode"):
+            tctx.configure(
+                sa,
+                mode = "Flibble"
+            )
+
 
 @mock.patch("mitmproxy.platform.original_addr", None)
 def test_no_transparent():
diff --git a/test/mitmproxy/test_optmanager.py b/test/mitmproxy/test_optmanager.py
index 1989cc0d..a38662d5 100644
--- a/test/mitmproxy/test_optmanager.py
+++ b/test/mitmproxy/test_optmanager.py
@@ -38,6 +38,12 @@ class TM(optmanager.OptManager):
         self.add_option("one", None, typing.Optional[str])
 
 
+def test_add_option():
+    o = TO()
+    with pytest.raises(ValueError, match="already exists"):
+        o.add_option("one", None, typing.Optional[int])
+
+
 def test_defaults():
     o = TD2()
     defaults = {
@@ -162,6 +168,8 @@ def test_rollback():
     def err(opts, updated):
         if opts.one == 10:
             raise exceptions.OptionsError()
+        if opts.bool is True:
+            raise exceptions.OptionsError()
 
     o.changed.connect(sub)
     o.changed.connect(err)
@@ -169,15 +177,24 @@ def test_rollback():
 
     assert o.one is None
     o.one = 10
+    o.bool = True
     assert isinstance(recerr[0]["exc"], exceptions.OptionsError)
     assert o.one is None
-    assert len(rec) == 2
+    assert o.bool is False
+    assert len(rec) == 4
     assert rec[0].one == 10
     assert rec[1].one is None
+    assert rec[2].bool is True
+    assert rec[3].bool is False
+
+    with pytest.raises(exceptions.OptionsError):
+        with o.rollback(set(["one"]), reraise=True):
+            raise exceptions.OptionsError()
 
 
-def test_repr():
+def test_simple():
     assert repr(TO())
+    assert "one" in TO()
 
 
 def test_serialize():
diff --git a/test/mitmproxy/tools/test_dump.py b/test/mitmproxy/tools/test_dump.py
index 3210b0bb..2542ec4b 100644
--- a/test/mitmproxy/tools/test_dump.py
+++ b/test/mitmproxy/tools/test_dump.py
@@ -3,6 +3,7 @@ import pytest
 from unittest import mock
 
 from mitmproxy import proxy
+from mitmproxy import exceptions
 from mitmproxy import log
 from mitmproxy import controller
 from mitmproxy import options
@@ -26,9 +27,9 @@ class TestDumpMaster(tservers.MasterTest):
                 self.mkmaster(None, rfile=p),
                 1, b"",
             )
-            with pytest.raises(dump.DumpError):
+            with pytest.raises(exceptions.OptionsError):
                 self.mkmaster(None, rfile="/nonexistent")
-            with pytest.raises(dump.DumpError):
+            with pytest.raises(exceptions.OptionsError):
                 self.mkmaster(None, rfile="test_dump.py")
 
     def test_has_error(self):