Merge ppa-dev-tools:results-class into ppa-dev-tools:main

Proposed by Bryce Harrington
Status: Merged
Merged at revision: 801f354619b8a21dee7350b01fcd32e5aef387a3
Proposed branch: ppa-dev-tools:results-class
Merge into: ppa-dev-tools:main
Diff against target: 786 lines (+673/-6)
10 files modified
.flake8 (+1/-0)
AUTHORS.md (+2/-0)
ppa/result.py (+265/-0)
ppa/subtest.py (+109/-0)
ppa/trigger.py (+80/-0)
tests/test_io.py (+2/-3)
tests/test_job.py (+7/-3)
tests/test_result.py (+66/-0)
tests/test_subtest.py (+86/-0)
tests/test_trigger.py (+55/-0)
Reviewer Review Type Date Requested Status
Athos Ribeiro (community) Approve
Canonical Server Reporter Pending
Review via email: mp+429187@code.staging.launchpad.net

Description of the change

Adds the Subtest, Trigger, and Results classes.

I've run lint/flake on these via the check-scripts command from ubuntu-helpers.

    $ check-scripts ./ppa/result.py
    $ check-scripts ./tests/test_result.py
    ...

Each module has a corresponding test that can be invoked via pytest-3, e.g:

    $ pytest-3 ./tests/test_result.py
    ...

Each module can also be run as a script, as a cheap smoketest:

    $ python3 -m ppa.result

To post a comment you must log in.
Revision history for this message
Athos Ribeiro (athos-ribeiro) wrote :

Thanks, Bryce. LGTM!

I left a few inline comments, including a question, suggestion on a changed test.

review: Approve
Revision history for this message
Bryce Harrington (bryce) wrote :

Answer & re-question for one of your feedback items, inline.

Revision history for this message
Athos Ribeiro (athos-ribeiro) wrote :

On Wed, Aug 31, 2022 at 06:40:02PM -0000, Bryce Harrington wrote:
>Answer & re-question for one of your feedback items, inline.
>
>Diff comments:
>
>> diff --git a/tests/test_job.py b/tests/test_job.py
>> index d9a9967..44491c2 100644
>> --- a/tests/test_job.py
>> +++ b/tests/test_job.py
>> @@ -71,9 +74,10 @@ def test_request_url():
>> jobinfo = {
>> 'triggers': ['t/1'],
>> 'ppas': ['ppa:a/b', 'ppa:c/d']
>> - }
>> + }
>> job = Job(0, 'a', 'b', 'c', 'd', jobinfo['triggers'], jobinfo['ppas'])
>> - assert job.request_url == "https://autopkgtest.ubuntu.com/request.cgi?release=c&arch=d&package=b&trigger=t/1"
>> + assert job.request_url.startswith("https://autopkgtest.ubuntu.com/request.cgi")
>> + assert job.request_url.endswith("?release=c&arch=d&package=b&trigger=t/1")
>
>Yeah it was for shortening the length for the lint warning. Normally I'd do the new variables but figured that would make the testing less clear.
>
>My thinking here for two tests is that it now checks a) is the cgi address correctly inserted, and then b) are the args showing up properly. Do you think it'd be better to just check the whole URL?
>

This was just a nitpick to point out that the test changed. To keep the
behavior, i.e, for the commit to be a refactoring only commit, it should
keep checking the whole thing. It should be OK to merge it as is though
:)

>>
>>
>> def test_get_running():
>
>
>--
>https://code.launchpad.net/~bryce/ppa-dev-tools/+git/ppa-dev-tools-1/+merge/429187
>You are reviewing the proposed merge of ppa-dev-tools:results-class into ppa-dev-tools:main.
>

--
Athos Ribeiro

Revision history for this message
Bryce Harrington (bryce) wrote :

Thanks Athos, yeah while it's not a pure refactor I think splitting the url check to two tests is a good change. I've gone ahead and landed the branch.

There was an error fetching revisions from git servers. Please try again in a few minutes. If the problem persists, contact Launchpad support.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: