Merge ppa-dev-tools:test-create-config into ppa-dev-tools:main

[Bryce Harrington]

This adds unit tests for the create_config() command, which I thought might be a bit superfluous but it did uncover some bad handling of invalid inputs. There's a few other testsuite related fixes and cleanups included.

Best way to test this branch is to run the full testsuite:

    $ pytest-3

The current codebase had a test failure relating to processors; this fixes that, and all subsequent commits should likewise pass the full testsuite.

[Alberto Contreras (aciba)]

Thanks, Bryce, for adding unit tests. The changes look good.

I have left some nits and two questions / concerns.

1) The main one is the inclusion of the mocking machinery in `Ppa.archive`.

This helps us to unit test the code and it is okay as is.

But, adds new code that it is not needed at runtime and could cause problems in the future. In this case, this is probably an exaggeration, as the modifications are simple and easy to track. But if we establish this pattern of allowing testing code within production code, things could get complicated and bite us at some point.

Have we thought about using other alternatives, as using `pytest.fixtures` and/or `unittests.mock` in the long term? Or are there restrictions to use those that I do not see?

2) The xfail comment.

review: Needs Information

[Athos Ribeiro (athos-ribeiro)]

Thanks Bryce!

Other than Alberto's questions/comments (in special, the XFAIL one), this LGTM :)

93aba18... by Bryce Harrington on 2023-03-03

tests: Create mock PPAs with the default processors enabled

[Bryce Harrington (bryce)]

Hi Alberto and Athos, thank you for reviewing! You're right the xfail can be dropped; I was using it to isolate some debugging work and appear to have forgotten to re-enable it.

Alberto, you're right that the _archive addition is a bit awkward. I took a fresh look this morning, and redid the code to avoid needing to add that, by creating the ArchiveMock just at the point of need, and avoid having to store it. For test code that should be fine, and it avoids impacting the run-time code, which could have side effects as you point out.

I also did the cleanups you suggested, squashed in the code review changes, and force pushed the branch. I think it looks a more solid, thank you both again for taking time to look at this.

[Athos Ribeiro (athos-ribeiro)]

LGTM.

I just did not get why we needed the dict typecast there. Is it to satisfy type checking?

review: Approve

[Bryce Harrington (bryce)]

Thanks! Answer to the question below.

[Bryce Harrington (bryce)]

Landed:

To git+ssh://git.launchpad.net/ppa-dev-tools
   555b089..7e59879 main -> main

diff --git a/ppa/ppa.py b/ppa/ppa.py
index dfbe5df..45e9886 100755
--- a/ppa/ppa.py
+++ b/ppa/ppa.py
@@ -97,6 +97,8 @@ class Ppa:
         :returns: The Launchpad archive object.
         :raises PpaDoesNotExist: Raised if a PPA does not exist in Launchpad.
         """
+        if not self._service:
+            raise AttributeError("Ppa object not connected to the Launchpad service")
         try:
             owner = self._service.people[self.team_name]
             return owner.getPPAByName(name=self.ppa_name)
diff --git a/scripts/ppa b/scripts/ppa
index 6be138e..ad6a09a 100755
--- a/scripts/ppa
+++ b/scripts/ppa
@@ -52,6 +52,7 @@ import time
 import argparse
 from inspect import currentframe
 from textwrap import indent
+from typing import Any
 from distro_info import UbuntuDistroInfo
 
 try:
@@ -111,7 +112,7 @@ def load_yaml_as_dict(filename):
         return d
 
 
-def add_global_options(parser) -> None:
+def add_global_options(parser: argparse.ArgumentParser) -> None:
     """Adds arguments to the given parser for generic options.
 
     :param argparse.ArgumentParser parser: A parser or subparser.
@@ -138,7 +139,7 @@ def add_global_options(parser) -> None:
                         help="Minimize output during processing")
 
 
-def add_basic_config_options(parser) -> None:
+def add_basic_config_options(parser: argparse.ArgumentParser) -> None:
     """Adds to a parser the command line options to configure the PPA.
 
     The config options are supported by the 'create' and 'set' command,
@@ -146,6 +147,8 @@ def add_basic_config_options(parser) -> None:
 
     These options represent what can be set as a user from the Launchpad
     web interface.
+
+    :param argparse.ArgumentParser parser: A parser or subparser.
     """
     # Architectures
     parser.add_argument(
@@ -203,7 +206,7 @@ def add_basic_config_options(parser) -> None:
     )
 
 
-def create_arg_parser():
+def create_arg_parser() -> argparse.ArgumentParser:
     """Sets up the command line parser object.
 
     :rtype: argparse.ArgumentParser
@@ -364,7 +367,14 @@ def create_arg_parser():
     return parser
 
 
-def create_config(lp, args):
+DEFAULT_CONFIG = {
+    'debug': False,
+    'ppa_name': None,
+    'team_name': None,
+    'wait_seconds': 10.0
+    }
+
+def create_config(lp: Lp, args: argparse.Namespace) -> dict[str, Any]:
     """Creates config object by loading from file and adding args.
 
     This routine merges the command line parameter values with data
@@ -380,19 +390,13 @@ def create_config(lp, args):
     :rtype: dict
     :returns: dict of configuration parameters and values, or None on error
     """
-    DEFAULT_CONFIG = {
-        'debug': False,
-        'ppa_name': None,
-        'team_name': None,
-        'wait_seconds': 10.0
-        }
     config_path = os.path.expanduser(args.config_filename)
     try:
         config = load_yaml_as_dict(config_path)
     except FileNotFoundError:
         # Assume defaults
         dbg("Using default config since no config file found at {}".format(config_path))
-        config = DEFAULT_CONFIG
+        config = dict(DEFAULT_CONFIG)
 
     # Map all non-empty elements from argparse Namespace into config dict
     config.update({k: v for k, v in vars(args).items() if v})
@@ -404,10 +408,13 @@ def create_config(lp, args):
     lp_username = None
     if lp.me:
         lp_username = lp.me.name
+    if not hasattr(args, 'ppa_name'):
+        warn("No ppa name given")
+        return None
+
     config['team_name'], config['ppa_name'] = ppa_address_split(args.ppa_name, lp_username)
     if not config['team_name'] or not config['ppa_name']:
-        warn("Invalid ppa name '{}'".format(args.ppa_name))
-        return None
+        raise ValueError("Invalid ppa name '{}'".format(args.ppa_name))
 
     if args.dry_run:
         config['dry_run'] = True
@@ -877,7 +884,7 @@ COMMANDS = {
     }
 
 
-def main(args):
+def main(args: argparse.Namespace) -> int:
     """Main entrypoint for the command.
 
     :param argparse.Namespace args: Command line arguments.
diff --git a/tests/helpers.py b/tests/helpers.py
index c6a55eb..07ad462 100644
--- a/tests/helpers.py
+++ b/tests/helpers.py
@@ -15,18 +15,32 @@ sys.path.insert(0, os.path.realpath(
     os.path.join(os.path.dirname(os.path.realpath(__file__)), "..")))
 
 from ppa.ppa import Ppa
+from ppa.constants import ARCHES_PPA_DEFAULT
 from ppa.ppa_group import PpaAlreadyExists
 
 
+class ProcessorMock:
+    """A stand-in for a Launchpad Processor object."""
+    def __init__(self, name):
+        self.name = name
+
+
 class ArchiveMock:
     """A stand-in for a Launchpad Archive object."""
     def __init__(self, name, description):
         self.displayname = name
         self.description = description
+        self.processors = [ProcessorMock(proc_name) for proc_name in ARCHES_PPA_DEFAULT]
 
     def lp_save(self):
         return True
 
+    def setProcessors(self, processors):
+        self.processors = []
+        for proc in processors:
+            proc_name = proc.split('/')[-1]
+            self.processors.append(ProcessorMock(proc_name))
+
 
 class PersonMock:
     """A stand-in for a Launchpad Person object."""
@@ -38,14 +52,13 @@ class PersonMock:
         for ppa in self._ppas:
             if ppa.name == name:
                 raise PpaAlreadyExists(name)
-        new_ppa = Ppa(name, self.name, description)
-        self._ppas.append(new_ppa)
+        self._ppas.append(Ppa(name, self.name, description))
         return True
 
     def getPPAByName(self, name):
         for ppa in self._ppas:
             if ppa.name == name:
-                return ArchiveMock(name, ppa.description)
+                return ArchiveMock(ppa.name, ppa.description)
         return None
 
     def lp_save(self):
diff --git a/tests/test_scripts_ppa.py b/tests/test_scripts_ppa.py
index 10acc30..40c7714 100644
--- a/tests/test_scripts_ppa.py
+++ b/tests/test_scripts_ppa.py
@@ -226,7 +226,7 @@ def test_create_arg_parser_basic_config(command):
     assert args.description == 'x'
     args.description = None
 
-    # Check --enable / -D|--disable
+    # Check --enable / --disable
     args = parser.parse_args([command, 'test-ppa', '--enable'])
     assert args.set_enabled is True
     args.set_enabled = False
@@ -355,27 +355,75 @@ def test_create_arg_parser_tests():
     args.show_urls = None
 
 
[e-mail](reason="Unimplemented")
-def test_create_config():
-    # args = []
-    # TODO: Set config_filename, ppa_name, team_name in args
-    # config = script.create_config(args)
[e-mail]('command_line_options, expected_config', [
+    pytest.param([], {}),
+    (['status', 'ppa:aa/bb'], {'command': 'status', 'team_name': 'aa', 'ppa_name': 'bb'}),
+    (['status', 'aa/bb'], {'team_name': 'aa', 'ppa_name': 'bb'}),
+    (['status', 'bb'], {'team_name': 'me', 'ppa_name': 'bb'}),
+    (['--debug', 'status', 'ppa:aa/bb'], {'debug': True}),
+    (['--dry-run', 'status', 'ppa:aa/bb'], {'dry_run': True}),
+    (['--verbose', 'status', 'ppa:aa/bb'], {'verbose': True}),
+    (['--quiet', 'status', 'ppa:aa/bb'], {'quiet': True}),
+    ])
+def test_create_config_from_args(command_line_options, expected_config):
+    '''Checks creation of a config object from an argparser object.
+
+    Prior tests cover the creation of proper args from the command line;
+    this test relies on the already-tested argparse machinery to create
+    various args to pass to create_config() in order to assure that the
+    right config dict is generated in response.
+    '''
+    lp = LpServiceMock()
+    parser = script.create_arg_parser()
+    args = parser.parse_args(command_line_options)
+    config = script.create_config(lp, args)
+
+    expected_result = script.DEFAULT_CONFIG
+    for key, value in expected_config.items():
+        assert key in config.keys()
+        assert config[key] == value
+
+
[e-mail]('args, expected_exception', [
+    # Bad command
+    ([None], SystemExit),
+    ([''], SystemExit),
+    (['INVALID'], SystemExit),
+    ([1], TypeError),
+
+    # Bad ppa name
+    (['status'], SystemExit),
+    (['status', None], ValueError),
+    (['status', ''], ValueError),
+    (['status', 'INVALID'], ValueError),
+    (['status', 1], TypeError),
+
+    # Bad argument name
+    (['--invalid', 'status', 'ppa:aa/bb'], SystemExit),
+    ])
+def test_create_config_from_args_error(args, expected_exception):
+    '''Checks creation of a config object from an argparser object.'''
+    lp = LpServiceMock()
+    parser = script.create_arg_parser()
 
-    pass
+    with pytest.raises(expected_exception):
+        args = parser.parse_args(args)
+        script.create_config(lp, args)
 
 
 def test_command_create(fake_config, monkeypatch):
     lp = LpServiceMock()
     monkeypatch.setattr("sys.stdin", io.StringIO('test description'))
-    assert script.command_create(lp, fake_config) == 0
 
+    assert script.command_create(lp, fake_config) == 0
     the_ppa = lp.me.getPPAByName(fake_config['ppa_name'])
+    assert the_ppa
 
     # Check basic attributes (further coverage is in test_ppa_group.py)
     assert the_ppa.displayname == fake_config['ppa_name']
     assert the_ppa.description == 'test description'
-
-    # TODO: Check processors
+    assert the_ppa.processors
+    assert type(the_ppa.processors) is list
 
 
 @pytest.mark.xfail(reason="Unimplemented")