Improved Primitive support and better testing

This changeset addresses several issues.

- Improve primitive support so the status and output of an executed
primitive can be retrieved
- Merge latest upstream libjuju (required for new primive features)
- New testing framework
    This is the start of a new testing framework with the ability to
create and configure LXD containers with SSH, to use while testing proxy
charms.
- Add support for using ssh keys with proxy charms
    See Feature 1429. This uses the per-proxy charm/unit ssh keypair

Signed-off-by: Adam Israel <adam.israel@canonical.com>
diff --git a/modules/libjuju/tests/base.py b/modules/libjuju/tests/base.py
index bae4b80..97eea53 100644
--- a/modules/libjuju/tests/base.py
+++ b/modules/libjuju/tests/base.py
@@ -1,6 +1,8 @@
 import inspect
 import subprocess
 import uuid
+from contextlib import contextmanager
+from pathlib import Path
 
 import mock
 from juju.client.jujudata import FileJujuData
@@ -10,10 +12,14 @@
 
 
 def is_bootstrapped():
-    result = subprocess.run(['juju', 'switch'], stdout=subprocess.PIPE)
-    return (
-        result.returncode == 0 and
-        len(result.stdout.decode().strip()) > 0)
+    try:
+        result = subprocess.run(['juju', 'switch'], stdout=subprocess.PIPE)
+        return (
+            result.returncode == 0 and
+            len(result.stdout.decode().strip()) > 0)
+    except FileNotFoundError:
+        return False
+
 
 
 bootstrapped = pytest.mark.skipif(
@@ -24,6 +30,13 @@
 
 
 class CleanController():
+    """
+    Context manager that automatically connects and disconnects from
+    the currently active controller.
+
+    Note: Unlike CleanModel, this will not create a new controller for you,
+    and an active controller must already be available.
+    """
     def __init__(self):
         self._controller = None
 
@@ -37,6 +50,14 @@
 
 
 class CleanModel():
+    """
+    Context manager that automatically connects to the currently active
+    controller, adds a fresh model, returns the connection to that model,
+    and automatically disconnects and cleans up the model.
+
+    The new model is also set as the current default for the controller
+    connection.
+    """
     def __init__(self, bakery_client=None):
         self._controller = None
         self._model = None
@@ -77,14 +98,6 @@
 
         return self._model
 
-    def _models(self):
-        result = self._orig_models()
-        models = result[self.controller_name]['models']
-        full_model_name = '{}/{}'.format(self.user_name, self.model_name)
-        if full_model_name not in models:
-            models[full_model_name] = {'uuid': self.model_uuid}
-        return result
-
     async def __aexit__(self, exc_type, exc, tb):
         await self._model.disconnect()
         await self._controller.destroy_model(self._model_uuid)
@@ -115,9 +128,22 @@
         cmodels = all_models[self.__controller_name]['models']
         cmodels[self.__model_name] = {'uuid': self.__model_uuid}
         return all_models
->>>>>>> New N2VC interface + updated libjuju
 
 
 class AsyncMock(mock.MagicMock):
     async def __call__(self, *args, **kwargs):
         return super().__call__(*args, **kwargs)
+
+
+@contextmanager
+def patch_file(filename):
+    """
+    "Patch" a file so that its current contents are automatically restored
+    when the context is exited.
+    """
+    filepath = Path(filename).expanduser()
+    data = filepath.read_bytes()
+    try:
+        yield
+    finally:
+        filepath.write_bytes(data)
diff --git a/modules/libjuju/tests/bundle/invalid.yaml b/modules/libjuju/tests/bundle/invalid.yaml
new file mode 100644
index 0000000..2e51c1e
--- /dev/null
+++ b/modules/libjuju/tests/bundle/invalid.yaml
@@ -0,0 +1,7 @@
+applications:
+  myapp:
+    charm: cs:xenial/ubuntu-0
+    num_units: 1
+    to:
+      - 0
+      - 0
diff --git a/modules/libjuju/tests/bundle/mini-bundle.yaml b/modules/libjuju/tests/bundle/mini-bundle.yaml
new file mode 100644
index 0000000..e351a12
--- /dev/null
+++ b/modules/libjuju/tests/bundle/mini-bundle.yaml
@@ -0,0 +1,3 @@
+applications:
+  myapp:
+    charm: cs:xenial/ubuntu-0
diff --git a/modules/libjuju/tests/integration/test_application.py b/modules/libjuju/tests/integration/test_application.py
index 7b780da..b705832 100644
--- a/modules/libjuju/tests/integration/test_application.py
+++ b/modules/libjuju/tests/integration/test_application.py
@@ -1,4 +1,5 @@
 import asyncio
+
 import pytest
 
 from .. import base
@@ -11,9 +12,9 @@
 async def test_action(event_loop):
     async with base.CleanModel() as model:
         ubuntu_app = await model.deploy(
-            'mysql',
+            'percona-cluster',
             application_name='mysql',
-            series='trusty',
+            series='xenial',
             channel='stable',
             config={
                 'tuning-level': 'safest',
@@ -28,11 +29,20 @@
         config = await ubuntu_app.get_config()
         assert config['tuning-level']['value'] == 'fast'
 
+        # Restore config back to default
+        await ubuntu_app.reset_config(['tuning-level'])
+        config = await ubuntu_app.get_config()
+        assert config['tuning-level']['value'] == 'safest'
+
         # update and check app constraints
         await ubuntu_app.set_constraints({'mem': 512 * MB})
         constraints = await ubuntu_app.get_constraints()
         assert constraints['mem'] == 512 * MB
 
+        # check action definitions
+        actions = await ubuntu_app.get_actions()
+        assert 'backup' in actions.keys()
+
 
 @base.bootstrapped
 @pytest.mark.asyncio
diff --git a/modules/libjuju/tests/integration/test_controller.py b/modules/libjuju/tests/integration/test_controller.py
index 9c6f7ac..93e2883 100644
--- a/modules/libjuju/tests/integration/test_controller.py
+++ b/modules/libjuju/tests/integration/test_controller.py
@@ -1,8 +1,10 @@
 import asyncio
-import pytest
+import subprocess
 import uuid
 
 from juju.client.connection import Connection
+from juju.client.jujudata import FileJujuData
+from juju.controller import Controller
 from juju.errors import JujuAPIError
 
 import pytest
@@ -168,3 +170,31 @@
         await asyncio.wait_for(_wait_for_model_gone(controller,
                                                     model_name),
                                timeout=60)
+
+
+# this test must be run serially because it modifies the login password
+@pytest.mark.serial
+@base.bootstrapped
+@pytest.mark.asyncio
+async def test_macaroon_auth(event_loop):
+    jujudata = FileJujuData()
+    account = jujudata.accounts()[jujudata.current_controller()]
+    with base.patch_file('~/.local/share/juju/accounts.yaml'):
+        if 'password' in account:
+            # force macaroon auth by "changing" password to current password
+            result = subprocess.run(
+                ['juju', 'change-user-password'],
+                input='{0}\n{0}\n'.format(account['password']),
+                universal_newlines=True,
+                stderr=subprocess.PIPE)
+            assert result.returncode == 0, ('Failed to change password: '
+                                            '{}'.format(result.stderr))
+        controller = Controller()
+        try:
+            await controller.connect()
+            assert controller.is_connected()
+        finally:
+            if controller.is_connected():
+                await controller.disconnect()
+        async with base.CleanModel():
+            pass  # create and login to model works
diff --git a/modules/libjuju/tests/integration/test_macaroon_auth.py b/modules/libjuju/tests/integration/test_macaroon_auth.py
new file mode 100644
index 0000000..9911c41
--- /dev/null
+++ b/modules/libjuju/tests/integration/test_macaroon_auth.py
@@ -0,0 +1,108 @@
+import logging
+import os
+
+import macaroonbakery.bakery as bakery
+import macaroonbakery.httpbakery as httpbakery
+import macaroonbakery.httpbakery.agent as agent
+from juju.errors import JujuAPIError
+from juju.model import Model
+
+import pytest
+
+from .. import base
+
+log = logging.getLogger(__name__)
+
+
+@base.bootstrapped
+@pytest.mark.asyncio
+@pytest.mark.xfail
+async def test_macaroon_auth(event_loop):
+    auth_info, username = agent_auth_info()
+    # Create a bakery client that can do agent authentication.
+    client = httpbakery.Client(
+        key=auth_info.key,
+        interaction_methods=[agent.AgentInteractor(auth_info)],
+    )
+
+    async with base.CleanModel(bakery_client=client) as m:
+        async with await m.get_controller() as c:
+            await c.grant_model(username, m.info.uuid, 'admin')
+        async with Model(
+            jujudata=NoAccountsJujuData(m._connector.jujudata),
+            bakery_client=client,
+        ):
+            pass
+
+
+@base.bootstrapped
+@pytest.mark.asyncio
+@pytest.mark.xfail
+async def test_macaroon_auth_with_bad_key(event_loop):
+    auth_info, username = agent_auth_info()
+    # Use a random key rather than the correct key.
+    auth_info = auth_info._replace(key=bakery.generate_key())
+    # Create a bakery client can do agent authentication.
+    client = httpbakery.Client(
+        key=auth_info.key,
+        interaction_methods=[agent.AgentInteractor(auth_info)],
+    )
+
+    async with base.CleanModel(bakery_client=client) as m:
+        async with await m.get_controller() as c:
+            await c.grant_model(username, m.info.uuid, 'admin')
+        try:
+            async with Model(
+                jujudata=NoAccountsJujuData(m._connector.jujudata),
+                bakery_client=client,
+            ):
+                pytest.fail('Should not be able to connect with invalid key')
+        except httpbakery.BakeryException:
+            # We're expecting this because we're using the
+            # wrong key.
+            pass
+
+
+@base.bootstrapped
+@pytest.mark.asyncio
+async def test_macaroon_auth_with_unauthorized_user(event_loop):
+    auth_info, username = agent_auth_info()
+    # Create a bakery client can do agent authentication.
+    client = httpbakery.Client(
+        key=auth_info.key,
+        interaction_methods=[agent.AgentInteractor(auth_info)],
+    )
+    async with base.CleanModel(bakery_client=client) as m:
+        # Note: no grant of rights to the agent user.
+        try:
+            async with Model(
+                jujudata=NoAccountsJujuData(m._connector.jujudata),
+                bakery_client=client,
+            ):
+                pytest.fail('Should not be able to connect without grant')
+        except (JujuAPIError, httpbakery.DischargeError):
+            # We're expecting this because we're using the
+            # wrong user name.
+            pass
+
+
+def agent_auth_info():
+    agent_data = os.environ.get('TEST_AGENTS')
+    if agent_data is None:
+        pytest.skip('skipping macaroon_auth because no TEST_AGENTS '
+                    'environment variable is set')
+    auth_info = agent.read_auth_info(agent_data)
+    if len(auth_info.agents) != 1:
+        raise Exception('TEST_AGENTS agent data requires exactly one agent')
+    return auth_info, auth_info.agents[0].username
+
+
+class NoAccountsJujuData:
+    def __init__(self, jujudata):
+        self.__jujudata = jujudata
+
+    def __getattr__(self, name):
+        return getattr(self.__jujudata, name)
+
+    def accounts(self):
+        return {}
diff --git a/modules/libjuju/tests/integration/test_machine.py b/modules/libjuju/tests/integration/test_machine.py
index 8957ae1..9a5f075 100644
--- a/modules/libjuju/tests/integration/test_machine.py
+++ b/modules/libjuju/tests/integration/test_machine.py
@@ -26,18 +26,15 @@
         assert machine.agent_status == 'pending'
         assert not machine.agent_version
 
+        # there is some inconsistency in the capitalization of status_message
+        # between different providers
         await asyncio.wait_for(
-            model.block_until(lambda: (machine.status == 'running' and
-                                       machine.agent_status == 'started' and
-                                       machine.agent_version is not None)),
+            model.block_until(
+                lambda: (machine.status == 'running' and
+                         machine.status_message.lower() == 'running' and
+                         machine.agent_status == 'started')),
             timeout=480)
 
-        assert machine.status == 'running'
-        # there is some inconsistency in the message case between providers
-        assert machine.status_message.lower() == 'running'
-        assert machine.agent_status == 'started'
-        assert machine.agent_version.major >= 2
-
 
 @base.bootstrapped
 @pytest.mark.asyncio
diff --git a/modules/libjuju/tests/integration/test_model.py b/modules/libjuju/tests/integration/test_model.py
index ba2da92..1cba79a 100644
--- a/modules/libjuju/tests/integration/test_model.py
+++ b/modules/libjuju/tests/integration/test_model.py
@@ -6,6 +6,12 @@
 from juju.client.client import ConfigValue, ApplicationFacade
 from juju.model import Model, ModelObserver
 from juju.utils import block_until, run_with_interrupt
+from juju.errors import JujuError
+
+import os
+import pylxd
+import time
+import uuid
 
 import pytest
 
@@ -20,7 +26,6 @@
 @base.bootstrapped
 @pytest.mark.asyncio
 async def test_deploy_local_bundle(event_loop):
-    from pathlib import Path
     tests_dir = Path(__file__).absolute().parent.parent
     bundle_path = tests_dir / 'bundle'
     mini_bundle_file_path = bundle_path / 'mini-bundle.yaml'
@@ -35,6 +40,16 @@
 
 @base.bootstrapped
 @pytest.mark.asyncio
+async def test_deploy_invalid_bundle(event_loop):
+    tests_dir = Path(__file__).absolute().parent.parent
+    bundle_path = tests_dir / 'bundle' / 'invalid.yaml'
+    async with base.CleanModel() as model:
+        with pytest.raises(JujuError):
+            await model.deploy(str(bundle_path))
+
+
+@base.bootstrapped
+@pytest.mark.asyncio
 async def test_deploy_local_charm(event_loop):
     from pathlib import Path
     tests_dir = Path(__file__).absolute().parent.parent
@@ -112,6 +127,114 @@
 
 @base.bootstrapped
 @pytest.mark.asyncio
+async def test_add_manual_machine_ssh(event_loop):
+
+    # Verify controller is localhost
+    async with base.CleanController() as controller:
+        cloud = await controller.get_cloud()
+        if cloud != "localhost":
+            pytest.skip('Skipping because test requires lxd.')
+
+    async with base.CleanModel() as model:
+        private_key_path = os.path.expanduser(
+            "~/.local/share/juju/ssh/juju_id_rsa"
+        )
+        public_key_path = os.path.expanduser(
+            "~/.local/share/juju/ssh/juju_id_rsa.pub"
+        )
+
+        # Use the self-signed cert generated by lxc on first run
+        crt = os.path.expanduser('~/snap/lxd/current/.config/lxc/client.crt')
+        assert os.path.exists(crt)
+
+        key = os.path.expanduser('~/snap/lxd/current/.config/lxc/client.key')
+        assert os.path.exists(key)
+
+        client = pylxd.Client(
+            endpoint="https://127.0.0.1:8443",
+            cert=(crt, key),
+            verify=False,
+        )
+
+        test_name = "test-{}-add-manual-machine-ssh".format(
+            uuid.uuid4().hex[-4:]
+        )
+
+        # create profile w/cloud-init and juju ssh key
+        public_key = ""
+        with open(public_key_path, "r") as f:
+            public_key = f.readline()
+
+        profile = client.profiles.create(
+            test_name,
+            config={'user.user-data': '#cloud-config\nssh_authorized_keys:\n- {}'.format(public_key)},
+            devices={
+                'root': {'path': '/', 'pool': 'default', 'type': 'disk'},
+                'eth0': {
+                    'nictype': 'bridged',
+                    'parent': 'lxdbr0',
+                    'type': 'nic'
+                }
+            }
+        )
+
+        # create lxc machine
+        config = {
+            'name': test_name,
+            'source': {
+                'type': 'image',
+                'alias': 'xenial',
+                'mode': 'pull',
+                'protocol': 'simplestreams',
+                'server': 'https://cloud-images.ubuntu.com/releases',
+            },
+            'profiles': [test_name],
+        }
+        container = client.containers.create(config, wait=True)
+        container.start(wait=True)
+
+        def wait_for_network(container, timeout=30):
+            """Wait for eth0 to have an ipv4 address."""
+            starttime = time.time()
+            while(time.time() < starttime + timeout):
+                time.sleep(1)
+                if 'eth0' in container.state().network:
+                    addresses = container.state().network['eth0']['addresses']
+                    if len(addresses) > 0:
+                        if addresses[0]['family'] == 'inet':
+                            return addresses[0]
+            return None
+
+        host = wait_for_network(container)
+
+        # HACK: We need to give sshd a chance to bind to the interface,
+        # and pylxd's container.execute seems to be broken and fails and/or
+        # hangs trying to properly check if the service is up.
+        time.sleep(5)
+
+        if host:
+            # add a new manual machine
+            machine1 = await model.add_machine(spec='ssh:{}@{}:{}'.format(
+                "ubuntu",
+                host['address'],
+                private_key_path,
+            ))
+
+            assert len(model.machines) == 1
+
+            res = await machine1.destroy(force=True)
+
+            assert res is None
+            assert len(model.machines) == 0
+
+        container.stop(wait=True)
+        container.delete(wait=True)
+
+        profile.delete()
+
+
+@base.bootstrapped
+@pytest.mark.asyncio
 async def test_relate(event_loop):
     from juju.relation import Relation
 
@@ -269,6 +392,14 @@
         assert result['extra-info'].source == 'model'
         assert result['extra-info'].value == 'booyah'
 
+@base.bootstrapped
+@pytest.mark.asyncio
+async def test_set_constraints(event_loop):
+    async with base.CleanModel() as model:
+        await model.set_constraints({'cpu-power': 1})
+        cons = await model.get_constraints()
+        assert cons['cpu_power'] == 1
+
 # @base.bootstrapped
 # @pytest.mark.asyncio
 # async def test_grant(event_loop)
diff --git a/modules/libjuju/tests/integration/test_unit.py b/modules/libjuju/tests/integration/test_unit.py
index 8b2251c..bb34969 100644
--- a/modules/libjuju/tests/integration/test_unit.py
+++ b/modules/libjuju/tests/integration/test_unit.py
@@ -25,6 +25,18 @@
             assert 'Stdout' in action.results
             break
 
+        for unit in app.units:
+            action = await unit.run('sleep 1', timeout=0.5)
+            assert isinstance(action, Action)
+            assert action.status == 'failed'
+            break
+
+        for unit in app.units:
+            action = await unit.run('sleep 0.5', timeout=2)
+            assert isinstance(action, Action)
+            assert action.status == 'completed'
+            break
+
 
 @base.bootstrapped
 @pytest.mark.asyncio
@@ -46,6 +58,10 @@
         for unit in app.units:
             action = await run_action(unit)
             assert action.results == {'dir': '/var/git/myrepo.git'}
+            out = await model.get_action_output(action.entity_id, wait=5)
+            assert out == {'dir': '/var/git/myrepo.git'}
+            status = await model.get_action_status(uuid_or_prefix=action.entity_id)
+            assert status[action.entity_id] == 'completed'
             break
 
 
diff --git a/modules/libjuju/tests/unit/test_client.py b/modules/libjuju/tests/unit/test_client.py
index 42134df..1d18bf9 100644
--- a/modules/libjuju/tests/unit/test_client.py
+++ b/modules/libjuju/tests/unit/test_client.py
@@ -4,7 +4,6 @@
 """
 
 import mock
-
 from juju.client import client
 
 
diff --git a/modules/libjuju/tests/unit/test_constraints.py b/modules/libjuju/tests/unit/test_constraints.py
index 00b9156..3c52090 100644
--- a/modules/libjuju/tests/unit/test_constraints.py
+++ b/modules/libjuju/tests/unit/test_constraints.py
@@ -32,6 +32,12 @@
         self.assertEqual(_("10G"), 10 * 1024)
         self.assertEqual(_("10M"), 10)
         self.assertEqual(_("10"), 10)
+        self.assertEqual(_("foo,bar"), "foo,bar")
+
+    def test_normalize_list_val(self):
+        _ = constraints.normalize_list_value
+
+        self.assertEqual(_("foo"), ["foo"])
         self.assertEqual(_("foo,bar"), ["foo", "bar"])
 
     def test_parse_constraints(self):
@@ -43,6 +49,9 @@
         )
 
         self.assertEqual(
-            _("mem=10G foo=bar,baz"),
-            {"mem": 10 * 1024, "foo": ["bar", "baz"]}
+            _("mem=10G foo=bar,baz tags=tag1 spaces=space1,space2"),
+            {"mem": 10 * 1024,
+             "foo": "bar,baz",
+             "tags": ["tag1"],
+             "spaces": ["space1", "space2"]}
         )
diff --git a/modules/libjuju/tests/unit/test_controller.py b/modules/libjuju/tests/unit/test_controller.py
new file mode 100644
index 0000000..44f488f
--- /dev/null
+++ b/modules/libjuju/tests/unit/test_controller.py
@@ -0,0 +1,110 @@
+import asynctest
+import mock
+from pathlib import Path
+from tempfile import NamedTemporaryFile
+
+from juju.controller import Controller
+from juju.client import client
+
+from .. import base
+
+
+class TestControllerConnect(asynctest.TestCase):
+    @asynctest.patch('juju.client.connector.Connector.connect_controller')
+    async def test_no_args(self, mock_connect_controller):
+        c = Controller()
+        await c.connect()
+        mock_connect_controller.assert_called_once_with(None)
+
+    @asynctest.patch('juju.client.connector.Connector.connect_controller')
+    async def test_with_controller_name(self, mock_connect_controller):
+        c = Controller()
+        await c.connect(controller_name='foo')
+        mock_connect_controller.assert_called_once_with('foo')
+
+    @asynctest.patch('juju.client.connector.Connector.connect')
+    async def test_with_endpoint_and_no_auth(self, mock_connect):
+        c = Controller()
+        with self.assertRaises(TypeError):
+            await c.connect(endpoint='0.1.2.3:4566')
+        self.assertEqual(mock_connect.call_count, 0)
+
+    @asynctest.patch('juju.client.connector.Connector.connect')
+    async def test_with_endpoint_and_userpass(self, mock_connect):
+        c = Controller()
+        with self.assertRaises(TypeError):
+            await c.connect(endpoint='0.1.2.3:4566', username='dummy')
+        await c.connect(endpoint='0.1.2.3:4566',
+                        username='user',
+                        password='pass')
+        mock_connect.assert_called_once_with(endpoint='0.1.2.3:4566',
+                                             username='user',
+                                             password='pass')
+
+    @asynctest.patch('juju.client.connector.Connector.connect')
+    async def test_with_endpoint_and_bakery_client(self, mock_connect):
+        c = Controller()
+        await c.connect(endpoint='0.1.2.3:4566', bakery_client='bakery')
+        mock_connect.assert_called_once_with(endpoint='0.1.2.3:4566',
+                                             bakery_client='bakery')
+
+    @asynctest.patch('juju.client.connector.Connector.connect')
+    async def test_with_endpoint_and_macaroons(self, mock_connect):
+        c = Controller()
+        await c.connect(endpoint='0.1.2.3:4566',
+                        macaroons=['macaroon'])
+        mock_connect.assert_called_with(endpoint='0.1.2.3:4566',
+                                        macaroons=['macaroon'])
+        await c.connect(endpoint='0.1.2.3:4566',
+                        bakery_client='bakery',
+                        macaroons=['macaroon'])
+        mock_connect.assert_called_with(endpoint='0.1.2.3:4566',
+                                        bakery_client='bakery',
+                                        macaroons=['macaroon'])
+
+    @asynctest.patch('juju.client.connector.Connector.connect_controller')
+    @asynctest.patch('juju.client.connector.Connector.connect')
+    async def test_with_posargs(self, mock_connect, mock_connect_controller):
+        c = Controller()
+        await c.connect('foo')
+        mock_connect_controller.assert_called_once_with('foo')
+        with self.assertRaises(TypeError):
+            await c.connect('endpoint', 'user')
+        await c.connect('endpoint', 'user', 'pass')
+        mock_connect.assert_called_once_with(endpoint='endpoint',
+                                             username='user',
+                                             password='pass')
+        await c.connect('endpoint', 'user', 'pass', 'cacert', 'bakery',
+                        'macaroons', 'loop', 'max_frame_size')
+        mock_connect.assert_called_with(endpoint='endpoint',
+                                        username='user',
+                                        password='pass',
+                                        cacert='cacert',
+                                        bakery_client='bakery',
+                                        macaroons='macaroons',
+                                        loop='loop',
+                                        max_frame_size='max_frame_size')
+
+    @asynctest.patch('juju.client.client.CloudFacade')
+    async def test_file_cred(self, mock_cf):
+        with NamedTemporaryFile() as tempfile:
+            tempfile.close()
+            temppath = Path(tempfile.name)
+            temppath.write_text('cred-test')
+            cred = client.CloudCredential(auth_type='jsonfile',
+                                          attrs={'file': tempfile.name})
+            jujudata = mock.MagicMock()
+            c = Controller(jujudata=jujudata)
+            c._connector = base.AsyncMock()
+            up_creds = base.AsyncMock()
+            mock_cf.from_connection().UpdateCredentials = up_creds
+            await c.add_credential(
+                name='name',
+                credential=cred,
+                cloud='cloud',
+                owner='owner',
+            )
+            assert up_creds.called
+            new_cred = up_creds.call_args[0][0][0].credential
+            assert cred.attrs['file'] == tempfile.name
+            assert new_cred.attrs['file'] == 'cred-test'
diff --git a/modules/libjuju/tests/unit/test_gocookies.py b/modules/libjuju/tests/unit/test_gocookies.py
new file mode 100644
index 0000000..033a0e9
--- /dev/null
+++ b/modules/libjuju/tests/unit/test_gocookies.py
@@ -0,0 +1,244 @@
+"""
+Tests for the gocookies code.
+"""
+import os
+import shutil
+import tempfile
+import unittest
+import urllib.request
+
+import pyrfc3339
+from juju.client.gocookies import GoCookieJar
+
+# cookie_content holds the JSON contents of a Go-produced
+# cookie file (reformatted so it's not all on one line but
+# otherwise unchanged).
+cookie_content = """
+[
+    {
+        "CanonicalHost": "bar.com",
+        "Creation": "2017-11-17T08:53:55.088820092Z",
+        "Domain": "bar.com",
+        "Expires": "2345-11-15T18:16:08Z",
+        "HostOnly": true,
+        "HttpOnly": false,
+        "LastAccess": "2017-11-17T08:53:55.088822562Z",
+        "Name": "bar",
+        "Path": "/",
+        "Persistent": true,
+        "Secure": false,
+        "Updated": "2017-11-17T08:53:55.088822562Z",
+        "Value": "bar-value"
+    },
+    {
+        "CanonicalHost": "x.foo.com",
+        "Creation": "2017-11-17T08:53:55.088814857Z",
+        "Domain": "x.foo.com",
+        "Expires": "2345-11-15T18:16:05Z",
+        "HostOnly": true,
+        "HttpOnly": false,
+        "LastAccess": "2017-11-17T08:53:55.088884015Z",
+        "Name": "foo",
+        "Path": "/path",
+        "Persistent": true,
+        "Secure": false,
+        "Updated": "2017-11-17T08:53:55.088814857Z",
+        "Value": "foo-path-value"
+    },
+    {
+        "CanonicalHost": "x.foo.com",
+        "Creation": "2017-11-17T08:53:55.088814857Z",
+        "Domain": "foo.com",
+        "Expires": "2345-11-15T18:16:06Z",
+        "HostOnly": false,
+        "HttpOnly": false,
+        "LastAccess": "2017-11-17T08:53:55.088919437Z",
+        "Name": "foo4",
+        "Path": "/path",
+        "Persistent": true,
+        "Secure": false,
+        "Updated": "2017-11-17T08:53:55.088814857Z",
+        "Value": "foo4-value"
+    },
+    {
+        "CanonicalHost": "x.foo.com",
+        "Creation": "2017-11-17T08:53:55.088790709Z",
+        "Domain": "x.foo.com",
+        "Expires": "2345-11-15T18:16:01Z",
+        "HostOnly": true,
+        "HttpOnly": false,
+        "LastAccess": "2017-11-17T08:53:55.088884015Z",
+        "Name": "foo",
+        "Path": "/",
+        "Persistent": true,
+        "Secure": false,
+        "Updated": "2017-11-17T08:53:55.088790709Z",
+        "Value": "foo-value"
+    },
+    {
+        "CanonicalHost": "x.foo.com",
+        "Creation": "2017-11-17T08:53:55.088790709Z",
+        "Domain": "foo.com",
+        "Expires": "2345-11-15T18:16:02Z",
+        "HostOnly": false,
+        "HttpOnly": false,
+        "LastAccess": "2017-11-17T08:53:55.088919437Z",
+        "Name": "foo1",
+        "Path": "/",
+        "Persistent": true,
+        "Secure": false,
+        "Updated": "2017-11-17T08:53:55.088790709Z",
+        "Value": "foo1-value"
+    },
+    {
+        "CanonicalHost": "x.foo.com",
+        "Creation": "2017-11-17T08:53:55.088790709Z",
+        "Domain": "x.foo.com",
+        "Expires": "2345-11-15T18:16:03Z",
+        "HostOnly": true,
+        "HttpOnly": false,
+        "LastAccess": "2017-11-17T08:53:55.088850252Z",
+        "Name": "foo2",
+        "Path": "/",
+        "Persistent": true,
+        "Secure": true,
+        "Updated": "2017-11-17T08:53:55.088790709Z",
+        "Value": "foo2-value"
+    },
+    {
+        "CanonicalHost": "x.foo.com",
+        "Creation": "2017-11-17T08:53:55.088790709Z",
+        "Domain": "foo.com",
+        "Expires": "2345-11-15T18:16:04Z",
+        "HostOnly": false,
+        "HttpOnly": false,
+        "LastAccess": "2017-11-17T08:53:55.088919437Z",
+        "Name": "foo3",
+        "Path": "/",
+        "Persistent": true,
+        "Secure": false,
+        "Updated": "2017-11-17T08:53:55.088790709Z",
+        "Value": "foo3-value"
+    }
+]
+"""
+
+# cookie_content_queries holds a set of queries
+# that were automatically generated by running
+# the queries on the above cookie_content data
+# and printing the results.
+cookie_content_queries = [
+    ('http://x.foo.com', [
+        ('foo', 'foo-value'),
+        ('foo1', 'foo1-value'),
+        ('foo3', 'foo3-value'),
+    ]),
+    ('https://x.foo.com', [
+        ('foo', 'foo-value'),
+        ('foo1', 'foo1-value'),
+        ('foo2', 'foo2-value'),
+        ('foo3', 'foo3-value'),
+    ]),
+    ('http://arble.foo.com', [
+        ('foo1', 'foo1-value'),
+        ('foo3', 'foo3-value'),
+    ]),
+    ('http://arble.com', [
+    ]),
+    ('http://x.foo.com/path/x', [
+        ('foo', 'foo-path-value'),
+        ('foo4', 'foo4-value'),
+        ('foo', 'foo-value'),
+        ('foo1', 'foo1-value'),
+        ('foo3', 'foo3-value'),
+    ]),
+    ('http://arble.foo.com/path/x', [
+        ('foo4', 'foo4-value'),
+        ('foo1', 'foo1-value'),
+        ('foo3', 'foo3-value'),
+    ]),
+    ('http://foo.com/path/x', [
+        ('foo4', 'foo4-value'),
+        ('foo1', 'foo1-value'),
+        ('foo3', 'foo3-value'),
+    ]),
+]
+
+
+class TestGoCookieJar(unittest.TestCase):
+    def setUp(self):
+        self.dir = tempfile.mkdtemp()
+
+    def tearDown(self):
+        shutil.rmtree(self.dir)
+
+    def test_readcookies(self):
+        jar = self.load_jar(cookie_content)
+        self.assert_jar_queries(jar, cookie_content_queries)
+
+    def test_roundtrip(self):
+        jar = self.load_jar(cookie_content)
+        filename2 = os.path.join(self.dir, 'cookies2')
+        jar.save(filename=filename2)
+        jar = GoCookieJar()
+        jar.load(filename=filename2)
+        self.assert_jar_queries(jar, cookie_content_queries)
+
+    def test_expiry_time(self):
+        content = '''[
+            {
+                "CanonicalHost": "bar.com",
+                "Creation": "2017-11-17T08:53:55.088820092Z",
+                "Domain": "bar.com",
+                "Expires": "2345-11-15T18:16:08Z",
+                "HostOnly": true,
+                "HttpOnly": false,
+                "LastAccess": "2017-11-17T08:53:55.088822562Z",
+                "Name": "bar",
+                "Path": "/",
+                "Persistent": true,
+                "Secure": false,
+                "Updated": "2017-11-17T08:53:55.088822562Z",
+                "Value": "bar-value"
+            }
+        ]'''
+        jar = self.load_jar(content)
+        got_expires = tuple(jar)[0].expires
+        want_expires = int(pyrfc3339.parse('2345-11-15T18:16:08Z').timestamp())
+        self.assertEqual(got_expires, want_expires)
+
+    def load_jar(self, content):
+        filename = os.path.join(self.dir, 'cookies')
+        with open(filename, 'x') as f:
+            f.write(content)
+        jar = GoCookieJar()
+        jar.load(filename=filename)
+        return jar
+
+    def assert_jar_queries(self, jar, queries):
+        '''Assert that all the given queries (see cookie_content_queries)
+        are satisfied when run on the given cookie jar.
+        :param jar CookieJar: the cookie jar to query
+        :param queries: the queries to run.
+        '''
+        for url, want_cookies in queries:
+            req = urllib.request.Request(url)
+            jar.add_cookie_header(req)
+            # We can't use SimpleCookie to find out what cookies
+            # have been presented, because SimpleCookie
+            # only allows one cookie with a given name,
+            # so we naively parse the cookies ourselves, which
+            # is OK because we know we don't have to deal
+            # with any complex cases.
+
+            cookie_header = req.get_header('Cookie')
+            got_cookies = []
+            if cookie_header is not None:
+                got_cookies = [
+                    tuple(part.split('='))
+                    for part in cookie_header.split('; ')
+                ]
+                got_cookies.sort()
+            want_cookies = list(want_cookies)
+            want_cookies.sort()
+            self.assertEqual(got_cookies, want_cookies, msg='query {}; got {}; want {}'.format(url, got_cookies, want_cookies))
diff --git a/modules/libjuju/tests/unit/test_model.py b/modules/libjuju/tests/unit/test_model.py
index 2e33236..2753d85 100644
--- a/modules/libjuju/tests/unit/test_model.py
+++ b/modules/libjuju/tests/unit/test_model.py
@@ -5,6 +5,7 @@
 import asynctest
 
 from juju.client.jujudata import FileJujuData
+from juju.model import Model
 
 
 def _make_delta(entity, type_, data=None):
@@ -159,44 +160,105 @@
                 pass
 
 
+@asynctest.patch('juju.model.Model._after_connect')
 class TestModelConnect(asynctest.TestCase):
     @asynctest.patch('juju.client.connector.Connector.connect_model')
-    @asynctest.patch('juju.model.Model._after_connect')
-    async def test_model_connect_no_args(self, mock_after_connect, mock_connect_model):
-        from juju.model import Model
+    async def test_no_args(self, mock_connect_model, _):
         m = Model()
         await m.connect()
         mock_connect_model.assert_called_once_with(None)
 
     @asynctest.patch('juju.client.connector.Connector.connect_model')
-    @asynctest.patch('juju.model.Model._after_connect')
-    async def test_model_connect_with_model_name(self, mock_after_connect, mock_connect_model):
-        from juju.model import Model
+    async def test_with_model_name(self, mock_connect_model, _):
         m = Model()
         await m.connect(model_name='foo')
         mock_connect_model.assert_called_once_with('foo')
 
     @asynctest.patch('juju.client.connector.Connector.connect_model')
-    @asynctest.patch('juju.model.Model._after_connect')
-    async def test_model_connect_with_endpoint_but_no_uuid(
-        self,
-        mock_after_connect,
-        mock_connect_model,
-    ):
-        from juju.model import Model
+    async def test_with_endpoint_but_no_uuid(self, mock_connect_model, _):
         m = Model()
-        with self.assertRaises(ValueError):
+        with self.assertRaises(TypeError):
             await m.connect(endpoint='0.1.2.3:4566')
         self.assertEqual(mock_connect_model.call_count, 0)
 
     @asynctest.patch('juju.client.connector.Connector.connect')
-    @asynctest.patch('juju.model.Model._after_connect')
-    async def test_model_connect_with_endpoint_and_uuid(
-        self,
-        mock_after_connect,
-        mock_connect,
-    ):
-        from juju.model import Model
+    async def test_with_endpoint_and_uuid_no_auth(self, mock_connect, _):
         m = Model()
-        await m.connect(endpoint='0.1.2.3:4566', uuid='some-uuid')
-        mock_connect.assert_called_once_with(endpoint='0.1.2.3:4566', uuid='some-uuid')
+        with self.assertRaises(TypeError):
+            await m.connect(endpoint='0.1.2.3:4566', uuid='some-uuid')
+        self.assertEqual(mock_connect.call_count, 0)
+
+    @asynctest.patch('juju.client.connector.Connector.connect')
+    async def test_with_endpoint_and_uuid_with_userpass(self, mock_connect, _):
+        m = Model()
+        with self.assertRaises(TypeError):
+            await m.connect(endpoint='0.1.2.3:4566',
+                            uuid='some-uuid',
+                            username='user')
+        await m.connect(endpoint='0.1.2.3:4566',
+                        uuid='some-uuid',
+                        username='user',
+                        password='pass')
+        mock_connect.assert_called_once_with(endpoint='0.1.2.3:4566',
+                                             uuid='some-uuid',
+                                             username='user',
+                                             password='pass')
+
+    @asynctest.patch('juju.client.connector.Connector.connect')
+    async def test_with_endpoint_and_uuid_with_bakery(self, mock_connect, _):
+        m = Model()
+        await m.connect(endpoint='0.1.2.3:4566',
+                        uuid='some-uuid',
+                        bakery_client='bakery')
+        mock_connect.assert_called_once_with(endpoint='0.1.2.3:4566',
+                                             uuid='some-uuid',
+                                             bakery_client='bakery')
+
+    @asynctest.patch('juju.client.connector.Connector.connect')
+    async def test_with_endpoint_and_uuid_with_macaroon(self, mock_connect, _):
+        m = Model()
+        with self.assertRaises(TypeError):
+            await m.connect(endpoint='0.1.2.3:4566',
+                            uuid='some-uuid',
+                            username='user')
+        await m.connect(endpoint='0.1.2.3:4566',
+                        uuid='some-uuid',
+                        macaroons=['macaroon'])
+        mock_connect.assert_called_with(endpoint='0.1.2.3:4566',
+                                        uuid='some-uuid',
+                                        macaroons=['macaroon'])
+        await m.connect(endpoint='0.1.2.3:4566',
+                        uuid='some-uuid',
+                        bakery_client='bakery',
+                        macaroons=['macaroon'])
+        mock_connect.assert_called_with(endpoint='0.1.2.3:4566',
+                                        uuid='some-uuid',
+                                        bakery_client='bakery',
+                                        macaroons=['macaroon'])
+
+    @asynctest.patch('juju.client.connector.Connector.connect_model')
+    @asynctest.patch('juju.client.connector.Connector.connect')
+    async def test_with_posargs(self, mock_connect, mock_connect_model, _):
+        m = Model()
+        await m.connect('foo')
+        mock_connect_model.assert_called_once_with('foo')
+        with self.assertRaises(TypeError):
+            await m.connect('endpoint', 'uuid')
+        with self.assertRaises(TypeError):
+            await m.connect('endpoint', 'uuid', 'user')
+        await m.connect('endpoint', 'uuid', 'user', 'pass')
+        mock_connect.assert_called_once_with(endpoint='endpoint',
+                                             uuid='uuid',
+                                             username='user',
+                                             password='pass')
+        await m.connect('endpoint', 'uuid', 'user', 'pass', 'cacert', 'bakery',
+                        'macaroons', 'loop', 'max_frame_size')
+        mock_connect.assert_called_with(endpoint='endpoint',
+                                        uuid='uuid',
+                                        username='user',
+                                        password='pass',
+                                        cacert='cacert',
+                                        bakery_client='bakery',
+                                        macaroons='macaroons',
+                                        loop='loop',
+                                        max_frame_size='max_frame_size')