Merge ppa-dev-tools:refactor-autopkgtest-results into ppa-dev-tools:main

[Bryce Harrington]

Refactors logic out of the tests command relating to Autopkgtest results into a new
get_autopkgtest_results() routine, with a corresponding test case.

The test case was the most challenging part, but I feel its finally adequate. I plan to add some functionality for filtering and refining the test results displayed when running 'ppa tests <ppa>' so that you can limit to particular packages, releases, etc. However, before getting into that I wanted a solid test to base the work on. In implementing this I can see some additional refactoring that could make the test more concise and narrowly focused, however I'll leave that to the planned followup work.

In addition to this commit, I'm including several testing-related refinements that feel like they go along with this.

[Andreas Hasenack (ahasenack)]

+1, just two minor optional comments.

review: Approve

[Bryce Harrington (bryce)]

Thanks, updated and landed:

stirling: ~/src/PpaDevTools/ppa-dev-tools-feature-rdepends-testing$ git merge --ff-only refactor-autopkgtest-results
Updating c56a856..6c63268
Fast-forward
 ppa/ppa.py | 25 +++++++++++++++++++++++++
 ppa/result.py | 2 +-
 scripts/ppa | 38 ++++++++++++++------------------------
 tests/test_ppa.py | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 tests/test_result.py | 9 ++++++++-
 5 files changed, 126 insertions(+), 31 deletions(-)
stirling: ~/src/PpaDevTools/ppa-dev-tools-feature-rdepends-testing$ git push
Total 0 (delta 0), reused 0 (delta 0), pack-reused 0
To git+ssh://git.launchpad.net/ppa-dev-tools
   c56a856..6c63268 main -> main

diff --git a/ppa/ppa.py b/ppa/ppa.py
index e055dc4..57ec14b 100755
--- a/ppa/ppa.py
+++ b/ppa/ppa.py
@@ -22,6 +22,7 @@ from lazr.restfulclient.errors import BadRequest, NotFound, Unauthorized
 from .constants import URL_AUTOPKGTEST
 from .io import open_url
 from .job import (get_waiting, get_running)
+from .result import get_results
 
 
 class PendingReason(enum.Enum):
@@ -555,6 +556,30 @@ class Ppa:
             return get_running(response, releases=releases, ppa=str(self))
         return []
 
+    def get_autopkgtest_results(self, releases, architectures):
+        """Returns iterator of results from autopkgtest runs for this PPA.
+
+        See get_results() for details
+
+        :param list[str] releases: The Ubuntu series codename(s), or None.
+        :param list[str] architectures: The hardware architectures.
+        :rtype: Iterator[dict]
+        :returns: Autopkgtest results, if any, or an empty list on error
+        """
+        results = []
+        for release in releases:
+            base_results_fmt = f"{URL_AUTOPKGTEST}/results/autopkgtest-%s-%s-%s/"
+            base_results_url = base_results_fmt % (release, self.owner_name, self.name)
+            response = open_url(f"{base_results_url}?format=plain")
+            if response:
+                trigger_sets = {}
+                for result in get_results(response, base_results_url, arches=architectures):
+                    trigger = ', '.join([str(r) for r in result.get_triggers()])
+                    trigger_sets.setdefault(trigger, [])
+                    trigger_sets[trigger].append(result)
+                results.append(trigger_sets)
+        return results
+
 
 def ppa_address_split(ppa_address):
     """Parse an address for a PPA into its owner and name components.
diff --git a/ppa/result.py b/ppa/result.py
index fe47621..d0afa95 100755
--- a/ppa/result.py
+++ b/ppa/result.py
@@ -340,7 +340,7 @@ if __name__ == "__main__":
     print("Loading live excuses data")
     print("-------------------------")
     base_results_fmt = f"{URL_AUTOPKGTEST}/results/autopkgtest-%s-%s-%s/"
-    base_results_url = base_results_fmt % ('kinetic', 'bryce', 'nginx-merge-v1.22.0-1')
+    base_results_url = base_results_fmt % ('mantic', 'bryce', 'apache2-merge-v2.4.54-3')
     url = f"{base_results_url}?format=plain"
     response = open_url(url)
 
diff --git a/scripts/ppa b/scripts/ppa
index c4af301..9dbb15a 100755
--- a/scripts/ppa
+++ b/scripts/ppa
@@ -71,12 +71,10 @@ from ppa.constants import (
     ARCHES_PPA_ALL,
     ARCHES_PPA_DEFAULT,
     ARCHES_AUTOPKGTEST,
-    URL_AUTOPKGTEST,
     LOCAL_REPOSITORY_PATH,
     LOCAL_REPOSITORY_MIRRORING_DIRECTIONS,
 )
 from ppa.dict import unpack_to_dict
-from ppa.io import open_url
 from ppa.job import show_waiting, show_running
 from ppa.lp import Lp
 from ppa.ppa import (
@@ -88,7 +86,7 @@ from ppa.ppa import (
 )
 from ppa.ppa_group import PpaGroup, PpaAlreadyExists
 from ppa.repository import Repository
-from ppa.result import get_results, show_results
+from ppa.result import show_results
 from ppa.text import o2str, ansi_hyperlink
 from ppa.trigger import Trigger
 
@@ -865,7 +863,7 @@ def command_tests(lp: Lp, config: dict[str, str]) -> int:
 
     try:
         # Triggers
-        print("* Triggers:")
+        source_pub_triggers = []
         for source_pub in the_ppa.get_source_publications():
             series_codename = source_pub.distro_series.name
             if series_codename not in releases:
@@ -874,9 +872,6 @@ def command_tests(lp: Lp, config: dict[str, str]) -> int:
             if packages and (pkg not in packages):
                 continue
             ver = source_pub.source_package_version
-            url = f"https://launchpad.net/ubuntu/+source/{pkg}/{ver}"
-            source_hyperlink = ansi_hyperlink(url, f"{pkg}/{ver}")
-            print(f"  - Source {source_hyperlink}: {source_pub.status}")
             triggers = [Trigger(pkg, ver, arch, series_codename, the_ppa) for arch in architectures]
 
             if config.get("show_rdepends"):
@@ -910,7 +905,16 @@ def command_tests(lp: Lp, config: dict[str, str]) -> int:
                         for arch
                         in architectures
                     ])
+            source_pub_triggers.append((source_pub, triggers))
 
+        # Display triggers
+        print("* Triggers:")
+        for source_pub, triggers in source_pub_triggers:
+            pkg = source_pub.source_package_name
+            ver = source_pub.source_package_version
+            url = f"https://launchpad.net/ubuntu/+source/{pkg}/{ver}"
+            source_hyperlink = ansi_hyperlink(url, f"{pkg}/{ver}")
+            print(f"  - Source {source_hyperlink}: {source_pub.status}")
             if config.get("show_urls"):
                 for trigger in triggers:
                     title = ''
@@ -937,25 +941,11 @@ def command_tests(lp: Lp, config: dict[str, str]) -> int:
                         trigger.action_url + "&all-proposed=1",
                         f"Trigger all-proposed {title}@{trigger.arch}💍"
                     )
-                    print(f"    + " + pad.join([basic_trig, all_proposed_trig]))
+                    print("    + " + pad.join([basic_trig, all_proposed_trig]))
 
         # Results
-        ## get_autopkgtest_results
-        results = []
-        for release in releases:
-            base_results_fmt = f"{URL_AUTOPKGTEST}/results/autopkgtest-%s-%s-%s/"
-            base_results_url = base_results_fmt % (release, the_ppa.owner_name, the_ppa.name)
-            url = f"{base_results_url}?format=plain"
-            response = open_url(url)
-            if response:
-                trigger_sets = {}
-                for result in get_results(response, base_results_url, arches=architectures):
-                    trigger = ', '.join([str(r) for r in result.get_triggers()])
-                    trigger_sets.setdefault(trigger, [])
-                    trigger_sets[trigger].append(result)
-                results.append(trigger_sets)
-
-        show_results(results, config.get('show_urls'))
+        show_results(the_ppa.get_autopkgtest_results(releases, architectures),
+                     config.get('show_urls'))
 
         # Running Queue
         show_running(sorted(the_ppa.get_autopkgtest_running(releases),
diff --git a/tests/test_ppa.py b/tests/test_ppa.py
index 7e2e4bf..1f8fc65 100644
--- a/tests/test_ppa.py
+++ b/tests/test_ppa.py
@@ -8,28 +8,42 @@
 # Released under GNU GPLv2 or later, read the file 'LICENSE.GPLv2+' for
 # more information.
 
+"""Ppa class tests"""
+
 import os
 import sys
+import time
 
 import pytest
+from mock import patch
 
 sys.path.insert(0, os.path.realpath(
     os.path.join(os.path.dirname(os.path.realpath(__file__)), "..")))
 
+from ppa.constants import URL_AUTOPKGTEST
 from ppa.ppa import Ppa, ppa_address_split, get_ppa
+from ppa.result import Result
 
 
 def test_object():
     """Check that PPA objects can be instantiated."""
-    ppa = Ppa('test-ppa-name', 'test-owner-name')
+    ppa = Ppa('test-ppa-name', 'test-owner-name', ppa_description='test-desc', service='test-svc')
     assert ppa
+    assert ppa.ppa_name == 'test-ppa-name'
+    assert ppa.owner_name == 'test-owner-name'
+    assert ppa.ppa_description == 'test-desc'
 
 
-def test_description():
-    """Check specifying a description when creating a PPA."""
-    ppa = Ppa('test-ppa-name', 'test-owner-name', 'test-description')
+def test_repr():
+    """Check Ppa object representation."""
+    ppa = Ppa('a', 'b', 'c', 'd')
+    assert repr(ppa) == "Ppa(ppa_name='a', owner_name='b')"
 
-    assert 'test-description' in ppa.ppa_description
+
+def test_str():
+    """Check Ppa object textual presentation."""
+    ppa = Ppa('a', 'b', 'c', 'd')
+    assert f"{ppa}" == 'b/a'
 
 
 def test_address():
@@ -38,6 +52,65 @@ def test_address():
     assert ppa.address == "ppa:owner/test"
 
 
+def test_description():
+    """Check specifying a description when creating a PPA."""
+    ppa = Ppa('test-ppa-name', 'test-owner-name', 'test-description')
+
+    assert 'test-description' in ppa.ppa_description
+
+
[e-mail]('releases, architectures, expected_num_results, expected_num_triggers', [
+    ([], [], 0, 0),
+    (['x'], ['amd64'], 1, 1),
+    (['x', 'y', 'z'], ['amd64'], 3, 1),
+    (['x'], ['amd64', 'armhf', 'i386'], 1, 3),
+    (['x', 'y', 'z'], ['amd64', 'armhf', 'i386'], 3, 3),
+])
+@patch('ppa.ppa.get_results')
+@patch('ppa.ppa.open_url')
+def test_get_autopkgtest_results(mock_open_url, mock_get_results,
+                                 releases, architectures, expected_num_results, expected_num_triggers):
+    """Check that autopkgtest results are retrieved and iterated correctly."""
+    owner_name = 'a'
+    ppa_name = 'b'
+
+    # Bypass open_url() entirely so we don't try to retrieve anything.
+    # Usually this returns a response that if valid is then passed to
+    # get_results(), but since we'll be patching get_results(), all that
+    # matters is that validity check passes.  We can do that by having
+    # our mock open_url return a generic valid value, True.
+    mock_open_url.return_value = True
+
+    # Substitute in our fake results to be returned by get_results().
+    # We need to have semi-valid looking URLs to pass internal checks,
+    # but can provide trivially fake data.
+    fake_results = {}
+    fake_data_url = "https://fake.data"
+    timestamp = time.strptime('20030201_040506', "%Y%m%d_%H%M%S")
+    for release in releases:
+        k = f"{URL_AUTOPKGTEST}/results/autopkgtest-{release}-{owner_name}-{ppa_name}/"
+        fake_results[k] = []
+        for arch in architectures:
+            # We don't care about the triggers for the result, just the
+            # number of results and their basic identity, so replace the
+            # get_triggers() call to avoid it making any remote calls.
+            result = Result(url=fake_data_url, time=timestamp, series=release, arch=arch, source=None)
+            result.get_triggers = lambda: "x"
+            fake_results[k].append(result)
+
+    def fake_get_results(response, url, arches):
+        return fake_results[url]
+    mock_get_results.side_effect = fake_get_results
+
+    ppa = Ppa(ppa_name, owner_name)
+    results = ppa.get_autopkgtest_results(releases, architectures)
+
+    assert len(results) == expected_num_results
+    for trigger_set in results:
+        assert isinstance(trigger_set, dict)
+        assert all(len(triggers) == expected_num_triggers for triggers in trigger_set.values())
+
+
 @pytest.mark.parametrize('address, expected', [
     # Successful cases
     ('bb', (None, 'bb')),
diff --git a/tests/test_result.py b/tests/test_result.py
index 4063b83..96b1535 100644
--- a/tests/test_result.py
+++ b/tests/test_result.py
@@ -28,8 +28,15 @@ from ppa.result import Result
 
 def test_object():
     """Checks that Result objects can be instantiated."""
-    result = Result('url', None, 'b', 'c', 'd')
+    timestamp = time.strptime('20030201_040506', "%Y%m%d_%H%M%S")
+    result = Result('url', timestamp, 'b', 'c', 'd')
     assert result
+    assert result.url == 'url'
+    assert result.time == timestamp
+    assert result.series == 'b'
+    assert result.arch == 'c'
+    assert result.source == 'd'
+    assert not result.error_message
 
 
 def test_repr():