Address PR comments (version 4)

Author: Theofilos Manitaras

docs/manpage.rst

@@ -390,7 +390,7 @@ It does so by leveraging the selected system's environment modules system.
 .. option:: --module-path=PATH
 
    Manipulate the ``MODULEPATH`` environment variable before acting on any tests.
-   If ``PATH`` starts with the `-` character it will be removed from the ``MODULEPATH``, whereas if it starts with the `+` character, it will be added to it.
+   If ``PATH`` starts with the `-` character, it will be removed from the ``MODULEPATH``, whereas if it starts with the `+` character, it will be added to it.
    In all other cases, ``PATH`` will completely override MODULEPATH.
    This option may be specified multiple times, in which case all the paths specified will be added or removed in order.
 

reframe/core/modules.py

@@ -215,6 +215,7 @@ def execute(self, cmd, *args):
         '''
         return self._backend.execute(cmd, *args)
 
+
     def load_module(self, name, force=False, collection=False):
         '''Load the module ``name``.
 
@@ -391,10 +392,26 @@ class ModulesSystemImpl(abc.ABC):
     :meta private:
     '''
 
-    @abc.abstractmethod
     def execute(self, cmd, *args):
+        '''Execute an arbitrary module command using the modules backend.
+
+        :arg cmd: The command to execute, e.g., ``load``, ``restore`` etc.
+        :arg args: The arguments to pass to the command.
+        :returns: The command output.
+        '''
+        try:
+            exec_output = self._execute(cmd, *args)
+        except SpawnedProcessError as e:
+            raise EnvironError from e
+
+        return exec_output
+
+
+    @abc.abstractmethod
+    def _execute(self, cmd, *args):
         '''Execute an arbitrary command of the module system.'''
 
+
     @abc.abstractmethod
     def available_modules(self, substr):
         '''Return a list of available modules, whose name contains ``substr``.
@@ -530,7 +547,7 @@ def version(self):
     def modulecmd(self, *args):
         return ' '.join(['modulecmd', 'python', *args])
 
-    def execute(self, cmd, *args):
+    def _execute(self, cmd, *args):
         modulecmd = self.modulecmd(cmd, *args)
         completed = osext.run_command(modulecmd)
         if re.search(r'ERROR', completed.stderr) is not None:
@@ -652,7 +669,7 @@ def name(self):
     def modulecmd(self, *args):
         return ' '.join([self._command, *args])
 
-    def execute(self, cmd, *args):
+    def _execute(self, cmd, *args):
         modulecmd = self.modulecmd(cmd, *args)
         completed = osext.run_command(modulecmd)
         if re.search(r'ERROR', completed.stderr) is not None:
@@ -713,7 +730,7 @@ def name(self):
     def modulecmd(self, *args):
         return ' '.join(['modulecmd', 'python', *args])
 
-    def execute(self, cmd, *args):
+    def _execute(self, cmd, *args):
         modulecmd = self.modulecmd(cmd, *args)
         completed = osext.run_command(modulecmd, check=False)
         namespace = {}
@@ -874,7 +891,7 @@ def loaded_modules(self):
     def conflicted_modules(self, module):
         return []
 
-    def execute(self, cmd, *args):
+    def _execute(self, cmd, *args):
         return ''
 
     def load_module(self, module):

reframe/frontend/cli.py

@@ -770,6 +770,7 @@ def print_infoline(param, value):
         printer.debug('Loading user modules from command line')
         for m in site_config.get('general/0/user_modules'):
             try:
+                import pdb; pdb.set_trace()
                 rt.modules_system.load_module(**m, force=True)
             except errors.EnvironError as e:
                 printer.warning(

unittests/test_cli.py

@@ -599,15 +599,13 @@ def test_unuse_module_path(run_reframe, user_exec_ctx, monkeypatch):
     module_path = 'unittests/modules'
     monkeypatch.setenv('MODULEPATH', module_path)
     returncode, stdout, stderr = run_reframe(
-        more_options=[f'--module-path=-{module_path}', '-m testmod_foo'],
+        more_options=[f'--module-path=-{module_path}', '--module=testmod_foo'],
         config_file=fixtures.USER_CONFIG_FILE, action='run',
         system=rt.runtime().system.name
     )
-
-    # Here we test that ReFrame fails to run because 'testmod_foo' cannot
-    # be found and therefore the module name is included in the given error
-    assert 'testmod_foo' in stdout
-    assert returncode == 1
+    assert "could not load module 'testmod_foo' correctly" in stdout
+    assert 'Traceback' not in stderr
+    assert returncode == 0
 
 
 def test_use_module_path(run_reframe, user_exec_ctx):
@@ -617,13 +615,14 @@ def test_use_module_path(run_reframe, user_exec_ctx):
 
     module_path = 'unittests/modules'
     returncode, stdout, stderr = run_reframe(
-        more_options=[f'--module-path=+{module_path}', '-m testmod_foo'],
+        more_options=[f'--module-path=+{module_path}', '--module=testmod_foo'],
         config_file=fixtures.USER_CONFIG_FILE, action='run',
         system=rt.runtime().system.name
     )
 
     assert 'Traceback' not in stdout
     assert 'Traceback' not in stderr
+    assert "could not load module 'testmod_foo' correctly" not in stdout
     assert returncode == 0
 
 
@@ -634,13 +633,14 @@ def test_overwrite_module_path(run_reframe, user_exec_ctx):
 
     module_path = 'unittests/modules'
     returncode, stdout, stderr = run_reframe(
-        more_options=[f'--module-path={module_path}', '-m testmod_foo'],
+        more_options=[f'--module-path={module_path}', '--module=testmod_foo'],
         config_file=fixtures.USER_CONFIG_FILE, action='run',
         system=rt.runtime().system.name
     )
 
     assert 'Traceback' not in stdout
     assert 'Traceback' not in stderr
+    assert "could not load module 'testmod_foo' correctly" not in stdout
     assert returncode == 0
 
 

unittests/test_environments.py

@@ -258,7 +258,7 @@ def test_emit_loadenv_failure(user_runtime):
 
     # Suppress the module load error and verify that the original environment
     # is preserved
-    with contextlib.suppress(SpawnedProcessError):
+    with contextlib.suppress(EnvironError):
         rt.emit_loadenv_commands(environ)
 
     assert rt.snapshot() == snap

unittests/test_modules.py

@@ -80,7 +80,7 @@ def test_module_load(modules_system):
         modules_system.load_module('foo')
         modules_system.unload_module('foo')
     else:
-        with pytest.raises(SpawnedProcessError):
+        with pytest.raises(EnvironError):
             modules_system.load_module('foo')
 
         assert not modules_system.is_module_loaded('foo')
@@ -307,7 +307,7 @@ def loaded_modules(self):
         def conflicted_modules(self, module):
             return []
 
-        def execute(self, cmd, *args):
+        def _execute(self, cmd, *args):
             return ''
 
         def load_module(self, module):