Repository: repobee/repobee

Issue:
Add arg `--hook-results-file` to store hook results to JSON
This is related to #153 

The idea is that some plugins report results, and if `--hook-results-file` is specified, these results should be written as JSON to the specified file. Initially, it will be enough to add it to the `clone` parser, but in the end it should probably be an optional base argument.

Discussion:
codecov-io: # [Codecov](https://codecov.io/gh/repobee/repobee/pull/328?src=pr&el=h1) Report
> Merging [#328](https://codecov.io/gh/repobee/repobee/pull/328?src=pr&el=desc) into [master](https://codecov.io/gh/repobee/repobee/commit/84aae39ab1ba012cf942c7b247489276718ddbeb?src=pr&el=desc) will **decrease** coverage by `0.21%`.
> The diff coverage is `82.6%`.

[![Impacted file tree graph](https://codecov.io/gh/repobee/repobee/pull/328/graphs/tree.svg?width=650&token=5vOKgkphZe&height=150&src=pr)](https://codecov.io/gh/repobee/repobee/pull/328?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #328      +/-   ##
==========================================
- Coverage   89.87%   89.65%   -0.22%     
==========================================
  Files          20       20              
  Lines        1442     1460      +18     
  Branches      261      263       +2     
==========================================
+ Hits         1296     1309      +13     
- Misses        123      129       +6     
+ Partials       23       22       -1
```


| [Impacted Files](https://codecov.io/gh/repobee/repobee/pull/328?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/\_repobee/cli.py](https://codecov.io/gh/repobee/repobee/pull/328/diff?src=pr&el=tree#diff-c3JjL19yZXBvYmVlL2NsaS5weQ==) | `98.2% <100%> (-0.56%)` | :arrow_down: |
| [src/\_repobee/command.py](https://codecov.io/gh/repobee/repobee/pull/328/diff?src=pr&el=tree#diff-c3JjL19yZXBvYmVlL2NvbW1hbmQucHk=) | `92.81% <100%> (-0.59%)` | :arrow_down: |
| [src/\_repobee/util.py](https://codecov.io/gh/repobee/repobee/pull/328/diff?src=pr&el=tree#diff-c3JjL19yZXBvYmVlL3V0aWwucHk=) | `86.36% <42.85%> (-8.24%)` | :arrow_down: |
| [src/\_repobee/ext/gitlab.py](https://codecov.io/gh/repobee/repobee/pull/328/diff?src=pr&el=tree#diff-c3JjL19yZXBvYmVlL2V4dC9naXRsYWIucHk=) | `64.41% <0%> (ø)` | :arrow_up: |
| [src/\_repobee/formatters.py](https://codecov.io/gh/repobee/repobee/pull/328/diff?src=pr&el=tree#diff-c3JjL19yZXBvYmVlL2Zvcm1hdHRlcnMucHk=) | `95.74% <0%> (+4.25%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/repobee/repobee/pull/328?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/repobee/repobee/pull/328?src=pr&el=footer). Last update [84aae39...4233f47](https://codecov.io/gh/repobee/repobee/pull/328?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/requirements.txt b/requirements.txt
index 05c3420..f1c41ee 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -4,4 +4,4 @@ daiquiri
 pluggy>=0.8.0
 pygithub
 python-gitlab==1.8.0
-repobee-plug==0.8.0
+repobee-plug==0.9.0
diff --git a/setup.py b/setup.py
index 7e8c9fd..39587b7 100644
--- a/setup.py
+++ b/setup.py
@@ -23,7 +23,7 @@ required = [
     "daiquiri",
     "pygithub",
     "colored",
-    "repobee-plug==0.8.0",
+    "repobee-plug==0.9.0",
     "python-gitlab==1.8.0",
 ]
 
diff --git a/src/_repobee/cli.py b/src/_repobee/cli.py
index 43d6979..e43b0b5 100644
--- a/src/_repobee/cli.py
+++ b/src/_repobee/cli.py
@@ -30,6 +30,7 @@ from _repobee import util
 from _repobee import exception
 from _repobee import config
 from _repobee import constants
+from _repobee import formatters
 
 try:
     os.makedirs(str(constants.LOG_DIR), exist_ok=True)
@@ -282,6 +283,7 @@ def dispatch_command(
         ext_commands: A list of active extension commands.
     """
     ext_command_names = [cmd.name for cmd in ext_commands or []]
+    hook_results = {}
     if ext_command_names and args.subparser in ext_command_names:
         ext_cmd = ext_commands[ext_command_names.index(args.subparser)]
         with _sys_exit_on_expected_error():
@@ -311,7 +313,9 @@ def dispatch_command(
             command.migrate_repos(args.master_repo_urls, api)
     elif args.subparser == CLONE_PARSER:
         with _sys_exit_on_expected_error():
-            command.clone_repos(args.master_repo_names, args.students, api)
+            hook_results = command.clone_repos(
+                args.master_repo_names, args.students, api
+            )
     elif args.subparser == VERIFY_PARSER:
         with _sys_exit_on_expected_error():
             plug.manager.hook.get_api_class().verify_settings(
@@ -364,6 +368,25 @@ def dispatch_command(
             "This is a bug, please open an issue.".format(args.subparser)
         )
 
+    if hook_results:
+        _handle_hook_results(
+            hook_results=hook_results, filepath=args.hook_results_file
+        )
+
+
+def _handle_hook_results(hook_results, filepath):
+    LOGGER.info(formatters.format_hook_results_output(hook_results))
+    if filepath:
+        LOGGER.warning(
+            "Storing hook results to file is a beta feature, the file format "
+            "is not final"
+        )
+        output_file = pathlib.Path(filepath)
+        util.atomic_write(
+            plug.result_mapping_to_json(hook_results), output_file
+        )
+        LOGGER.info("Hook results stored to {}".format(filepath))
+
 
 def _add_peer_review_parsers(base_parsers, subparsers):
     assign_parser = subparsers.add_parser(
@@ -762,6 +785,13 @@ def _add_subparsers(parser, show_all_opts, ext_commands):
         parents=[base_parser, base_student_parser, repo_name_parser],
         formatter_class=_OrderedFormatter,
     )
+    clone.add_argument(
+        "--hook-results-file",
+        help="Path to a file to store results from plugin hooks in. The "
+        "results are stored as JSON, regardless of file extension.",
+        type=str,
+        default=None,
+    )
 
     plug.manager.hook.clone_parser_hook(clone_parser=clone)
 
diff --git a/src/_repobee/command.py b/src/_repobee/command.py
index 7b9e50e..ae5b14c 100644
--- a/src/_repobee/command.py
+++ b/src/_repobee/command.py
@@ -18,7 +18,7 @@ import shutil
 import os
 import sys
 import tempfile
-from typing import Iterable, List, Optional, Tuple, Generator
+from typing import Iterable, List, Optional, Tuple, Generator, Mapping
 from colored import bg, fg, style
 
 import daiquiri
@@ -343,7 +343,7 @@ def close_issue(
 
 def clone_repos(
     master_repo_names: Iterable[str], teams: Iterable[plug.Team], api: plug.API
-) -> None:
+) -> Mapping[str, List[plug.HookResult]]:
     """Clone all student repos related to the provided master repos and student
     teams.
 
@@ -352,6 +352,8 @@ def clone_repos(
         teams: An iterable of student teams.
         api: An implementation of :py:class:`repobee_plug.API` used to
             interface with the platform (e.g. GitHub or GitLab) instance.
+    Returns:
+        A mapping from repo name to a list of hook results.
     """
     repo_urls = api.get_repo_urls(master_repo_names, teams=teams)
     # the reason we first compute the urls and then extract repo names is that
@@ -374,8 +376,8 @@ def clone_repos(
     for plugin in plug.manager.get_plugins():
         if "act_on_cloned_repo" in dir(plugin):
             repo_names = util.generate_repo_names(teams, master_repo_names)
-            _execute_post_clone_hooks(repo_names, api)
-            break
+            return _execute_post_clone_hooks(repo_names, api)
+    return {}
 
 
 def _clone_repos_no_check(repo_urls, dst_dirpath, api):
@@ -400,7 +402,9 @@ def _clone_repos_no_check(repo_urls, dst_dirpath, api):
     return [repo.name for repo in cloned_repos]
 
 
-def _execute_post_clone_hooks(repo_names: List[str], api: plug.API):
+def _execute_post_clone_hooks(
+    repo_names: List[str], api: plug.API
+) -> Mapping[str, List[plug.HookResult]]:
     LOGGER.info("Executing post clone hooks on repos")
     local_repos = [name for name in os.listdir() if name in repo_names]
 
@@ -411,9 +415,8 @@ def _execute_post_clone_hooks(repo_names: List[str], api: plug.API):
             path=os.path.abspath(repo_name), api=api
         )
         results[repo_name] = res
-    LOGGER.info(formatters.format_hook_results_output(results))
 
-    LOGGER.info("Post clone hooks done")
+    return results
 
 
 def migrate_repos(master_repo_urls: Iterable[str], api: plug.API) -> None:
diff --git a/src/_repobee/util.py b/src/_repobee/util.py
index 9adaa95..e4678f9 100644
--- a/src/_repobee/util.py
+++ b/src/_repobee/util.py
@@ -9,6 +9,8 @@
 import os
 import sys
 import pathlib
+import shutil
+import tempfile
 from typing import Iterable, Generator, Union, Set
 
 from repobee_plug import apimeta
@@ -135,3 +137,21 @@ def find_files_by_extension(
         for file in files:
             if _ends_with_ext(file, extensions):
                 yield pathlib.Path(cwd) / file
+
+
+def atomic_write(content: str, dst: pathlib.Path) -> None:
+    """Write the given contents to the destination "atomically". Achieved by
+    writin in a temporary directory and then moving the file to the
+    destination.
+
+    Args:
+        content: The content to write to the new file.
+        dst: Path to the file.
+    """
+    with tempfile.TemporaryDirectory() as tmpdir:
+        with tempfile.NamedTemporaryFile(
+            delete=False, dir=tmpdir, mode="w"
+        ) as file:
+            file.write(content)
+
+        shutil.move(file.name, str(dst))


Test patch:
diff --git a/tests/unit_tests/test_cli.py b/tests/unit_tests/test_cli.py
index e25b6db..f20f739 100644
--- a/tests/unit_tests/test_cli.py
+++ b/tests/unit_tests/test_cli.py
@@ -1,7 +1,7 @@
 import os
 import pathlib
 import argparse
-from unittest.mock import MagicMock
+import collections
 from unittest import mock
 
 import pytest
@@ -56,12 +56,13 @@ VALID_PARSED_ARGS = dict(
     author=None,
     token=TOKEN,
     num_reviews=1,
+    hook_results_file=None,
 )
 
 
 @pytest.fixture(autouse=True)
 def api_instance_mock(mocker):
-    instance_mock = MagicMock(spec=_repobee.ext.github.GitHubAPI)
+    instance_mock = mock.MagicMock(spec=_repobee.ext.github.GitHubAPI)
     instance_mock.get_repo_urls.side_effect = lambda repo_names, org_name: [
         generate_repo_url(rn, org_name) for rn in repo_names
     ]
@@ -162,9 +163,82 @@ def command_all_raise_mock(command_mock, api_class_mock, request):
     return command_mock
 
 
+@pytest.fixture
+def hook_result_mapping():
+    """A hook result mapping for use as a mocked return value to commands that
+    return hook results (e.g. command.clone_repos).
+    """
+    hook_results = collections.defaultdict(list)
+    for repo_name, hook_name, status, msg, data in [
+        (
+            "slarse-task-1",
+            "junit4",
+            plug.Status.SUCCESS,
+            "All tests passed",
+            {"extra": "data", "arbitrary": {"nesting": "here"}},
+        ),
+        (
+            "slarse-task-1",
+            "javac",
+            plug.Status.ERROR,
+            "Some stuff failed",
+            None,
+        ),
+        (
+            "glassey-task-2",
+            "pylint",
+            plug.Status.WARNING,
+            "-10/10 code quality",
+            None,
+        ),
+    ]:
+        hook_results[repo_name].append(
+            plug.HookResult(hook=hook_name, status=status, msg=msg, data=data)
+        )
+    return dict(hook_results)
+
+
 class TestDispatchCommand:
     """Test the handling of parsed arguments."""
 
+    def test_writes_hook_results_correctly(
+        self, tmpdir, hook_result_mapping, api_instance_mock
+    ):
+        expected_filepath = pathlib.Path(".").resolve() / "results.json"
+        expected_json = plug.result_mapping_to_json(hook_result_mapping)
+        args_dict = dict(VALID_PARSED_ARGS)
+        args_dict["hook_results_file"] = str(expected_filepath)
+        with mock.patch(
+            "_repobee.util.atomic_write", autospec=True
+        ) as write, mock.patch(
+            "_repobee.command.clone_repos",
+            autospec=True,
+            return_value=hook_result_mapping,
+        ):
+            args = argparse.Namespace(subparser=cli.CLONE_PARSER, **args_dict)
+            cli.dispatch_command(args, api_instance_mock)
+
+        write.assert_called_once_with(expected_json, expected_filepath)
+
+    def test_does_not_write_hook_results_if_there_are_none(
+        self, tmpdir, api_instance_mock
+    ):
+        """Test that no file is created if there are no hook results, even if
+        --hook-results-file is specified.
+        """
+        expected_filepath = pathlib.Path(".").resolve() / "results.json"
+        args_dict = dict(VALID_PARSED_ARGS)
+        args_dict["hook_results_file"] = str(expected_filepath)
+        with mock.patch(
+            "_repobee.util.atomic_write", autospec=True
+        ) as write, mock.patch(
+            "_repobee.command.clone_repos", autospec=True, return_value={}
+        ):
+            args = argparse.Namespace(subparser=cli.CLONE_PARSER, **args_dict)
+            cli.dispatch_command(args, api_instance_mock)
+
+        assert not write.called
+
     def test_raises_on_invalid_subparser_value(self, api_instance_mock):
         parser = "DOES_NOT_EXIST"
         args = argparse.Namespace(subparser=parser, **VALID_PARSED_ARGS)
@@ -555,7 +629,7 @@ class TestExtensionCommands:
         """Return a mock callback function for use with an extension
         command.
         """
-        yield MagicMock(
+        yield mock.MagicMock(
             spec=_repobee.ext.configwizard.create_extension_command
         )