Repository: repobee/repobee

Issue:
RepoBee must expose more functionality to allow for reasonable end-to-end tests of plugins
It should reasonably be possible to run RepoBee with a plugin in plugin tests. At the very least, this requires the following:

* The main function being publically exposed, or some wrapped version of it.
* The register_plugin function being publically exposed
* An easy way to unload plugins.

This also means that it's important for the main function to _not_ have side effects. In other words, it must unload plugins after being run. Or, perhaps, we should expose another derivative of the main function that runs the "real" main function, and then unloads plugins.

Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/repobee/repobee/pull/449?src=pr&el=h1) Report
> Merging [#449](https://codecov.io/gh/repobee/repobee/pull/449?src=pr&el=desc) into [master](https://codecov.io/gh/repobee/repobee/commit/ad936229e3ba3dd8505279f4bbb33408c39e5cf5&el=desc) will **decrease** coverage by `4.02%`.
> The diff coverage is `93.33%`.

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

```diff
@@            Coverage Diff             @@
##           master     #449      +/-   ##
==========================================
- Coverage   92.72%   88.69%   -4.03%     
==========================================
  Files          40       40              
  Lines        2143     2149       +6     
  Branches      362      368       +6     
==========================================
- Hits         1987     1906      -81     
- Misses        124      207      +83     
- Partials       32       36       +4     
```


| [Impacted Files](https://codecov.io/gh/repobee/repobee/pull/449?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/\_repobee/plugin.py](https://codecov.io/gh/repobee/repobee/pull/449/diff?src=pr&el=tree#diff-c3JjL19yZXBvYmVlL3BsdWdpbi5weQ==) | `93.22% <92.85%> (-0.72%)` | :arrow_down: |
| [src/\_repobee/main.py](https://codecov.io/gh/repobee/repobee/pull/449/diff?src=pr&el=tree#diff-c3JjL19yZXBvYmVlL21haW4ucHk=) | `91.48% <100.00%> (+3.98%)` | :arrow_up: |
| [src/\_repobee/ext/gitlab.py](https://codecov.io/gh/repobee/repobee/pull/449/diff?src=pr&el=tree#diff-c3JjL19yZXBvYmVlL2V4dC9naXRsYWIucHk=) | `63.86% <0.00%> (-28.56%)` | :arrow_down: |
| [src/\_repobee/git.py](https://codecov.io/gh/repobee/repobee/pull/449/diff?src=pr&el=tree#diff-c3JjL19yZXBvYmVlL2dpdC5weQ==) | `96.62% <0.00%> (-3.38%)` | :arrow_down: |
| [src/\_repobee/command/repos.py](https://codecov.io/gh/repobee/repobee/pull/449/diff?src=pr&el=tree#diff-c3JjL19yZXBvYmVlL2NvbW1hbmQvcmVwb3MucHk=) | `91.80% <0.00%> (-3.28%)` | :arrow_down: |
| [src/\_repobee/ext/query.py](https://codecov.io/gh/repobee/repobee/pull/449/diff?src=pr&el=tree#diff-c3JjL19yZXBvYmVlL2V4dC9xdWVyeS5weQ==) | `48.48% <0.00%> (-1.52%)` | :arrow_down: |
| [src/\_repobee/cli/parsing.py](https://codecov.io/gh/repobee/repobee/pull/449/diff?src=pr&el=tree#diff-c3JjL19yZXBvYmVlL2NsaS9wYXJzaW5nLnB5) | `92.26% <0.00%> (-1.20%)` | :arrow_down: |
| [src/\_repobee/ext/github.py](https://codecov.io/gh/repobee/repobee/pull/449/diff?src=pr&el=tree#diff-c3JjL19yZXBvYmVlL2V4dC9naXRodWIucHk=) | `90.29% <0.00%> (-0.11%)` | :arrow_down: |
| [src/repobee\_plug/\_corehooks.py](https://codecov.io/gh/repobee/repobee/pull/449/diff?src=pr&el=tree#diff-c3JjL3JlcG9iZWVfcGx1Zy9fY29yZWhvb2tzLnB5) | `100.00% <0.00%> (ø)` | |
| ... and [3 more](https://codecov.io/gh/repobee/repobee/pull/449/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/repobee/repobee/pull/449?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/449?src=pr&el=footer). Last update [ad93622...581bd5e](https://codecov.io/gh/repobee/repobee/pull/449?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/src/_repobee/main.py b/src/_repobee/main.py
index 1c9afde..fe9a164 100644
--- a/src/_repobee/main.py
+++ b/src/_repobee/main.py
@@ -50,9 +50,10 @@ def main(sys_args: List[str]):
             LOGGER.info("Non-default plugins disabled")
             plugin.initialize_plugins([constants.DEFAULT_PLUGIN])
         else:
-            plugin_names = plugin.resolve_plugin_names(
-                parsed_preparser_args.plug, config_file
-            )
+            plugin_names = (
+                parsed_preparser_args.plug
+                or config.get_plugin_names(config_file)
+            ) or []
             # IMPORTANT: the default plugin MUST be loaded last to ensure that
             # any user-defined plugins override the firstresult hooks
             plugin.initialize_plugins(
@@ -93,6 +94,8 @@ def main(sys_args: List[str]):
         else:
             LOGGER.error("{.__class__.__name__}: {}".format(exc, str(exc)))
         sys.exit(1)
+    finally:
+        plugin.unregister_all_plugins()
 
 
 if __name__ == "__main__":
diff --git a/src/_repobee/plugin.py b/src/_repobee/plugin.py
index 20f8624..ee2d5de 100644
--- a/src/_repobee/plugin.py
+++ b/src/_repobee/plugin.py
@@ -16,12 +16,11 @@ import types
 import pathlib
 import importlib
 from types import ModuleType
-from typing import List, Optional, Iterable, Mapping
+from typing import List, Optional, Iterable, Mapping, Union
 
 import daiquiri
 
 import _repobee
-from _repobee import config
 from _repobee import exception
 
 import repobee_plug as plug
@@ -90,32 +89,129 @@ def _try_load_module(qualname: str) -> Optional[ModuleType]:
         return None
 
 
-def register_plugins(modules: List[ModuleType]) -> None:
+def register_plugins(modules: List[ModuleType],) -> List[object]:
     """Register the namespaces of the provided modules, and any plug.Plugin
     instances in them. Registers modules in reverse order as they are
     run in LIFO order.
 
     Args:
         modules: A list of modules.
+    Returns:
+        A list of registered modules and and plugin class instances.
     """
     assert all([isinstance(mod, ModuleType) for mod in modules])
+
+    registered = []
     for module in reversed(modules):  # reverse because plugins are run LIFO
         plug.manager.register(module)
+        registered.append(module)
+
         for value in module.__dict__.values():
             if (
                 isinstance(value, type)
                 and issubclass(value, plug.Plugin)
                 and value != plug.Plugin
             ):
-                plug.manager.register(value())
+                obj = value()
+                plug.manager.register(obj)
+                registered.append(obj)
+
+    return registered
+
+
+def unregister_all_plugins() -> None:
+    """Unregister all currently registered plugins."""
+    for p in plug.manager.get_plugins():
+        plug.manager.unregister(p)
+
+
+def try_register_plugin(
+    plugin_module: ModuleType, *plugin_classes: List[type],
+) -> None:
+    """Attempt to register a plugin module and then immediately unregister it.
+
+    .. important::
+        This is a convenience method for sanity checking plugins, and should
+        only be called in test suites. It's not for production use.
+
+    This convenience method can be used to sanity check plugins by registering
+    them with RepoBee. If they have incorrectly defined hooks, this will be
+    discovered only when registering.
+
+    As an example, assume that we have a plugin module with a single (useless)
+    plugin class in it, like this:
+
+    .. code-block:: python
+        :caption: useless.py
+
+        import repobee_plug as plug
+
+        class Useless(plug.Plugin):
+            \"\"\"This plugin does nothing!\"\"\"
+
+    We want to make sure that both the ``useless`` module and the ``Useless``
+    plugin class are registered correctly, and for that we can write some
+    simple code like this.
+
+    .. code-block:: python
+        :caption: Example test case to check registering
 
+        import repobee
+        # assuming that useless is defined in the external plugin
+        # repobee_useless
+        from repobee_useless import useless
 
-def initialize_plugins(plugin_names: List[str] = None):
+        def test_register_useless_plugin():
+            repobee.try_register_plugin(useless, useless.Useless)
+
+    Args:
+        plugin_module: A plugin module.
+        plugin_classes: If the plugin contains any plugin classes (i.e. classes
+            that extend :py:class:`repobee_plug.Plugin`), then these must be
+            provided here. Otherwise, this option should not be provided.
+    Raises:
+        :py:class:`repobee_plug.PlugError` if the module cannot be registered,
+        or if the contained plugin classes does not match
+        plugin_classes.
+    """
+    expected_plugin_classes = set(plugin_classes or [])
+    newly_registered = register_plugins([plugin_module])
+
+    registered_modules = [
+        reg for reg in newly_registered if isinstance(reg, ModuleType)
+    ]
+    registered_classes = {
+        cl.__class__ for cl in newly_registered if cl not in registered_modules
+    }
+
+    errors = []
+    if len(registered_modules) != 1 or registered_modules[0] != plugin_module:
+        errors.append(
+            f"Expected only module {plugin_module}, got {registered_modules}"
+        )
+    elif expected_plugin_classes != registered_classes:
+        errors.append(
+            f"Expected plugin classes {expected_plugin_classes}, "
+            f"got {registered_classes}"
+        )
+
+    for reg in newly_registered:
+        plug.manager.unregister(reg)
+
+    if errors:
+        raise plug.PlugError(errors)
+
+
+def initialize_plugins(
+    plugin_names: List[str] = None,
+) -> List[Union[ModuleType, type]]:
     """Load and register plugins.
 
     Args:
         plugin_names: An optional list of plugin names that overrides the
         configuration file's plugins.
+    Returns:
+        A list of registered modules and classes.
     """
     registered_plugins = plug.manager.get_plugins()
     plug_modules = [
@@ -123,23 +219,9 @@ def initialize_plugins(plugin_names: List[str] = None):
         for p in load_plugin_modules(plugin_names=plugin_names)
         if p not in registered_plugins
     ]
-    register_plugins(plug_modules)
+    registered = register_plugins(plug_modules)
     _handle_deprecation()
-
-
-def resolve_plugin_names(
-    plugin_names: Optional[List[str]], config_file: pathlib.Path,
-) -> List[str]:
-    """Return a list of plugin names to load into RepoBee given a list of
-    externally specified plugin names, and a path to a configuration file.
-
-    Args:
-        plugin_names: A list of names of plugins.
-        config_file: A configuration file.
-    Returns:
-        A list of plugin names that should be loaded.
-    """
-    return [*(plugin_names or config.get_plugin_names(config_file) or [])]
+    return registered
 
 
 def resolve_plugin_version(plugin_module: types.ModuleType,) -> Optional[str]:
diff --git a/src/repobee.py b/src/repobee.py
index 690885b..f474ba0 100644
--- a/src/repobee.py
+++ b/src/repobee.py
@@ -1,5 +1,13 @@
 import sys
-from _repobee import main
+
+from _repobee.main import main
+from _repobee.plugin import unregister_all_plugins, try_register_plugin
+
+__all__ = [
+    main.__name__,
+    try_register_plugin.__name__,
+    unregister_all_plugins.__name__,
+]
 
 if __name__ == "__main__":
-    main.main(sys.argv)
+    main(sys.argv)


Test patch:
diff --git a/tests/unit_tests/repobee/conftest.py b/tests/unit_tests/repobee/conftest.py
index ac28544..52c8a11 100644
--- a/tests/unit_tests/repobee/conftest.py
+++ b/tests/unit_tests/repobee/conftest.py
@@ -7,10 +7,9 @@ import os
 from unittest.mock import MagicMock
 import pytest
 
-from repobee_plug import manager
-
 import _repobee.constants
 import _repobee.config
+import _repobee.plugin
 
 import constants
 
@@ -64,9 +63,7 @@ def unused_path():
 @pytest.fixture(autouse=True)
 def unregister_plugins():
     """All plugins should be unregistered after each function."""
-    registered = manager.get_plugins()
-    for plugin in registered:
-        manager.unregister(plugin)
+    _repobee.plugin.unregister_all_plugins()
 
 
 @pytest.fixture(autouse=True)
diff --git a/tests/unit_tests/repobee/test_command.py b/tests/unit_tests/repobee/test_command.py
index 7a405e9..f5185e8 100644
--- a/tests/unit_tests/repobee/test_command.py
+++ b/tests/unit_tests/repobee/test_command.py
@@ -535,9 +535,7 @@ class TestCloneRepos:
         monkeypatch.setattr("_repobee.ext.pylint.act", act_hook_func)
         _repobee.ext.pylint.act(None, None)
 
-        plugin_names = plugin.resolve_plugin_names(
-            plugin_names=constants.PLUGINS, config_file=unused_path
-        )
+        plugin_names = constants.PLUGINS
         modules = plugin.load_plugin_modules(plugin_names)
         plugin.register_plugins(modules)
 
diff --git a/tests/unit_tests/repobee/test_plugin.py b/tests/unit_tests/repobee/test_plugin.py
index cbd74cc..2e207f0 100644
--- a/tests/unit_tests/repobee/test_plugin.py
+++ b/tests/unit_tests/repobee/test_plugin.py
@@ -34,21 +34,6 @@ def unregister_plugins():
         plug.manager.unregister(p)
 
 
-class TestResolvePluginNames:
-    """Tests for resolve_plugin_names."""
-
-    def test_plugin_names_override_config_file(self, config_mock, mocker):
-        """Test that the plugin_names argument override the configuration
-        file."""
-        plugin_names = ["awesome", "the_slarse_plugin", "ric_easter_egg"]
-
-        actual_plugin_names = plugin.resolve_plugin_names(
-            config_file=str(config_mock), plugin_names=plugin_names
-        )
-
-        assert actual_plugin_names == plugin_names
-
-
 class TestLoadPluginModules:
     """Tests for load_plugin_modules."""
 
@@ -136,6 +121,33 @@ class TestRegisterPlugins:
         plugin_manager_mock.register.assert_has_calls(expected_calls)
 
 
+class TestTryRegisterPlugin:
+    """Tests for try_register_plugin."""
+
+    def test_modules_unregistered_after_success(self):
+        plugin.try_register_plugin(pylint)
+        assert not plug.manager.get_plugins()
+
+    def test_modules_and_classes_unregistered_after_success(self):
+        plugin.try_register_plugin(javac, javac.JavacCloneHook)
+        assert not plug.manager.get_plugins()
+
+    def test_does_not_unregister_unrelated_plugins(self):
+        plug.manager.register(pylint)
+        plugin.try_register_plugin(javac, javac.JavacCloneHook)
+        assert pylint in plug.manager.get_plugins()
+
+    def test_modules_unregistered_after_fail(self):
+        with pytest.raises(plug.PlugError):
+            plugin.try_register_plugin(pylint, javac.JavacCloneHook)
+        assert not plug.manager.get_plugins()
+
+    def test_fails_if_classes_not_specified(self):
+        with pytest.raises(plug.PlugError) as exc_info:
+            plugin.try_register_plugin(javac)
+        assert javac.JavacCloneHook.__name__ in str(exc_info.value)
+
+
 class TestTasks:
     """Tests for testing RepoBee tasks."""