CLI: fix bug in the CliFormatter log utility

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)]

aiida/cmdline/utils/log.py

@@ -51,7 +51,15 @@ def format(record):
         except KeyError:
             fg = 'white'
 
-        if record.prefix:
+        try:
+            prefix = record.prefix
+        except AttributeError:
+            prefix = None
+
+        if prefix:
             return f'{click.style(record.levelname.capitalize(), fg=fg, bold=True)}: {record.msg % record.args}'
 
-        return f'{record.msg % record.args}'
+        if record.args:
+            return f'{record.msg % record.args}'
+
+        return record.msg

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/cmdline/utils/test_log.py

@@ -0,0 +1,43 @@
+# -*- coding: utf-8 -*-
+###########################################################################
+# Copyright (c), The AiiDA team. All rights reserved.                     #
+# This file is part of the AiiDA code.                                    #
+#                                                                         #
+# The code is hosted on GitHub at https://github.com/aiidateam/aiida-core #
+# For further information on the license, see the LICENSE.txt file        #
+# For further information please visit http://www.aiida.net               #
+###########################################################################
+"""Tests for the :mod:`aiida.cmdline.utils.log` module."""
+import logging
+
+from aiida.cmdline.utils import log
+
+
+def test_cli_formatter():
+    """Test the ``CliFormatter.format`` method for a plain message.
+
+    Note that if it contains percentage signs but no arguments, it should not try to convert it.
+    """
+    message = 'message'
+    record = logging.LogRecord('name', logging.INFO, 'pathname', 0, message, (), 'exc_info')
+    assert log.CliFormatter.format(record) == message
+
+
+def test_cli_formatter_no_args():
+    """Test the ``CliFormatter.format`` method for a message with percentage signs but no args."""
+    message = 'PID    MEM %    CPU %  started'
+    record = logging.LogRecord('name', logging.INFO, 'pathname', 0, message, (), 'exc_info')
+    assert log.CliFormatter.format(record) == message
+
+
+def test_cli_formatter_args():
+    """Test the ``CliFormatter.format`` method for a message with a single argument."""
+    record = logging.LogRecord('name', logging.INFO, 'pathname', 0, 'Some %s', ('value',), 'exc_info')
+    assert log.CliFormatter.format(record) == 'Some value'
+
+
+def test_cli_formatter_prefix():
+    """Test the ``CliFormatter.format`` method for a message with a single argument."""
+    record = logging.LogRecord('name', logging.INFO, 'pathname', 0, 'Some %s', ('value',), 'exc_info')
+    record.prefix = True
+    assert log.CliFormatter.format(record) == '\x1b[34m\x1b[1mInfo\x1b[0m: Some value'

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."""