CLI: fix bug in the CliFormatter log utility #5107
Conversation
|
@chrisjsewell |
Codecov Report
@@ Coverage Diff @@
## develop #5107 +/- ##
===========================================
+ Coverage 80.71% 80.74% +0.04%
===========================================
Files 534 534
Lines 36966 36972 +6
===========================================
+ Hits 29834 29851 +17
+ Misses 7132 7121 -11
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
except the dependencies have changed: importlib-metadata -> 4.8.1 |
sphuber
force-pushed
the
fix/3051/verdi-daemon-tests
branch
from
4967a38 to
e7dd091
Compare
|
@chrisjsewell I temporarily fixed the mypy issue on the other PR, so this is now ready to go |
sphuber
force-pushed
the
fix/3051/verdi-daemon-tests
branch
from
e7dd091 to
330c216
Compare
tests/cmdline/utils/test_log.py
Outdated
| 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 = 'Prefix' |
There was a problem hiding this comment.
Out of interest, what's the deal with setting 'Prefix' here. The actual prefix is the log level, so what relevance does that string value have? Feels like it should just be set to true
There was a problem hiding this comment.
Yep, you are right, it should just be a boolean even though this also works since a string is truthy :) Changed it for consistency
The `format` method assumed that the `prefix` attribute would always exist, which is not always this case. This would result in an uncaught exception when a message would get logged. This went unnoticed since the utility was not individually tested. Additionally, messages with literal percentage signs would also except because it would always be interpolated with the `record.args` but if that was an empty tuple, a `TypeError` would be raised.
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.
sphuber
force-pushed
the
fix/3051/verdi-daemon-tests
branch
from
330c216 to
c75df65
Compare
|
Thanks @chrisjsewell . Addressed the comments so this should be good to go. This should be rebased merged and not squashed |
Fixes #3051
This PR fixes a long standing open issue of flaky
verdi daemon starttests, essentially by converting them topyteststyle and properly isolating changes to the config done by the tests.This work was triggered because
verdi daemon statuswas broken after the recent refactoring of CLI logging and there are no tests for it. The actual underlying bug in the CLI logging utils has been addressed in a separate commit.