CLI: fix flaky verdi daemon start command tests

The `verdi daemon start` command tests that define a specific number of
workers, either through a CLI option or through the config, were flaky
and failing. The probably cause was that the tests were changing the
configuration during the test, which was changing the actual
configuration and changes were not undone after the tests. These changes
can therefore interfere with other tests.

To fix it, the tests are converted to `pytest` style and the tests that
manipulate the config use a new fixture that creates a clone of the
config before running the test and making sure to restore the original
config after the test was done. In addition, it monkeypatches the
`Config` class to make sure it doesn't write backups to disk when it is
changed which is the default behavior but is not desirable during
testing.

Finally, a test is added for `verdi daemon status`. It had been broken
due to a recent refactor in the CLI logging code but went unnoticed
since it is not tested. The actual bug was fixed in the previous commit.

aiida/cmdline/commands/cmd_daemon.py

@@ -61,7 +61,7 @@ def start(foreground, number):
 
     client = get_daemon_client()
 
-    echo.echo('Starting the daemon... ', nl=False)
+    echo.echo(f'Starting the daemon with {number} workers... ', nl=False)
 
     if foreground:
         command = ['verdi', '-p', client.profile.name, 'daemon', _START_CIRCUS_COMMAND, '--foreground', str(number)]

tests/cmdline/commands/conftest.py

@@ -13,8 +13,6 @@
 import click
 import pytest
 
-from aiida.cmdline.commands.cmd_verdi import VerdiCommandGroup
-
 
 @pytest.fixture
 def run_cli_command():
@@ -35,6 +33,14 @@ def _run_cli_command(command: click.Command, options: list = None, raises: bool
         """
         import traceback
 
+        from aiida.cmdline.commands.cmd_verdi import VerdiCommandGroup
+        from aiida.common import AttributeDict
+        from aiida.manage.configuration import get_config, get_profile
+
+        config = get_config()
+        profile = get_profile()
+        obj = AttributeDict({'config': config, 'profile': profile})
+
         # Convert any ``pathlib.Path`` objects in the ``options`` to their absolute filepath string representation.
         # This is necessary because the ``invoke`` command does not support these path objects.
         options = [str(option) if isinstance(option, pathlib.Path) else option for option in options or []]
@@ -45,7 +51,7 @@ def _run_cli_command(command: click.Command, options: list = None, raises: bool
         command = VerdiCommandGroup.add_verbosity_option(command)
 
         runner = click.testing.CliRunner()
-        result = runner.invoke(command, options)
+        result = runner.invoke(command, options, obj=obj)
 
         if raises:
             assert result.exception is not None, result.output

tests/cmdline/commands/test_daemon.py

@@ -7,138 +7,88 @@
 # For further information on the license, see the LICENSE.txt file        #
 # For further information please visit http://www.aiida.net               #
 ###########################################################################
-"""Tests for `verdi daemon`."""
-from click.testing import CliRunner
+# pylint: disable=redefined-outer-name
+"""Tests for ``verdi daemon``."""
 import pytest
 
-from aiida.backends.testbase import AiidaTestCase
 from aiida.cmdline.commands import cmd_daemon
-from aiida.common.extendeddicts import AttributeDict
-from aiida.engine.daemon.client import DaemonClient
-from aiida.manage.configuration import get_config
-
-
-class VerdiRunner(CliRunner):
-    """Subclass of `click`'s `CliRunner` that injects an object in the context containing current config and profile."""
-
-    def __init__(self, config, profile, **kwargs):
-        """Construct an instance and define the `obj` dictionary that is required by the `Context`."""
-        super().__init__(**kwargs)
-        self.obj = AttributeDict({'config': config, 'profile': profile})
 
-    def invoke(self, *args, **extra):  # pylint: disable=signature-differs
-        """Invoke the command but add the `obj` to the `extra` keywords.
 
-        The `**extra` keywords will be forwarded all the way to the `Context` that finally invokes the command. Some
-        `verdi` commands will rely on this `obj` to be there to retrieve the current active configuration and profile.
-        """
-        extra['obj'] = self.obj
-        return super().invoke(*args, **extra)
+def test_daemon_start(run_cli_command, daemon_client):
+    """Test ``verdi daemon start``."""
+    run_cli_command(cmd_daemon.start)
 
+    daemon_response = daemon_client.get_daemon_info()
+    worker_response = daemon_client.get_worker_info()
 
-class TestVerdiDaemon(AiidaTestCase):
-    """Tests for `verdi daemon` commands."""
-
-    def setUp(self):
-        super().setUp()
-        self.config = get_config()
-        self.profile = self.config.current_profile
-        self.daemon_client = DaemonClient(self.profile)
-        self.cli_runner = VerdiRunner(self.config, self.profile)
-
-    def test_daemon_start(self):
-        """Test `verdi daemon start`."""
-        try:
-            result = self.cli_runner.invoke(cmd_daemon.start, [])
-            self.assertClickResultNoException(result)
-
-            daemon_response = self.daemon_client.get_daemon_info()
-            worker_response = self.daemon_client.get_worker_info()
-
-            self.assertIn('status', daemon_response, daemon_response)
-            self.assertEqual(daemon_response['status'], 'ok', daemon_response)
-
-            self.assertIn('info', worker_response, worker_response)
-            self.assertEqual(len(worker_response['info']), 1, worker_response)
-        finally:
-            self.daemon_client.stop_daemon(wait=True)
-
-    def test_daemon_restart(self):
-        """Test `verdi daemon restart` both with and without `--reset` flag."""
-        try:
-            result = self.cli_runner.invoke(cmd_daemon.start, [])
-            self.assertClickResultNoException(result)
-
-            result = self.cli_runner.invoke(cmd_daemon.restart, [])
-            self.assertClickResultNoException(result)
-
-            result = self.cli_runner.invoke(cmd_daemon.restart, ['--reset'])
-            self.assertClickResultNoException(result)
-
-            daemon_response = self.daemon_client.get_daemon_info()
-            worker_response = self.daemon_client.get_worker_info()
-
-            self.assertIn('status', daemon_response, daemon_response)
-            self.assertEqual(daemon_response['status'], 'ok', daemon_response)
-
-            self.assertIn('info', worker_response, worker_response)
-            self.assertEqual(len(worker_response['info']), 1, worker_response)
-        finally:
-            self.daemon_client.stop_daemon(wait=True)
-
-    # Tracked in issue #3051
-    @pytest.mark.flaky(reruns=2)
-    def test_daemon_start_number(self):
-        """Test `verdi daemon start` with a specific number of workers."""
-
-        # The `number` argument should be a positive non-zero integer
-        for invalid_number in ['string', 0, -1]:
-            result = self.cli_runner.invoke(cmd_daemon.start, [str(invalid_number)])
-            self.assertIsNotNone(result.exception)
-
-        try:
-            number = 4
-            result = self.cli_runner.invoke(cmd_daemon.start, [str(number)])
-            self.assertClickResultNoException(result)
-
-            daemon_response = self.daemon_client.get_daemon_info()
-            worker_response = self.daemon_client.get_worker_info()
-
-            self.assertIn('status', daemon_response, daemon_response)
-            self.assertEqual(daemon_response['status'], 'ok', daemon_response)
-
-            self.assertIn('info', worker_response, worker_response)
-            self.assertEqual(len(worker_response['info']), number, worker_response)
-        finally:
-            self.daemon_client.stop_daemon(wait=True)
-
-    # Tracked in issue #3051
-    @pytest.mark.flaky(reruns=2)
-    def test_daemon_start_number_config(self):
-        """Test `verdi daemon start` with `daemon.default_workers` config option being set."""
-        number = 3
-        self.config.set_option('daemon.default_workers', number, self.profile.name)
-
-        try:
-            result = self.cli_runner.invoke(cmd_daemon.start)
-            self.assertClickResultNoException(result)
-
-            daemon_response = self.daemon_client.get_daemon_info()
-            worker_response = self.daemon_client.get_worker_info()
-
-            self.assertIn('status', daemon_response, daemon_response)
-            self.assertEqual(daemon_response['status'], 'ok', daemon_response)
-
-            self.assertIn('info', worker_response, worker_response)
-            self.assertEqual(len(worker_response['info']), number, worker_response)
-        finally:
-            self.daemon_client.stop_daemon(wait=True)
-
-    def test_foreground_multiple_workers(self):
-        """Test `verdi daemon start` in foreground with more than one worker will fail."""
-        try:
-            number = 4
-            result = self.cli_runner.invoke(cmd_daemon.start, ['--foreground', str(number)])
-            self.assertIsNotNone(result.exception)
-        finally:
-            self.daemon_client.stop_daemon(wait=True)
+    assert 'status' in daemon_response
+    assert daemon_response['status'] == 'ok'
+
+    assert 'info' in worker_response
+    assert len(worker_response['info']) == 1
+
+
+def test_daemon_restart(run_cli_command, daemon_client):
+    """Test ``verdi daemon restart`` both with and without ``--reset`` flag."""
+    run_cli_command(cmd_daemon.start, [])
+    run_cli_command(cmd_daemon.restart, [])
+    run_cli_command(cmd_daemon.restart, ['--reset'])
+
+    daemon_response = daemon_client.get_daemon_info()
+    worker_response = daemon_client.get_worker_info()
+
+    assert 'status' in daemon_response
+    assert daemon_response['status'] == 'ok'
+
+    assert 'info' in worker_response
+    assert len(worker_response['info']) == 1
+
+
+def test_daemon_start_number(run_cli_command, daemon_client):
+    """Test ``verdi daemon start`` with a specific number of workers."""
+    number = 4
+    run_cli_command(cmd_daemon.start, [str(number)])
+
+    daemon_response = daemon_client.get_daemon_info()
+    worker_response = daemon_client.get_worker_info()
+
+    assert 'status' in daemon_response
+    assert daemon_response['status'] == 'ok'
+
+    assert 'info' in worker_response
+    assert len(worker_response['info']) == number
+
+
+def test_daemon_start_number_config(run_cli_command, daemon_client, isolated_config):
+    """Test ``verdi daemon start`` with ``daemon.default_workers`` config option being set."""
+    number = 3
+    isolated_config.set_option('daemon.default_workers', number, scope=isolated_config.current_profile.name)
+    isolated_config.store()
+
+    run_cli_command(cmd_daemon.start)
+
+    daemon_response = daemon_client.get_daemon_info()
+    worker_response = daemon_client.get_worker_info()
+
+    assert 'status' in daemon_response
+    assert daemon_response['status'] == 'ok'
+
+    assert 'info' in worker_response
+    assert len(worker_response['info']) == number
+
+
+@pytest.mark.usefixtures('daemon_client')
+def test_foreground_multiple_workers(run_cli_command):
+    """Test `verdi daemon start` in foreground with more than one worker will fail."""
+    run_cli_command(cmd_daemon.start, ['--foreground', str(4)], raises=True)
+
+
+@pytest.mark.usefixtures('daemon_client')
+def test_daemon_status(run_cli_command, isolated_config):
+    """Test ``verdi daemon status``."""
+    run_cli_command(cmd_daemon.start)
+    result = run_cli_command(cmd_daemon.status)
+    last_line = result.output_lines[-1]
+
+    assert f'Profile: {isolated_config.current_profile.name}' in result.output
+    assert last_line == 'Use verdi daemon [incr | decr] [num] to increase / decrease the amount of workers'

tests/conftest.py

@@ -151,6 +151,32 @@ def _generate_calculation_node(process_state=ProcessState.FINISHED, exit_status=
     return _generate_calculation_node
 
 
+@pytest.fixture
+def isolated_config(monkeypatch):
+    """Return a copy of the currently loaded config and set that as the loaded config.
+
+    This allows a test to change the config during the test, and after the test the original config will be restored
+    making the changes of the test fully temporary. In addition, the ``Config._backup`` method is monkeypatched to be a
+    no-op in order to prevent that backup files are written to disk when the config is stored. Storing the config after
+    changing it in tests maybe necessary if a command is invoked that will be reading the config from disk in another
+    Python process and so doesn't have access to the loaded config in memory in the process that is running the test.
+    """
+    import copy
+    from aiida.manage import configuration
+
+    monkeypatch.setattr(configuration.Config, '_backup', lambda *args, **kwargs: None)
+
+    current_config = configuration.CONFIG
+    configuration.CONFIG = copy.deepcopy(current_config)
+    configuration.CONFIG.set_default_profile(configuration.PROFILE.name, overwrite=True)
+
+    try:
+        yield configuration.CONFIG
+    finally:
+        configuration.CONFIG = current_config
+        configuration.CONFIG.store()
+
+
 @pytest.fixture
 def empty_config(tmp_path) -> Config:
     """Create a temporary configuration instance.
@@ -367,6 +393,20 @@ def with_daemon():
     os.kill(daemon.pid, signal.SIGTERM)
 
 
+@pytest.fixture
+def daemon_client():
+    """Return a daemon client instance and stop any daemon instances running for the test profile after the test."""
+    from aiida.engine.daemon.client import DaemonClient
+    from aiida.manage.configuration import get_profile
+
+    client = DaemonClient(get_profile())
+
+    try:
+        yield client
+    finally:
+        client.stop_daemon(wait=True)
+
+
 @pytest.fixture(scope='function')
 def chdir_tmp_path(request, tmp_path):
     """Change to a temporary directory before running the test and reverting to original working directory."""