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 operate on the Trigger
objects.  I found that rearranging the logic got a bit involved so
decided to postpone that part, and just get the initial bit through
review.  The good news is it looks like this will allow a lot of logic
to get condensed nicely.

Also, so far the changes are just going to the presentation logic for
triggers, however as we know there is also going to come functionality
for creating triggers.  So this hopefully sets some groundwork towards
that.

> 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.

Thanks again, will do!

Bryce

> -- 
> https://code.launchpad.net/~bryce/ubuntu-helpers/+git/ubuntu-helpers/+merge/426849
> You are the owner of ~bryce/ubuntu-helpers:add-trigger-class.