Merge ~jugmac00/lpci:increase-test-coverage into lpci:main

[Jürgen Gmach]

WIP: increase test coverage for lpcraft

[Jürgen Gmach (jugmac00)]

There is one usage of `with pytest.raises` which would be incompatible with alternative test runners. I went with it as afaik testtools prevents using the `with self.assertRaises` syntax of the std lib.

Some tests seem artificial and use a lot of mocking, but I have not seen a much better way to get to 100%.

After this MP has landed, I will refactor some of the tests to get rid of at least the environment variable patching.

[Colin Watson (cjwatson)]

I think it should be possible to eliminate the majority of the extra mocking added here. Some specific suggestions below.

review: Needs Fixing

[Jürgen Gmach (jugmac00)]

I noticed that `test_job_not_defined_for_host_architecture` was actually not testing what the name proposed, but it was testing that jobs are skipped when they are not used in a pipeline. I fixed the test to do what it was meant to do - but I did not create a new one to test jobs are skipped if they are not defined in a pipeline. If I should do so, please just say so.

[Jürgen Gmach (jugmac00)]

Reached 100% and is ready to review.

From Mattermost:
Colin, when I interpret https://pastebin.canonical.com/p/R7wpc8FYj8/ correctly, than verbose/quiet/trace cannot be used together with the version or the run command ( https://pastebin.canonical.com/p/fVNFbBJZxv/ ) - which would mean that the code at the bottom ( https://pastebin.canonical.com/p/TGDbkxWbMQ/ ) can't be reached, as it is called from run. Ok - lpcraft version and lpcraft --version act differently :confused: https://pastebin.canonical.com/p/wWhx3CNpcG/

I added some temporary tests for the emitter states in the main function, and excluded the (afaik) unreachable emitter handling in the run (_run...) function.

I'd like to discuss the cli design maybe int the next standup.

[Colin Watson (cjwatson)]

review: Approve

[Jürgen Gmach (jugmac00)]

Thanks for the review!

diff --git a/.coveragerc b/.coveragerc
index b233f3d..7155812 100644
--- a/.coveragerc
+++ b/.coveragerc
@@ -3,3 +3,4 @@ show_contexts = true
 
 [run]
 dynamic_context = test_function
+branch = True
diff --git a/.gitignore b/.gitignore
index 703c45f..94df19c 100644
--- a/.gitignore
+++ b/.gitignore
@@ -5,3 +5,4 @@
 __pycache__
 .coverage
 htmlcov
+build/
diff --git a/lpcraft/commands/run.py b/lpcraft/commands/run.py
index c02f43d..e5a5fcc 100644
--- a/lpcraft/commands/run.py
+++ b/lpcraft/commands/run.py
@@ -72,12 +72,14 @@ def _run_pipeline(args: Namespace) -> None:
                 continue
 
             cmd = ["lpcraft"]
+            # XXX jugmac00 2021-11-25: coverage ignored for now
+            # but should be tested in future
             if emit.get_mode() == EmitterMode.QUIET:
-                cmd.append("--quiet")
+                cmd.append("--quiet")  # pragma: no cover
             elif emit.get_mode() == EmitterMode.VERBOSE:
-                cmd.append("--verbose")
+                cmd.append("--verbose")  # pragma: no cover
             elif emit.get_mode() == EmitterMode.TRACE:
-                cmd.append("--trace")
+                cmd.append("--trace")  # pragma: no cover
             cmd.extend(["run", "--series", job.series, job_name])
 
             emit.progress(
diff --git a/lpcraft/commands/tests/__init__.py b/lpcraft/commands/tests/__init__.py
index ac9287c..86d857d 100644
--- a/lpcraft/commands/tests/__init__.py
+++ b/lpcraft/commands/tests/__init__.py
@@ -19,6 +19,7 @@ class _CommandResult:
     exit_code: int
     messages: List[str]
     errors: List[CraftError]
+    trace: List[str]
 
 
 class CommandBaseTestCase(TestCase):
@@ -26,7 +27,7 @@ class CommandBaseTestCase(TestCase):
         with patch("sys.argv", ["lpcraft"] + list(args)):
             with RecordingEmitterFixture() as emitter:
                 exit_code = main()
-                return _CommandResult(
+                result = _CommandResult(
                     exit_code,
                     [
                         c.args[1]
@@ -38,4 +39,10 @@ class CommandBaseTestCase(TestCase):
                         for c in emitter.recorder.interactions
                         if c == call("error", ANY)
                     ],
+                    [
+                        c.args[1]
+                        for c in emitter.recorder.interactions
+                        if c == call("trace", ANY)
+                    ],
                 )
+                return result
diff --git a/lpcraft/commands/tests/test_run.py b/lpcraft/commands/tests/test_run.py
index 669e26d..b31b75c 100644
--- a/lpcraft/commands/tests/test_run.py
+++ b/lpcraft/commands/tests/test_run.py
@@ -339,6 +339,7 @@ class RunPipelineTestCase(CommandBaseTestCase):
             """
             pipeline:
                 - test
+                - build-wheel
 
             jobs:
                 test:
diff --git a/lpcraft/providers/_buildd.py b/lpcraft/providers/_buildd.py
index 83ad5d6..9acbc3a 100644
--- a/lpcraft/providers/_buildd.py
+++ b/lpcraft/providers/_buildd.py
@@ -81,7 +81,7 @@ class LPCraftBuilddBaseConfiguration(bases.BuilddBase):
 
     def __eq__(self, other: Any) -> bool:
         if not isinstance(other, LPCraftBuilddBaseConfiguration):
-            return NotImplemented
+            raise TypeError
         return (
             self.alias == other.alias
             and self.environment == other.environment
diff --git a/lpcraft/providers/_lxd.py b/lpcraft/providers/_lxd.py
index 8f0342b..677f97b 100644
--- a/lpcraft/providers/_lxd.py
+++ b/lpcraft/providers/_lxd.py
@@ -67,15 +67,15 @@ class _RealLXDInstaller:
 
     def install(self) -> str:
         """Install LXD."""
-        return lxd.install()
+        return lxd.install()  # pragma: no cover
 
     def is_installed(self) -> bool:
         """Check if LXD is installed (and found on PATH)."""
-        return lxd.is_installed()
+        return lxd.is_installed()  # pragma: no cover
 
     def ensure_lxd_is_ready(self) -> None:
         """Ensure LXD is ready for use."""
-        return lxd.ensure_lxd_is_ready()
+        return lxd.ensure_lxd_is_ready()  # pragma: no cover
 
 
 class LXDProvider(Provider):
diff --git a/lpcraft/providers/tests/__init__.py b/lpcraft/providers/tests/__init__.py
index e0dd083..f9e3072 100644
--- a/lpcraft/providers/tests/__init__.py
+++ b/lpcraft/providers/tests/__init__.py
@@ -29,10 +29,7 @@ class FakeLXDInstaller:
     is_ready: bool = True
 
     def install(self) -> str:
-        if self.can_install:
-            return "4.0"
-        else:
-            raise LXDInstallationError("Cannot install LXD")
+        raise LXDInstallationError("Cannot install LXD")
 
     def is_installed(self) -> bool:
         return self.already_installed
diff --git a/lpcraft/providers/tests/test_buildd.py b/lpcraft/providers/tests/test_buildd.py
index 95410b0..03898c8 100644
--- a/lpcraft/providers/tests/test_buildd.py
+++ b/lpcraft/providers/tests/test_buildd.py
@@ -3,8 +3,10 @@
 
 from unittest.mock import Mock, patch
 
+import pytest
 from craft_providers import Executor, bases
 from craft_providers.actions import snap_installer
+from craft_providers.bases.buildd import BuilddBaseAlias
 
 from lpcraft.providers._buildd import (
     SERIES_TO_BUILDD_IMAGE_ALIAS,
@@ -45,3 +47,10 @@ class TestLPCraftBuilddBaseConfiguration(ProviderBaseTestCase):
             config.setup(executor=mock_instance)
 
         self.assertIsNotNone(raised.exception.__cause__)
+
+    def test_compare_configuration_with_other_type(self):
+        """The configuration should only be comparable to its own type"""
+        with pytest.raises(TypeError):
+            "foo" == LPCraftBuilddBaseConfiguration(
+                alias=BuilddBaseAlias.FOCAL,
+            )
diff --git a/lpcraft/providers/tests/test_lxd.py b/lpcraft/providers/tests/test_lxd.py
index b1fd347..0e111b7 100644
--- a/lpcraft/providers/tests/test_lxd.py
+++ b/lpcraft/providers/tests/test_lxd.py
@@ -343,7 +343,7 @@ class TestLXDProvider(ProviderBaseTestCase):
                 series="focal",
                 architecture="amd64",
             ):
-                pass
+                pass  # pragma: no cover
 
         self.assertIs(error, raised.exception.__cause__)
 
@@ -353,7 +353,6 @@ class TestLXDProvider(ProviderBaseTestCase):
             "craft_providers.lxd.launch", autospec=True, side_effect=error
         )
         provider = self.makeLXDProvider(lxd_launcher=mock_launcher)
-
         with self.assertRaisesRegex(CommandError, r"Boom") as raised:
             with provider.launched_environment(
                 project_name="my-project",
@@ -361,14 +360,13 @@ class TestLXDProvider(ProviderBaseTestCase):
                 series="focal",
                 architecture="amd64",
             ):
-                pass
+                pass  # pragma: no cover
 
         self.assertIs(error, raised.exception.__cause__)
 
     def test_launched_environment_unmounts_and_stops_after_error(self):
         mock_launcher = Mock(spec=launch)
         provider = self.makeLXDProvider(lxd_launcher=mock_launcher)
-
         with self.assertRaisesRegex(RuntimeError, r"Boom"):
             with provider.launched_environment(
                 project_name="my-project",
@@ -417,3 +415,24 @@ class TestLXDProvider(ProviderBaseTestCase):
                 pass
 
         self.assertIs(error, raised.exception.__cause__)
+
+    @patch("lpcraft.providers._lxd.lxd")
+    def test_launched_environment_configure_buildd_image_remote_lxd_error(
+        self, mock_lxd
+    ):  # noqa: E501
+        error = LXDError(brief="Boom")
+        mock_lxd.configure_buildd_image_remote.side_effect = error
+        # original behavior has to be restored as lxd is now a mock
+        mock_lxd.LXDError = LXDError
+        provider = self.makeLXDProvider()
+
+        with self.assertRaisesRegex(CommandError, r"Boom") as raised:
+            with provider.launched_environment(
+                project_name="my-project",
+                project_path=self.mock_path,
+                series="focal",
+                architecture="amd64",
+            ):
+                pass  # pragma: no cover
+
+        self.assertIs(error, raised.exception.__cause__)
diff --git a/lpcraft/providers/tests/test_replay_logs.py b/lpcraft/providers/tests/test_replay_logs.py
new file mode 100644
index 0000000..0a7e9c6
--- /dev/null
+++ b/lpcraft/providers/tests/test_replay_logs.py
@@ -0,0 +1,59 @@
+# Copyright 2021 Canonical Ltd.  This software is licensed under the
+# GNU General Public License version 3 (see the file LICENSE).
+
+from pathlib import Path
+from shutil import copyfile
+from unittest.mock import Mock
+
+from craft_providers import Executor
+from fixtures import TempDir
+
+from lpcraft.commands.tests import CommandBaseTestCase
+from lpcraft.providers import replay_logs
+from lpcraft.tests.fixtures import RecordingEmitterFixture
+
+
+class TestReplayLogs(CommandBaseTestCase):
+    def test_cannot_pull_file(self):
+        mock_instance = Mock(spec=Executor)
+        mock_instance.pull_file.side_effect = FileNotFoundError()
+
+        with RecordingEmitterFixture() as emitter:
+            replay_logs(mock_instance)
+
+        self.assertEqual(
+            ("trace", "No logs found in instance."),
+            emitter.recorder.interactions[0].args,
+        )
+
+    def test_replay_logs(self):
+        self.tempdir = Path(self.useFixture(TempDir()).path)
+        path = self.tempdir / "stub_remote_log_file"
+        path.write_text("line1\nline2\nline3")
+
+        def fake_pull_file(source, destination):
+            # use injected `path` rather than source, which would be a
+            # lpcraft.env.get_managed_environment_log_path, which is not
+            # available in a test
+            self.assertEqual(Path("/tmp/lpcraft.log"), Path(source))
+            copyfile(path, destination)
+
+        mock_instance = Mock(spec=Executor)
+        mock_instance.pull_file = fake_pull_file
+
+        with RecordingEmitterFixture() as emitter:
+            replay_logs(mock_instance)
+
+        self.assertEqual(
+            ("trace", "Logs captured from managed instance:"),
+            emitter.recorder.interactions[0].args,
+        )
+        self.assertEqual(
+            ("trace", ":: line1"), emitter.recorder.interactions[1].args
+        )
+        self.assertEqual(
+            ("trace", ":: line2"), emitter.recorder.interactions[2].args
+        )
+        self.assertEqual(
+            ("trace", ":: line3"), emitter.recorder.interactions[3].args
+        )
diff --git a/lpcraft/tests/test_errors.py b/lpcraft/tests/test_errors.py
new file mode 100644
index 0000000..4c7f303
--- /dev/null
+++ b/lpcraft/tests/test_errors.py
@@ -0,0 +1,14 @@
+# Copyright 2021 Canonical Ltd.  This software is licensed under the
+# GNU General Public License version 3 (see the file LICENSE).
+
+from unittest import TestCase
+
+from lpcraft.errors import CommandError
+
+
+class TestError(TestCase):
+    def test_compare_command_error_with_other_type(self):
+        """If the other type is not a CommandError, defer eq to other type."""
+        self.assertEqual(
+            NotImplemented, CommandError("message").__eq__("message")
+        )
diff --git a/lpcraft/tests/test_main.py b/lpcraft/tests/test_main.py
index dfcb817..e8808e5 100644
--- a/lpcraft/tests/test_main.py
+++ b/lpcraft/tests/test_main.py
@@ -2,6 +2,7 @@
 # GNU General Public License version 3 (see the file LICENSE).
 
 import sys
+from unittest.mock import patch
 
 from craft_cli import EmitterMode
 from fixtures import MockPatch
@@ -9,6 +10,7 @@ from testtools import TestCase
 
 from lpcraft._version import version_description as lpcraft_version
 from lpcraft.main import main
+from lpcraft.tests.fixtures import RecordingEmitterFixture
 
 
 class TestMain(TestCase):
@@ -42,3 +44,76 @@ class TestMain(TestCase):
             sys.stderr,
         )
         mock_emit.ended_ok.assert_called_once_with()
+
+    @patch("lpcraft.main.run")
+    def test_keyboard_interrupt(self, mock_run):
+        self.useFixture(MockPatch("sys.argv", ["lpcraft"]))
+        mock_run.side_effect = KeyboardInterrupt()
+
+        with RecordingEmitterFixture() as emitter:
+            ret = main()
+
+        self.assertEqual(1, ret)
+        # cannot compare the results directly but only the str representation
+        # see https://github.com/canonical/craft-cli/issues/40
+        self.assertEqual(
+            "call('error', CraftError('Interrupted.'))",
+            str(emitter.recorder.interactions[0]),
+        )
+
+    @patch("lpcraft.main.run")
+    def test_handling_unexpected_exception(self, mock_run):
+        self.useFixture(MockPatch("sys.argv", ["lpcraft"]))
+        mock_run.side_effect = RuntimeError()
+
+        with RecordingEmitterFixture() as emitter:
+            ret = main()
+
+        self.assertEqual(1, ret)
+        # cannot compare the results directly but only the str representation
+        # see https://github.com/canonical/craft-cli/issues/40
+        self.assertEqual(
+            "call('error', CraftError('lpcraft internal error: RuntimeError()'))",  # noqa: E501
+            str(emitter.recorder.interactions[0]),
+        )
+
+    def test_quiet_mode(self):
+        # temporary test until cli API is set and a more meaningful test is
+        # possible
+        self.useFixture(MockPatch("sys.argv", ["lpcraft", "--version", "-q"]))
+
+        with RecordingEmitterFixture() as emitter:
+            main()
+
+        # result is something like "lpcraft, version 0.0.1"
+        self.assertIn(
+            "lpcraft, version", emitter.recorder.interactions[0].args[1]
+        )
+
+    def test_verbose_mode(self):
+        # temporary test until cli API is set and a more meaningful test is
+        # possible
+        self.useFixture(MockPatch("sys.argv", ["lpcraft", "--version", "-v"]))
+
+        with RecordingEmitterFixture() as emitter:
+            main()
+
+        # result is something like "lpcraft, version 0.0.1"
+        self.assertIn(
+            "lpcraft, version", emitter.recorder.interactions[0].args[1]
+        )
+
+    def test_trace_mode(self):
+        # temporary test until cli API is set and a more meaningful test is
+        # possible
+        self.useFixture(
+            MockPatch("sys.argv", ["lpcraft", "--version", "--trace"])
+        )
+
+        with RecordingEmitterFixture() as emitter:
+            main()
+
+        # result is something like "lpcraft, version 0.0.1"
+        self.assertIn(
+            "lpcraft, version", emitter.recorder.interactions[0].args[1]
+        )
diff --git a/lpcraft/tests/test_utils.py b/lpcraft/tests/test_utils.py
index 5b28469..9ebc83b 100644
--- a/lpcraft/tests/test_utils.py
+++ b/lpcraft/tests/test_utils.py
@@ -137,6 +137,7 @@ class TestAskUser(TestCase):
             ("N", False),
             ("no", False),
             ("NO", False),
+            ("x", False),  # anything outside y/n should return default
         ):
             with self.subTest(user_input=user_input):
                 mock_input.return_value = user_input
diff --git a/tox.ini b/tox.ini
index 4bba8cc..d506239 100644
--- a/tox.ini
+++ b/tox.ini
@@ -32,6 +32,7 @@ basepython =
     python3.8
 deps =
     -r requirements.txt
+    .[test]
     mypy
     types-PyYAML
 commands =
@@ -45,4 +46,4 @@ commands =
     coverage erase
     coverage run -m pytest
     coverage html
-    coverage report -m  --fail-under=80
+    coverage report -m  --fail-under=100