Improve parse_args tests (#580)

* Improve parse_args tests

Make the tests more precise, so we can be more confident
that the tests are verifying the expected cases.

Also fix the docs for supported datetime format for --purge-before-date

cou/cli.py

@@ -245,8 +245,14 @@ async def _run_command(args: CLIargs) -> None:
 
 
 def entrypoint() -> None:
-    """Execute 'charmed-openstack-upgrade' command."""
+    """Run the cli app.
+
+    Only this function and parse_args() should call sys.exit.
+    Other functions that find a blocking error should raise an Exception
+    and let it be handled by this function.
+    """
     args = parse_args(sys.argv[1:])
+
     log_file: Optional[Path] = None
     try:
         # disable progress indicator when in quiet mode to suppress its console output

cou/commands.py

@@ -125,7 +125,7 @@ def purge_before_arg(value: str) -> str:
             return value
         except ValueError:
             pass
-    raise argparse.ArgumentTypeError("purge before format must be YYYY-MM-DD[HH:mm][:ss]")
+    raise argparse.ArgumentTypeError("format must be YYYY-MM-DD[ HH:mm[:ss]]")
 
 
 def get_subcommand_common_opts_parser() -> argparse.ArgumentParser:
@@ -197,7 +197,7 @@ def get_subcommand_common_opts_parser() -> argparse.ArgumentParser:
         help=(
             "Providing this argument will delete data from all shadow tables"
             "\nthat is older than the date provided."
-            "\nDate string format should be YYYY-MM-DD[HH:mm][:ss]."
+            "\nDate string format should be YYYY-MM-DD[ HH:mm[:ss]]."
             "\nWithout --purge-before-date the purge step will delete all the data."
             "\nThis option requires --purge."
         ),
@@ -502,14 +502,17 @@ def prompt(self) -> bool:
         return not self.auto_approve
 
 
-def parse_args(args: Any) -> CLIargs:  # pylint: disable=inconsistent-return-statements
+def parse_args(args: list[str]) -> CLIargs:  # pylint: disable=inconsistent-return-statements
     """Parse cli arguments.
 
-    :param args: Arguments parser.
-    :type args: Any
+    Calls sys.exit via the argparse methods
+    if there are errors with the arguments,
+    or a help command is used.
+
+    :param args: List of arguments to parse
+    :type args: list[str]
     :return: CLIargs custom object.
     :rtype: CLIargs
-    :raises argparse.ArgumentError: Unexpected arguments input.
     """
     # Configure top level argparser and its options
     parser = argparse.ArgumentParser(

cou/steps/nova_cloud_controller.py

@@ -76,7 +76,7 @@ async def purge(model: Model, apps: Sequence[Application], before: Optional[str]
     :type apps: Sequence of Applications
     :param before: specifying before will delete data from all shadow tables
         that is older than the data provided.
-        Date string format should be YYYY-MM-DD[HH:mm][:ss]
+        Date string format should be YYYY-MM-DD[ HH:mm[:ss]]
     :raises COUException: if action returned unexpected output or failed
     """
     action_params = {}

docs/how-to/purge-data-on-shadow-table.rst

@@ -7,7 +7,7 @@ This can be enabled with the ``--purge`` flag.
 
 This purge step is a performance optimisation, delete data from the shadow tables in nova database. The behaviour is equal to run juju action ``purge`` on nova-cloud-controller unit, which help to reduce the size of the database, make queries faster, backups efficiency, and follow the data retention policies.
 
-The ``purge-before-date`` flag is supported to delete the data older than the date provided. The date string format should be ``YYYY-MM-DD[HH:mm][:ss]``. This flag is useful to retain recent data for debugging or data retention policies.
+The ``--purge-before-date`` flag is supported to delete the data older than the date provided. The date string format should be ``YYYY-MM-DD[ HH:mm[:ss]]``. This flag is useful to retain recent data for debugging or data retention policies.
 
 
 Usage examples

tests/unit/test_commands.py

@@ -13,7 +13,7 @@
 # limitations under the License.
 
 from argparse import ArgumentParser, ArgumentTypeError
-from unittest.mock import patch
+from unittest.mock import ANY, patch
 
 import pytest
 
@@ -46,28 +46,45 @@ def test_CLIargs_prompt(auto_approve, expected_result):
         ["upgrade", "hypervisors", "-h"],
     ],
 )
-def test_parse_args_help(args):
+# NOTE: When we update to use python > 3.10,
+# use the wraps arg to patch (`wraps=ArgumentParser.exit`)
+# and remove the side_effect to the exit mock.
+# Likewise for patching ArgumentParser.error.
+# With python <= 3.10, we can't use autospec with wraps:
+# https://github.com/python/cpython/issues/75988
+@patch(
+    "cou.commands.argparse.ArgumentParser.print_help",
+    autospec=True,
+)
+@patch("cou.commands.argparse.ArgumentParser.exit", autospec=True)
+def test_parse_args_help(mock_exit, mock_print_help, args):
     """Test printing help messages."""
-    with patch(
-        "cou.commands.argparse.ArgumentParser.print_help"
-    ) as mock_print_help, pytest.raises(SystemExit, match="0"):
+    mock_exit.side_effect = SystemExit
+    with pytest.raises(SystemExit):
         commands.parse_args(args)
-        mock_print_help.assert_called_once()
+    mock_print_help.assert_called_once()
+    mock_exit.assert_called_once_with(ANY)
 
 
 @pytest.mark.parametrize(
     "args",
     [
-        ["--quiet", "--verbose"],
-        ["--quiet", "-v"],
-        ["--quiet", "-vvv"],
+        ["plan", "--quiet", "--verbose"],
+        ["upgrade", "--quiet", "-v"],
+        ["plan", "--quiet", "-vvv"],
     ],
 )
-def test_parse_args_quiet_verbose_exclusive(args):
+@patch("cou.commands.argparse.ArgumentParser.error", autospec=True)
+def test_parse_args_quiet_verbose_exclusive(mock_error, args):
     """Test that quiet and verbose options are mutually exclusive."""
-    with pytest.raises(SystemExit, match="2"):
+    mock_error.side_effect = SystemExit
+    with pytest.raises(SystemExit):
         commands.parse_args(args)
 
+    mock_error.assert_called_once_with(
+        ANY, "argument --verbose/-v: not allowed with argument --quiet/-q"
+    )
+
 
 @pytest.mark.parametrize(
     "args, expected_CLIargs",
@@ -822,22 +839,32 @@ def test_parse_args_upgrade(args, expected_CLIargs):
     assert parsed_args == expected_CLIargs
 
 
-@pytest.mark.parametrize(
-    "args",
-    [
-        ["upgrade", "hypervisors", "--machine 1", "--az 2"],
-        ["upgrade", "hypervisors", "--az 1", "-m 2"],
-    ],
-)
-def test_parse_args_hypervisors_exclusive_options(args):
+@patch("cou.commands.argparse.ArgumentParser.error", autospec=True)
+def test_parse_args_hypervisors_exclusive_options(mock_error):
     """Test parsing mutually exclusive hypervisors specific options."""
-    with pytest.raises(SystemExit, match="2"):
-        commands.parse_args(args)
+    mock_error.side_effect = SystemExit
+    with pytest.raises(SystemExit):
+        commands.parse_args(["upgrade", "hypervisors", "--machine", "1", "--az", "2"])
+    mock_error.assert_called_once_with(
+        ANY, "argument --availability-zone/--az: not allowed with argument --machine/-m"
+    )
+
+
+@patch("cou.commands.argparse.ArgumentParser.error", autospec=True)
+def test_parse_args_hypervisors_exclusive_options_reverse_order(mock_error):
+    """Test parsing mutually exclusive hypervisors specific options (reverse order)."""
+    mock_error.side_effect = SystemExit
+    with pytest.raises(SystemExit):
+        commands.parse_args(["upgrade", "hypervisors", "--az", "1", "-m", "2"])
+    mock_error.assert_called_once_with(
+        ANY, "argument --machine/-m: not allowed with argument --availability-zone/--az"
+    )
 
 
 @pytest.mark.parametrize(
     "args",
     [
+        ["foo"],
         ["upgrade", "--archive-batch-size", "asdf"],
         ["plan", "--archive-batch-size", "asdf"],
         ["upgrade", "--archive-batch-size", "-4"],
@@ -860,18 +887,21 @@ def test_parse_invalid_args(args):
 @pytest.mark.parametrize(
     "args",
     [
-        ["foo"],
         ["--bar"],
         ["plan", "data-plane", "--machine 1"],
         ["plan", "data-plane", "--availability-zone zone-1"],
         ["upgrade", "data-plane", "--machine 1"],
         ["upgrade", "data-plane", "--availability-zone zone-1"],
     ],
 )
-def test_parse_args_raise_exception(args):
+@patch("cou.commands.argparse.ArgumentParser.error", autospec=True)
+def test_parse_args_raise_exception(mock_error, args):
     """Test parsing unknown arguments."""
-    with pytest.raises(SystemExit, match="2"):
+    mock_error.side_effect = SystemExit
+    with pytest.raises(SystemExit):
         commands.parse_args(args)
+    mock_error.assert_called_once()
+    assert "unrecognized arguments" in mock_error.call_args[0][1]
 
 
 def test_capitalize_usage_prefix():
@@ -904,49 +934,54 @@ def test_capitalize_help_message():
 
 
 @pytest.mark.parametrize(
-    "val,raise_err",
+    "val",
     [
-        (
-            "2000-01-02",
-            False,
-        ),
-        (
-            "2000-01-02 03:04",
-            False,
-        ),
-        (
-            "2000-01-02 03:04:05",
-            False,
-        ),
-        (
-            "2000-01-02 03:04:05 something-wrong",
-            True,
-        ),
+        "2000-01-0203:04",
+        "2000-01-0203:04:05",
+        "2000-01-02 03:04:05 something-wrong",
     ],
 )
-def test_purge_before_arg(val, raise_err):
-    """Test purge_before_arg."""
-    if not raise_err:
-        result = commands.purge_before_arg(val)
-        assert val == result
-    else:
-        with pytest.raises(ArgumentTypeError):
-            commands.purge_before_arg(val)
+def test_purge_before_arg_invalid(val):
+    """Verify --purge-before validator handles error cases."""
+    with pytest.raises(ArgumentTypeError):
+        commands.purge_before_arg(val)
 
 
+@pytest.mark.parametrize(
+    "val",
+    [
+        "2000-01-02",
+        "2000-01-02 03:04",
+        "2000-01-02 03:04:05",
+    ],
+)
+def test_purge_before_arg_valid(val):
+    """Verify --purge-before validator handles valid cases."""
+    result = commands.purge_before_arg(val)
+    assert val == result
+
+
+@patch("cou.commands.argparse.ArgumentParser.error", autospec=True)
 @patch("cou.commands.setattr")
-def test_purge_before_argument_missing_dependency(mock_setattr):
-    with pytest.raises(SystemExit, match="2"):
-        commands.parse_args("plan --purge-before-date 2000-01-02".split())
+def test_purge_before_argument_missing_dependency(mock_setattr, mock_error):
+    mock_error.side_effect = SystemExit
+    with pytest.raises(SystemExit):
+        commands.parse_args(["plan", "--purge-before-date", "2000-01-02"])
+    mock_error.assert_called_once_with(ANY, "\n--purge-before-date requires --purge")
 
 
 @patch("cou.commands.setattr")
 def test_skip_apps(mock_setattr):
-    args = commands.parse_args("upgrade --skip-apps vault vault vault".split())
+    args = commands.parse_args(["upgrade", "--skip-apps", "vault", "vault", "vault"])
     args.skip_apps == ["vault", "vault", "vault"]
 
 
+@patch("cou.commands.argparse.ArgumentParser.error", autospec=True)
 @patch("cou.commands.setattr")
-def test_skip_apps_failed(mock_setattr):
-    with pytest.raises(SystemExit, match="2"):
-        commands.parse_args("upgrade --skip-apps vault keystone".split())
+def test_skip_apps_failed(mock_setattr, mock_error):
+    mock_error.side_effect = SystemExit
+    with pytest.raises(SystemExit):
+        commands.parse_args(["upgrade", "--skip-apps", "vault", "keystone"])
+    mock_error.assert_called_once_with(
+        ANY, "argument --skip-apps: invalid choice: 'keystone' (choose from 'vault')"
+    )