Code review comment for ~bryce/ubuntu-helpers:add-trigger-class

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

# 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

« Back to merge proposal