Repository: repobee/repobee

Issue:
--no-plugins does not exclude config file plugins
Even if you specify `--no-plugins`, plugins specced in the config file will be loaded.

Discussion:

Patch:
diff --git a/src/_repobee/cli.py b/src/_repobee/cli.py
index bdb7779..ae02f32 100644
--- a/src/_repobee/cli.py
+++ b/src/_repobee/cli.py
@@ -598,7 +598,7 @@ def create_parser_for_docs():
     """
     daiquiri.setup(level=logging.FATAL)
     # load default plugins
-    plugin.initialize_plugins()
+    plugin.initialize_plugins([_repobee.constants.DEFAULT_PLUGIN])
     ext_commands = plug.manager.hook.create_extension_command()
     return _create_parser(show_all_opts=True, ext_commands=ext_commands)
 
diff --git a/src/_repobee/constants.py b/src/_repobee/constants.py
index bc74930..6a4ce12 100644
--- a/src/_repobee/constants.py
+++ b/src/_repobee/constants.py
@@ -32,3 +32,5 @@ ORDERED_CONFIGURABLE_ARGS = (
     "plugins",
 )
 CONFIGURABLE_ARGS = set(ORDERED_CONFIGURABLE_ARGS)
+
+DEFAULT_PLUGIN = "defaults"
diff --git a/src/_repobee/main.py b/src/_repobee/main.py
index 3dbdd89..35b2f3e 100644
--- a/src/_repobee/main.py
+++ b/src/_repobee/main.py
@@ -15,6 +15,7 @@ import repobee_plug as plug
 from _repobee import cli
 from _repobee import plugin
 from _repobee import exception
+from _repobee import constants
 
 LOGGER = daiquiri.getLogger(__file__)
 
@@ -56,9 +57,16 @@ def main(sys_args: List[str]):
 
         if parsed_preparser_args.no_plugins:
             LOGGER.info("Non-default plugins disabled")
-            plugin.initialize_plugins()
+            plugin.initialize_plugins([constants.DEFAULT_PLUGIN])
         else:
-            plugin.initialize_plugins(parsed_preparser_args.plug or [])
+            plugin_names = plugin.resolve_plugin_names(
+                parsed_preparser_args.plug, constants.DEFAULT_CONFIG_FILE
+            )
+            # IMPORTANT: the default plugin MUST be loaded last to ensure that
+            # any user-defined plugins override the firstresult hooks
+            plugin.initialize_plugins(
+                plugin_names + [constants.DEFAULT_PLUGIN]
+            )
 
         ext_commands = plug.manager.hook.create_extension_command()
         parsed_args, api = cli.parse_args(
diff --git a/src/_repobee/plugin.py b/src/_repobee/plugin.py
index 7677abf..a6d6108 100644
--- a/src/_repobee/plugin.py
+++ b/src/_repobee/plugin.py
@@ -11,7 +11,7 @@ Module containing plugin system utility functions and classes.
 import pathlib
 import importlib
 from types import ModuleType
-from typing import Union, List, Optional, Iterable
+from typing import List, Optional, Iterable
 
 import daiquiri
 
@@ -35,21 +35,13 @@ def _external_plugin_qualname(plugin_name):
     )
 
 
-DEFAULT_PLUGIN = "defaults"
+def load_plugin_modules(plugin_names: Iterable[str]) -> List[ModuleType]:
+    """Load the given plugins. Plugins are loaded such that they are executed
+    in the same order that they are specified in the plugin_names list.
 
-
-def load_plugin_modules(
-    config_file: Union[str, pathlib.Path] = constants.DEFAULT_CONFIG_FILE,
-    plugin_names: Iterable[str] = None,
-) -> List[ModuleType]:
-    """Load plugins that are specified in the config, as well as default
-    plugins. Note that default plugins are always loaded first, such that
-    they are either run last or overridden by plugins with firstresult=True
-    (such as the default_peer_review plugin).
-
-    Try to import first from :py:mod:`_repobee.ext`, and then from
-    ``repobee_<plugin>``. For example, if ``javac`` is listed as a plugin, the
-    following imports will be attempted:
+    When loading a plugin, tries to import first from :py:mod:`_repobee.ext`,
+    and then from ``repobee_<plugin>``. For example, if ``javac`` is listed as
+    a plugin, the following imports will be attempted:
 
     .. code-block:: python
 
@@ -60,23 +52,13 @@ def load_plugin_modules(
         from repobee_javac import javac
 
     Args:
-        config_file: Path to the configuration file.
         plugin_names: A list of plugin names. Overrides the config file.
 
     Returns:
         a list of loaded modules.
     """
     loaded_modules = []
-    plugin_names = [
-        *(plugin_names or config.get_plugin_names(config_file) or []),
-        # default plugin last so hooks are overridden by user-specified hooks
-        DEFAULT_PLUGIN,
-    ]
     LOGGER.info("Loading plugins: " + ", ".join(plugin_names))
-    if plugin_names == [DEFAULT_PLUGIN]:
-        from .ext import defaults
-
-        return [defaults]
 
     for name in plugin_names:
         plug_mod = _try_load_module(
@@ -135,3 +117,19 @@ def initialize_plugins(plugin_names: List[str] = None):
     """
     plug_modules = load_plugin_modules(plugin_names=plugin_names)
     register_plugins(plug_modules)
+
+
+def resolve_plugin_names(
+    plugin_names: Optional[List[str]] = None,
+    config_file: pathlib.Path = constants.DEFAULT_CONFIG_FILE,
+) -> 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 [])]


Test patch:
diff --git a/tests/unit_tests/test_cli.py b/tests/unit_tests/test_cli.py
index 932031c..e6ac806 100644
--- a/tests/unit_tests/test_cli.py
+++ b/tests/unit_tests/test_cli.py
@@ -1,18 +1,17 @@
 import os
-import argparse
 import pathlib
 import argparse
 from unittest.mock import MagicMock
 from unittest import mock
 
 import pytest
-import repobee_plug as plug
 
 import repobee_plug as plug
 
 import _repobee
 import _repobee.ext
 import _repobee.ext.github
+import _repobee.constants
 import _repobee.plugin
 from _repobee import cli
 from _repobee import exception
@@ -82,7 +81,9 @@ def api_class_mock(mocker, api_instance_mock):
 @pytest.fixture(autouse=True)
 def load_default_plugins(api_instance_mock):
     """Load the default plugins after mocking the GitHubAPI."""
-    loaded = _repobee.plugin.load_plugin_modules()
+    loaded = _repobee.plugin.load_plugin_modules(
+        [_repobee.constants.DEFAULT_PLUGIN]
+    )
     _repobee.plugin.register_plugins(loaded)
 
 
@@ -1075,8 +1076,6 @@ class TestCloneParser:
             args=mock.ANY
         )
 
-    # def test_no_plugins_option_drops_plugins()
-
     @pytest.mark.parametrize(
         "parser, extra_args",
         [
diff --git a/tests/unit_tests/test_command.py b/tests/unit_tests/test_command.py
index 86a1586..dd2747c 100644
--- a/tests/unit_tests/test_command.py
+++ b/tests/unit_tests/test_command.py
@@ -482,8 +482,11 @@ class TestCloneRepos:
     """Tests for clone_repos."""
 
     @pytest.fixture
-    def register_default_plugins(self, config_mock):
-        modules = plugin.load_plugin_modules(str(config_mock))
+    def register_default_plugins(self):
+        plugin_names = plugin.resolve_plugin_names(
+            plugin_names=constants.PLUGINS
+        )
+        modules = plugin.load_plugin_modules(plugin_names)
         plugin.register_plugins(modules)
 
     @pytest.fixture
@@ -519,7 +522,10 @@ class TestCloneRepos:
             "_repobee.ext.pylint.act_on_cloned_repo", act_hook_func
         )
 
-        modules = plugin.load_plugin_modules(str(config_mock))
+        plugin_names = plugin.resolve_plugin_names(
+            plugin_names=constants.PLUGINS
+        )
+        modules = plugin.load_plugin_modules(plugin_names)
         plugin.register_plugins(modules)
 
         return javac_hook, pylint_hook
@@ -662,7 +668,9 @@ def test_purge_review_teams(master_names, students, api_mock):
 class TestAssignPeerReviewers:
     @pytest.fixture(autouse=True)
     def load_default_plugins(self):
-        modules = plugin.load_plugin_modules()
+        modules = plugin.load_plugin_modules(
+            [_repobee.constants.DEFAULT_PLUGIN]
+        )
         plugin.register_plugins(modules)
 
     @pytest.mark.parametrize(
diff --git a/tests/unit_tests/test_main.py b/tests/unit_tests/test_main.py
index 1b9230c..1d036b7 100644
--- a/tests/unit_tests/test_main.py
+++ b/tests/unit_tests/test_main.py
@@ -5,6 +5,7 @@ from collections import namedtuple
 import pytest
 
 from functions import raise_
+import _repobee.constants
 from _repobee import cli
 from _repobee import main
 from _repobee import plugin
@@ -131,7 +132,9 @@ def test_plugins_args(
 
     main.main(sys_args)
 
-    init_plugins_mock.assert_called_once_with(["javac", "pylint"])
+    init_plugins_mock.assert_called_once_with(
+        ["javac", "pylint", _repobee.constants.DEFAULT_PLUGIN]
+    )
     parse_args_mock.assert_called_once_with(
         CLONE_ARGS, show_all_opts=False, ext_commands=[]
     )
@@ -141,13 +144,48 @@ def test_no_plugins_arg(
     parse_args_mock, dispatch_command_mock, init_plugins_mock
 ):
     """Default plugins still need to be loaded, so initialize_plugins should be
-    called without arguments.
+    called only with the default plugin.
+    """
+    sys_args = ["repobee", "--no-plugins", *CLONE_ARGS]
+
+    main.main(sys_args)
+
+    init_plugins_mock.assert_called_once_with(
+        [_repobee.constants.DEFAULT_PLUGIN]
+    )
+    parse_args_mock.assert_called_once_with(
+        CLONE_ARGS, show_all_opts=False, ext_commands=[]
+    )
+
+
+def test_no_plugins_with_configured_plugins(
+    parse_args_mock, dispatch_command_mock, init_plugins_mock, config_mock
+):
+    """Test that --no-plugins causes any plugins listed in the config file to
+    NOT be loaded.
     """
     sys_args = ["repobee", "--no-plugins", *CLONE_ARGS]
 
     main.main(sys_args)
 
-    init_plugins_mock.assert_called_once_with()
+    init_plugins_mock.assert_called_once_with(
+        [_repobee.constants.DEFAULT_PLUGIN]
+    )
+    parse_args_mock.assert_called_once_with(
+        CLONE_ARGS, show_all_opts=False, ext_commands=[]
+    )
+
+
+def test_configured_plugins_are_loaded(
+    parse_args_mock, dispatch_command_mock, init_plugins_mock, config_mock
+):
+    sys_args = ["repobee", *CLONE_ARGS]
+
+    main.main(sys_args)
+
+    init_plugins_mock.assert_called_once_with(
+        [*constants.PLUGINS, _repobee.constants.DEFAULT_PLUGIN]
+    )
     parse_args_mock.assert_called_once_with(
         CLONE_ARGS, show_all_opts=False, ext_commands=[]
     )
@@ -160,7 +198,9 @@ def test_plugin_with_subparser_name(
 
     main.main(sys_args)
 
-    init_plugins_mock.assert_called_once_with(["javac", "clone"])
+    init_plugins_mock.assert_called_once_with(
+        ["javac", "clone", _repobee.constants.DEFAULT_PLUGIN]
+    )
     parse_args_mock.assert_called_once_with(
         CLONE_ARGS, show_all_opts=False, ext_commands=[]
     )
@@ -185,8 +225,7 @@ def test_invalid_plug_options(dispatch_command_mock, init_plugins_mock):
     Note that the default plugins must be loaded for this test to work.
     """
     # load default plugins
-    loaded = plugin.load_plugin_modules()
-    print(loaded)
+    loaded = plugin.load_plugin_modules([_repobee.constants.DEFAULT_PLUGIN])
     plugin.register_plugins(loaded)
 
     sys_args = ["repobee", "-p", "javac", "-f", *CLONE_ARGS]
@@ -194,7 +233,9 @@ def test_invalid_plug_options(dispatch_command_mock, init_plugins_mock):
     with pytest.raises(SystemExit):
         main.main(sys_args)
 
-    init_plugins_mock.assert_called_once_with(["javac"])
+    init_plugins_mock.assert_called_once_with(
+        ["javac", _repobee.constants.DEFAULT_PLUGIN]
+    )
     assert not dispatch_command_mock.called
 
 
diff --git a/tests/unit_tests/test_plugin.py b/tests/unit_tests/test_plugin.py
index 84b199b..770e5b9 100644
--- a/tests/unit_tests/test_plugin.py
+++ b/tests/unit_tests/test_plugin.py
@@ -6,99 +6,66 @@
         installed any other plugins, tests in here may fail unexpectedly
         without anything actually being wrong.
 """
-import os
 from unittest.mock import call, MagicMock
 
 import pytest
 
+import _repobee.constants
 from _repobee import plugin
 from _repobee import exception
 
-from _repobee.plugin import DEFAULT_PLUGIN
-from _repobee.ext import javac, pylint, defaults
+from _repobee.ext import javac, pylint
 
 import constants
 
 PLUGINS = constants.PLUGINS
 
 
-class TestLoadPluginModules:
-    """Tests for load_plugin_modules.
-
-    Note that the default plugin _repobee.ext.defaults is always loaded.
-    """
-
-    def test_load_all_bundled_plugins(self, config_mock):
-        """Test load the bundled plugins, i.e. the ones listed in
-        constants.PLUGINS.
-        """
-        expected_names = list(
-            map(plugin._plugin_qualname, [*PLUGINS, DEFAULT_PLUGIN])
-        )
-
-        modules = plugin.load_plugin_modules(str(config_mock))
-        module_names = [mod.__name__ for mod in modules]
-
-        assert module_names == expected_names
+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"]
-        expected_calls = [
-            call(plug)
-            for plug in map(
-                plugin._plugin_qualname, plugin_names + [DEFAULT_PLUGIN]
-            )
-        ]
-
-        class module:
-            pass
-
-        load_module_mock = mocker.patch(
-            "_repobee.plugin._try_load_module", return_value=module
-        )
 
-        plugin.load_plugin_modules(
+        actual_plugin_names = plugin.resolve_plugin_names(
             config_file=str(config_mock), plugin_names=plugin_names
         )
 
-        load_module_mock.assert_has_calls(expected_calls)
+        assert actual_plugin_names == plugin_names
 
-    def test_load_no_plugins(self, no_config_mock):
-        """Test calling load plugins when no plugins are specified results in
-        only the default being loaded.
-        """
-        modules = plugin.load_plugin_modules()
 
-        assert modules == [defaults]
+class TestLoadPluginModules:
+    """Tests for load_plugin_modules."""
 
-    def test_specify_single_plugin(self, empty_config_mock):
-        plugin_name = "javac"
-        config_contents = os.linesep.join(
-            ["[DEFAULTS]", "plugins = {}".format(plugin_name)]
-        )
-        empty_config_mock.write(config_contents)
+    def test_load_all_bundled_plugins(self):
+        """Test load the bundled plugins, i.e. the ones listed in
+        constants.PLUGINS.
+        """
+        plugin_names = [*PLUGINS, _repobee.constants.DEFAULT_PLUGIN]
+        plugin_qualnames = list(map(plugin._plugin_qualname, plugin_names))
 
-        modules = plugin.load_plugin_modules(str(empty_config_mock))
+        modules = plugin.load_plugin_modules(plugin_names)
         module_names = [mod.__name__ for mod in modules]
 
-        assert module_names == list(
-            map(plugin._plugin_qualname, [plugin_name, DEFAULT_PLUGIN])
-        )
+        assert module_names == plugin_qualnames
+
+    def test_load_no_plugins(self, no_config_mock):
+        """Test calling load plugins when no plugins are specified results in
+        no plugins being loaded."""
+        modules = plugin.load_plugin_modules([])
+
+        assert modules == []
 
     def test_raises_when_loading_invalid_module(self, empty_config_mock):
         """Test that PluginError is raised when when the plugin specified
         does not exist.
         """
         plugin_name = "this_plugin_does_not_exist"
-        config_contents = os.linesep.join(
-            ["[DEFAULTS]", "plugins = {}".format(plugin_name)]
-        )
-        empty_config_mock.write(config_contents)
 
         with pytest.raises(exception.PluginError) as exc_info:
-            plugin.load_plugin_modules(str(empty_config_mock))
+            plugin.load_plugin_modules([plugin_name])
 
         assert "failed to load plugin module " + plugin_name in str(
             exc_info.value