Merge ppa-dev-tools:fix-all-proposed-triggers into ppa-dev-tools:main

[Bryce Harrington]

The fix for LP: #2044608 is quite simple, however test coverage of the code in question was missing. This branch provides those tests both for the trigger-related functionality in the Results class, and for the Trigger class itself.

Additionally, this includes a fix for a recent regression in a branch that improved exit codes. A standard python exit code documented in the os module turns out to be platform-specific, and Linux is not one of those platforms... Without this fix, the command line examples will fail.

Anyway, testing is as usual:
  $ make check
or:
  $ pytest-3

To verify the CLI operation live, I ran against seb's PPA:
  $ ./scripts/ppa tests -L ppa:seb128/update-manager-sru -a amd64 -p distro-info

Try it with and without -L, and with and without --show-rdepends. It should show the line for the all-proposed links in all cases.

[Athos Ribeiro (athos-ribeiro)]

LGTM. Thanks, Bryce :)

It seems this branch needs to be rebased on the main branch though (I reviewed and tested these changes after a local rebase).

I no longer se duplicated entries. Instead, I see the all-proposed URLs as intended.

And thanks for writing all those tests.

I am also seeing some issues (unrelated to the changes proposed here):

- The UTF-8 characters seem to be broken here (I am on noble). I could find no good past version of the package in the development history, so I suppose this is due to some new/inconsistent package in my system. Once it is safe to pull the latest noble changes in, I will test again and report issues as needed.
- I see 4 failing tests when running the test suite. I am unsure if this is related to the bullet point above, but git-bisect shows me that 5d7cbc9 is the first "bad" revision here. I suppose they are due to the "file:///home/bryce/src/PpaDevTools/" hardcoded entries in the test suite.
- Finally, lately I have been seeing error messages when running the `tests` sub-command. These does not seem to affect the subcommand usability, but the error messages are always present. They look like this:

Error: Could not retrieve data from https://autopkgtest.ubuntu.com/results/autopkgtest-focal-athos-ribeiro-phpmyadmin-php83/?format=plain: HTTP Error 404: Not FoundError: Could not retrieve data from https://autopkgtest.ubuntu.com/results/autopkgtest-jammy-athos-ribeiro-phpmyadmin-php83/?format=plain: HTTP Error 404: Not FoundError: Could not retrieve data from https://autopkgtest.ubuntu.com/results/autopkgtest-mantic-athos-ribeiro-phpmyadmin-php83/?format=plain: HTTP Error 404: Not FoundError: Could not retrieve data from https://autopkgtest.ubuntu.com/results/autopkgtest-trusty-athos-ribeiro-phpmyadmin-php83/?format=plain: HTTP Error 404: Not FoundError: Could not retrieve data from https://autopkgtest.ubuntu.com/results/autopkgtest-xenial-athos-ribeiro-phpmyadmin-php83/?format=plain: HTTP Error 404: Not FoundError: Could not retrieve data from https://autopkgtest.ubuntu.com/results/autopkgtest-bionic-athos-ribeiro-phpmyadmin-php83/?f

review: Approve

[Bryce Harrington (bryce)]

Hi Athos, thanks for the review.

Yes both those issues are resolved by the other MP up for review:
  https://code.launchpad.net/~bryce/ppa-dev-tools/+git/ppa-dev-tools-1/+merge/462844

[Bryce Harrington (bryce)]

Landed:

Writing objects: 100% (24/24), 6.12 KiB | 3.06 MiB/s, done.
Total 24 (delta 17), reused 0 (delta 0), pack-reused 0
To git+ssh://git.launchpad.net/ppa-dev-tools
   023e7f3..5dc684c main -> main

diff --git a/ppa/trigger.py b/ppa/trigger.py
index 4345508..8f1680a 100755
--- a/ppa/trigger.py
+++ b/ppa/trigger.py
@@ -173,8 +173,8 @@ def show_triggers(package, version, triggers, status,
             title = ''
             if show_trigger_names:
                 title = trigger.test_package
-                url = trigger.action_url + "&all-proposed=1"
-                print(f"    + {trigger.package}@{trigger.arch}: {url}💍")
+            url = trigger.action_url + "&all-proposed=1"
+            print(f"    + {title}@{trigger.arch}: {url}💍")
 
     else:
         for trigger in triggers:
diff --git a/scripts/ppa b/scripts/ppa
index 8484979..2172740 100755
--- a/scripts/ppa
+++ b/scripts/ppa
@@ -94,6 +94,7 @@ from ppa.trigger import get_triggers, show_triggers
 import ppa.debug
 from ppa.debug import dbg, warn, error
 
+EX_NOTFOUND = 127
 EX_KEYBOARD_INTERRUPT = 130
 
 
@@ -804,7 +805,7 @@ def command_set(lp: Lp, config: dict[str, str]) -> int:
         return os.EX_NOPERM
     except PpaNotFoundError as e:
         print(e)
-        return os.EX_NOTFOUND
+        return EX_NOTFOUND
     except ValueError as e:
         print(f"Error: {e}")
         return os.EX_USAGE
@@ -868,7 +869,7 @@ def command_show(lp: Lp, config: dict[str, str]) -> int:
         return os.EX_OK
     except PpaNotFoundError as e:
         print(e)
-        return os.EX_NOTFOUND
+        return EX_NOTFOUND
     except KeyboardInterrupt:
         return EX_KEYBOARD_INTERRUPT
     print("Unhandled error")
@@ -939,7 +940,7 @@ def command_wait(lp: Lp, config: dict[str, str]) -> int:
         return os.EX_OK
     except PpaNotFoundError as e:
         print(e)
-        return os.EX_NOTFOUND
+        return EX_NOTFOUND
     except ValueError as e:
         print(f"Error: {e}")
         return os.EX_USAGE
@@ -967,7 +968,7 @@ def command_tests(lp: Lp, config: dict[str, str]) -> int:
             apt_repository = Repository(cache_dir=local_dists_path)
         except FileNotFoundError as e:
             error(f'Missing checkout\n{LOCAL_REPOSITORY_MIRRORING_DIRECTIONS}: {e}')
-            return os.EX_NOTFOUND
+            return EX_NOTFOUND
 
     releases = config.get('releases', None)
     if releases is None:
@@ -989,7 +990,7 @@ def command_tests(lp: Lp, config: dict[str, str]) -> int:
     the_ppa = get_ppa(lp, config)
     if not the_ppa.exists():
         error(f"PPA {the_ppa.name} does not exist for user {the_ppa.owner_name}")
-        return os.EX_NOTFOUND
+        return EX_NOTFOUND
 
     architectures = config.get('architectures', ARCHES_AUTOPKGTEST)
     if isinstance(architectures, str):
diff --git a/tests/data/results-chrony-armhf.log.gz b/tests/data/results-chrony-armhf.log.gz
 new file mode 100644
index 0000000..1813920
 Binary files /dev/null and b/tests/data/results-chrony-armhf.log.gz differ
diff --git a/tests/test_result.py b/tests/test_result.py
index c7e2510..a3597af 100644
--- a/tests/test_result.py
+++ b/tests/test_result.py
@@ -17,7 +17,6 @@ import time
 import gzip
 import json
 import pytest
-# from mock import Mock
 
 sys.path.insert(0, os.path.realpath(
     os.path.join(os.path.dirname(os.path.realpath(__file__)), "..")))
@@ -126,10 +125,49 @@ def test_log(tmp_path):
     assert result.log == "abcde"
 
 
-def test_triggers():
[e-mail]('filename, expected_triggers', [
+    ('results-six-s390x.log.gz', ['pygobject/3.42.2-2', 'six/1.16.0-4']),
+    ('results-chrony-armhf.log.gz', ['dpkg/1.22.6ubuntu5'])
+])
+def test_triggers(filename, expected_triggers):
     """Checks that autopkgtest triggers can be extracted from test result logs."""
-    result = Result(f"file://{DATA_DIR}/results-six-s390x.log.gz", None, None, None, None)
-    assert result.triggers == ['pygobject/3.42.2-2', 'six/1.16.0-4']
+    result = Result(f"file://{DATA_DIR}/{filename}", None, None, None, None)
+    assert result.triggers == expected_triggers
+
+
[e-mail]('triggers, name, expected', [
+    ([], None, []),
+    (['x/1'], None, ["Trigger(package='x', version='1', arch=None, series=None, ppa=None, test_package='x')"]),
+    (['x/1'], 'x', ["Trigger(package='x', version='1', arch=None, series=None, ppa=None, test_package='x')"]),
+    pytest.param(
+        ['x/1'], 'z', [],
+        marks=pytest.mark.xfail(reason="Unimplemented")
+    ),
+    (
+        ['x/1', 'y/2', 'z/3'],
+        None,
+        [
+            "Trigger(package='x', version='1', arch=None, series=None, ppa=None, test_package='x')",
+            "Trigger(package='y', version='2', arch=None, series=None, ppa=None, test_package='y')",
+            "Trigger(package='z', version='3', arch=None, series=None, ppa=None, test_package='z')"
+        ]
+    ),
+    pytest.param(
+        ['x/1', 'y/2', 'z/3'],
+        'y',
+        [
+            "Trigger(package='y', version='2', arch=None, series=None, ppa=None, test_package='y')"
+        ],
+        marks=pytest.mark.xfail(reason="Unimplemented"),
+    ),
+])
+def test_get_triggers(monkeypatch, triggers, name, expected):
+    """Checks retrieval of Trigger objects from autopkgtest results."""
+    result = Result('url', None, None, None, None)
+    monkeypatch.setattr(Result, "triggers", triggers)
+
+    triggers = result.get_triggers(name)
+    assert [repr(t) for t in triggers] == expected
 
 
 @pytest.mark.parametrize('log_text, subtest_name, expected', [
@@ -212,6 +250,54 @@ def test_get_subtests(tmp_path, log_text: str, subtest_name: str, expected: dict
     assert {s.desc: s.status for s in subtests} == expected
 
 
[e-mail]('subtest_states, error_message, expected', [
+    ([], None, 'PASS'),
+    (['PASS'], None, 'PASS'),
+    (['FAIL'], None, 'FAIL'),
+    (['PASS', 'FAIL'], None, 'FAIL'),
+    (['FAIL', 'PASS'], None, 'FAIL'),
+    (['PASS', 'PASS', 'PASS'], None, 'PASS'),
+    (['PASS'], 'x', 'BAD'),
+    (['FAIL'], 'x', 'BAD'),
+])
+def test_status(monkeypatch, subtest_states, error_message, expected):
+    """Checks retrieval of status from autopkgtest results."""
+    result = Result("file://tmp/x", None, None, None, None)
+    result.error_message = error_message
+
+    # Add subtests with given states
+    subtests = [Subtest(f"... {state}...") for state in subtest_states]
+    monkeypatch.setattr(Result, "get_subtests", lambda x: subtests)
+
+    assert result.status == expected
+
+
[e-mail]('status, expected', [
+    ('PASS', "✅"),
+    ('FAIL', "❌"),
+    ('BAD', "⛔"),
+])
+def test_status_icon(monkeypatch, status, expected):
+    """Checks generation of correct icon based on autopkgtest results."""
+    result = Result("file://tmp/x", None, None, None, None)
+    monkeypatch.setattr(Result, "status", status)
+
+    assert result.status_icon == expected
+
+
[e-mail]('status, expected_exception', [
+    (None, KeyError),
+    ('x', KeyError),
+])
+def test_status_icon_error(monkeypatch, status, expected_exception):
+    """Checks generation of correct icon based on autopkgtest results."""
+    result = Result("file://tmp/x", None, None, None, None)
+    monkeypatch.setattr(Result, "status", status)
+
+    with pytest.raises(expected_exception):
+        print(result.status_icon)
+
+
 @pytest.mark.parametrize('log_text, series, arch, source, expected_dict', [
     (
         # Empty/invalid log should return empty triggers & subtests.
diff --git a/tests/test_trigger.py b/tests/test_trigger.py
index 1441b85..5e42f71 100644
--- a/tests/test_trigger.py
+++ b/tests/test_trigger.py
@@ -137,40 +137,52 @@ def test_get_triggers(params, expected):
 
 @pytest.mark.parametrize('triggers, params, expected_in_stdout', [
     (
-        # Basic function parameters
+        # Basic function parameters, no triggers
         [],
-        {'package': 'a', 'version': 'b', 'status': 'c'},
-        ["  - Source ", "source/a/b", ": c"]
+        {'package': 'pkg', 'version': '123', 'status': 'OK'},
+        ["  - Source \x1b]8;;https://launchpad.net/ubuntu/+source/pkg/123\x1b\\pkg/123\x1b]8;;\x1b\\: OK\n"]
     ),
     (
-        # Specified trigger
-        [('a', '1')],
-        {'show_trigger_urls': True},
-        ["    + @amd64: ", "trigger=a%2F1"]
+        # Specified trigger (clickable)
+        [('pkg', '123', 'i386')],
+        {'show_trigger_urls': False},
+        [
+            "&trigger=pkg%2F123&ppa=z\x1b\\Trigger basic @i386♻️ \x1b]8;;\x1b",
+            "&trigger=pkg%2F123&ppa=z&all-proposed=1\x1b\\Trigger all-proposed @i386💍\x1b]8;;\x1b\\\n"
+        ]
     ),
     (
-        # Display trigger URLS
-        [('a', '1')],
+        # Specified trigger (display plain URLs)
+        [('pkg', '123', 'i386')],
         {'show_trigger_urls': True},
-        ["https://autopkgtest.ubuntu.com/request.cgi?release=y&package=x&arch=amd64&trigger=a%2F1&ppa=z"]
+        [
+            "&trigger=pkg%2F123&ppa=z♻️ \n",
+            "&trigger=pkg%2F123&ppa=z&all-proposed=1💍\n"
+        ]
     ),
     (
         # Display names of packages in trigger lines
-        [('a', '1')],
+        [('pkg', '123', 'i386')],
         {'show_trigger_names': True},
-        ["Trigger basic", "Trigger all-proposed"]
+        ["Trigger basic x@i386♻️ ", "Trigger all-proposed x@i386💍"]
+    ),
+    (
+        # Omit package names if specified
+        [('pkg', '123', 'i386')],
+        {'show_trigger_names': False},
+        ["Trigger basic @i386", "Trigger all-proposed @i386"]
     ),
     (
         # Display names of packages in trigger lines when trigger URLs are shown
-        [('a', '1')],
+        [('pkg', '123', 'i386')],
         {'show_trigger_urls': True, 'show_trigger_names': True},
-        ["a@amd64: https:", "trigger=a%2F1"]
+        ["x@i386: https:", "trigger=pkg%2F123"]
     ),
     (
         # Multiple triggers
-        [('a', '1'), ('b', '2')],
+        [('pkg', '123', 'i386'), ('lib', '321', 'i386')],
         {'show_trigger_urls': True},
-        ["trigger=a%2F1", "trigger=b%2F2"]
+        ["trigger=pkg%2F123", "trigger=lib%2F321"]
     ),
 ])
 def test_show_triggers(capfd, triggers, params, expected_in_stdout):
@@ -179,7 +191,7 @@ def test_show_triggers(capfd, triggers, params, expected_in_stdout):
     params.setdefault('triggers', [])
     params.setdefault('status', 'x')
     for t in triggers:
-        params['triggers'].append(Trigger(t[0], t[1], 'amd64', 'y', 'z', 'x'))
+        params['triggers'].append(Trigger(t[0], t[1], t[2], 'y', 'z', 'x'))
     show_triggers(**params)
     out, err = capfd.readouterr()
     print(out)