Merge ~bryce/ubuntu-helpers:add-trigger-class into ~ubuntu-server/+git/ubuntu-helpers:main

[Bryce Harrington]

This adds a Trigger class and cleans up the regex used to extract triggers.
The new additions aren't actually used in the code, but I've also included a super simple smoketest to check the functionality.

I've also been running it against a PPA as a real world test.

    $ time lp-test-ppa ppa:sergiodj/sssd-merge --release kinetic
    real 0m15.545s
    user 0m0.674s
    sys 0m0.109s

I've been hoping to find ways to make the code run quicker, however the performance is pretty much dominated by network I/O.

[Christian Ehrhardt  (paelzer)]

# ack to add some testing, no matter how trivial - A few things here though

1. shouldn't it have a list of expected results to check?

2. Instead of just being an extra script, couldn't it (mabye not in this PR, but later) be a test in whatever python testing framework you wanted to use for the final thing?

3. The artifacts are not always reachable forever, please add a check if the fetch worked and if e.g. the expected number of lines came back. If not skip the asserts that should follow (see #1)

# Then on "Simplify regex for triggers"

1. It is fine to reduce the potential mass parsing, but could we be more explicit by using
header_split = self.log.split(": @@@@@@@@@@@@@@@@@@@@ test bed setup", 1)
instead of
header_split = self.log.split(": @@@@@@@@@@@@@@@@@@@@", 1)
Then also the <2 makes more sense. Because otherwise you might split latter sections, the split will be >=2 but still might not contain what you expect.

2. could we define and compile the regex for ADT_TEST_TRIGGERS only if we passed the header split
That reads more like first header split, then regex to parse.
In addition for the (gladly unlikely) case that the header is bad you won't even compile the regex

# Trigger class

I like that this aligns things with each other, but as asked above comparing against expected output in the test - especially using this split package/version/arch/series as well as the stringified autopkgtest_url would help to test that.

# Finally
While all this is nice, I assume there will be changes that then use Triggers right?
Because otherwise while nice these changes will end up feeling odd.
So let me just ask for confirmation there will be users of this other than the test?

TL;DR:
Great, a few small further enhancements/changes requested.
My asks are all simple, feel free to merge yourself without re-review once you addressed them.

review: Needs Fixing

[Christian Ehrhardt  (paelzer)]

I've found this on testing against a PPA that exists but has no tests nor builds run yet.

$ lp-test-ppa ppa:paelzer/kinetic-librabbitmq --release kinetic
Tests for PPA kinetic-librabbitmq
---- ---- ---- ----
Release: kinetic
Sources:
  SRC: librabbitmq @ 0.11.0-1~kineticppa1 - Pending
Triggers on published Sources:
Error: Could not retrieve results from http://autopkgtest.ubuntu.com/results/autopkgtest-kinetic-paelzer-kinetic-librabbitmq/?format=plain: HTTP Error 401: Unauthorized
Results: (none yet at http://autopkgtest.ubuntu.com/results/autopkgtest-kinetic-paelzer-kinetic-librabbitmq/?format=plain)
Running: (none)
Waiting: (none)

Maybe while updating the triggers we should harden it to not yield
"Error: Could not retrieve results from http://autopkgtest.ubuntu.com/results/autopkgtest-kinetic-paelzer-kinetic-librabbitmq/?format=plain: HTTP Error 401: Unauthorized"

Instead if should either say nothing or something like "no tests/builds yet"

[Bryce Harrington (bryce)]

On Thu, Jul 14, 2022 at 07:51:08AM -0000, Christian Ehrhardt  wrote:
> Review: Needs Fixing
>
> # ack to add some testing, no matter how trivial - A few things here though
>
> 1. shouldn't it have a list of expected results to check?
>
> 2. Instead of just being an extra script, couldn't it (mabye not in this PR, but later) be a test in whatever python testing framework you wanted to use for the final thing?
>
> 3. The artifacts are not always reachable forever, please add a check if the fetch worked and if e.g. the expected number of lines came back. If not skip the asserts that should follow (see #1)

Yep, the intent is this will evolve into a test case in ppa-dev-tools
(which is already set up with a pytest-based testsuite). I just wanted
something quick and dirty to exercise the code without putting too much
time in. I do plan on evolving this into a proper test, and tackling
all those points.

> # Then on "Simplify regex for triggers"
>
> 1. It is fine to reduce the potential mass parsing, but could we be more explicit by using
> header_split = self.log.split(": @@@@@@@@@@@@@@@@@@@@ test bed setup", 1)
> instead of
> header_split = self.log.split(": @@@@@@@@@@@@@@@@@@@@", 1)
> Then also the <2 makes more sense. Because otherwise you might split latter sections, the split will be >=2 but still might not contain what you expect.
>

Yeah, what might be better would be to go ahead and split all the
sections and store them into a list. The section names are unforunately
not unique (i.e. each test starts with 'test bed setup') so can't be
like an OrderedDict, but it could be a list of tuples.

In any case, the header is always the first section, which is why I
didn't reference the second section's 'test bed setup' name.

But yeah, having some sort of retained data structure for all the
sections could be useful if we ever want to do per-subtest analysis.
I'll followup this MP with another to do this.

> 2. could we define and compile the regex for ADT_TEST_TRIGGERS only if we passed the header split
> That reads more like first header split, then regex to parse.
> In addition for the (gladly unlikely) case that the header is bad you won't even compile the regex

In fact I think the regex compilation can be moved to a static class
variable; typically regex compilation only needs to be done once at
program startup. So yeah, I'll move this to a better location.

> # Trigger class
>
> I like that this aligns things with each other, but as asked above comparing against expected output in the test - especially using this split package/version/arch/series as well as the stringified autopkgtest_url would help to test that.
>

Agreed.

> # Finally
> While all this is nice, I assume there will be changes that then use Triggers right?
> Because otherwise while nice these changes will end up feeling odd.
> So let me just ask for confirmation there will be users of this other than the test?

Yes, this MP is just the first part of a larger effort for triggers, it
was just the amount I could complete in the time I had for it
yesterday. The next step is refactoring the code that is operating on
"package/version" trigger strings to instead operat...

[Christian Ehrhardt  (paelzer)]

#1
ok on quick'n-dirty test for now, as you confirmed this will merge into a real test (framework).

#2 ok to do this in a follow up as you suggested - thanks

#3 ok as well

So land it on your terms and take not of the identified actions to be done later.
I'm looking forward to the next few PRs :-)

+1

review: Approve

diff --git a/cpaelzer/lp-test-ppa b/cpaelzer/lp-test-ppa
index 63d95d1..08dbfe3 100755
--- a/cpaelzer/lp-test-ppa
+++ b/cpaelzer/lp-test-ppa
@@ -87,6 +87,24 @@ class Subtest:
         return Subtest.VALUES[self.status]
 
 
+class Trigger:
+    def __init__(self, package, version, arch, series):
+        self.package = package
+        self.version = version
+        self.arch = arch
+        self.series = series
+
+    def __repr__(self):
+        return f"{self.package}/{self.version}"
+
+    def __str__(self):
+        return f"{self.package}/{self.version}"
+
+    @property
+    def autopkgtest_url(self):
+        return f"{URL_AUTOPKGTEST}/o/{self.package}/{self.series}/{self.arch}"
+        pass
+
 class Result:
     VALUES = {
         'PASS': "✅",
@@ -149,14 +167,35 @@ class Result:
         :rtype: list[str]
         :returns: List of package/version triggers.
         """
-        res_trigger = re.search("--env=ADT_TEST_TRIGGERS=.* -- ", self.log)
-        if not res_trigger:
+        regex_triggers = re.compile(r'--env=ADT_TEST_TRIGGERS=(.*?) -- ')
+        header_split = self.log.split(": @@@@@@@@@@@@@@@@@@@@", 1)
+        if len(header_split) < 2:
+            return []
+        m = re.search(regex_triggers, header_split[0])
+        if not m:
+            return []
+
+        return m.group(1).strip("'").split()
+
+    @lru_cache
+    def get_triggers(self, name=None):
+        """Provides list of Triggers that were used to create this Result
+
+        This returns the set of Triggers used to create the Result, as
+        recorded in the test log.  Each trigger identifies a
+        package/version pair corresponding to source packages to use
+        from the proposed archive (instead of from the release archive).
+
+        :param str name: If defined, only return triggers starting with this name.
+        :rtype: Iterator[Trigger]
+        :returns: Triggers used to generate this Result, if any, or an empty list
+        """
+        if not self.triggers:
             return []
-        res_trigger = res_trigger.group(0)
-        res_trigger = res_trigger[len("--env=ADT_TEST_TRIGGERS="):]
-        res_trigger = res_trigger[:res_trigger.find(" -- ")]
-        res_trigger = res_trigger.strip("'")
-        return res_trigger.split()
+
+        for t in self.triggers:
+            package, version = t.split('/', 1)
+            yield Trigger(package, version, arch=self.arch, series=self.series)
 
     @lru_cache
     def get_subtests(self, name=None):
diff --git a/cpaelzer/smoketest_lp-test-ppa b/cpaelzer/smoketest_lp-test-ppa
new file mode 100755
index 0000000..d8b2965
--- /dev/null
+++ b/cpaelzer/smoketest_lp-test-ppa
@@ -0,0 +1,37 @@
+#!/usr/bin/python3
+
+# Author:  Bryce Harrington <[e-mail]>
+#
+# Copyright (C) 2021 Bryce W. Harrington
+#
+# Released under GNU GPLv2 or later, read the file 'LICENSE.GPLv2+' for
+# more information.
+
+import os
+import sys
+import types
+
+import importlib.machinery
+
+SCRIPT_NAME = 'lp-test-ppa'
+BASE_PATH = os.path.realpath(os.path.join(os.path.dirname(__file__), '..'))
+scripts_path = os.path.join(BASE_PATH, 'cpaelzer')
+sys.path.insert(0, BASE_PATH)
+
+script_path = os.path.join(scripts_path, SCRIPT_NAME)
+loader = importlib.machinery.SourceFileLoader(SCRIPT_NAME, script_path)
+script = types.ModuleType(loader.name)
+loader.exec_module(script)
+
+autopkgtest_log_urls = [
+    "https://autopkgtest.ubuntu.com/results/autopkgtest-kinetic-sergiodj-sssd-merge/kinetic/arm64/s/sssd/20220614_220046_4c037@/log.gz",
+    "https://autopkgtest.ubuntu.com/results/autopkgtest-kinetic/kinetic/arm64/a/apache2/20220513_233749_05356@/log.gz",
+    "https://autopkgtest.ubuntu.com/results/autopkgtest-kinetic/kinetic/amd64/u/ubuntu-image/20220523_161504_bf590@/log.gz"
+    ]
+
+for url in autopkgtest_log_urls:
+    result = script.Result(url, None, None, None, None)
+    print(url)
+    print(list(result.get_triggers()))
+    print()
+