Merge ppa-dev-tools:fix-lp2038651-bionic-not-included into ppa-dev-tools:main

Proposed by Bryce Harrington
Status: Merged
Merged at revision: 3ce7aaf4b8b5a457ca4b228a7758a1b2ec4216c7
Proposed branch: ppa-dev-tools:fix-lp2038651-bionic-not-included
Merge into: ppa-dev-tools:main
Diff against target: 84 lines (+38/-3)
2 files modified
scripts/ppa (+1/-0)
tests/test_scripts_ppa.py (+37/-3)
Reviewer Review Type Date Requested Status
Sebastien Bacher (community) Approve
Andreas Hasenack (community) Approve
PpaDevTools Developers Pending
Canonical Server Reporter Pending
Review via email: mp+453478@code.launchpad.net

Description of the change

Simple fix for issue discovered and reported by Sebastien Bacher, that the trigger list misses packages targeting ESM releases.

I included with this change an expansion of the relevant test case to cover this issue. Running 'make check' without the fix applied will result in a test failure, and with it applied it then passes.

Admittedly, the test case has some bitrot potential in that eventually even ESM releases will become EOL, so eventually this test case will start failing when 'bionic' hits that point. But this isn't the only test case with such an issue. (Eventually these will need fixed by mocking distro-info to allow hardcoding the releases, however that's a larger scope cleanup than this bugfix.)

To post a comment you must log in.
Revision history for this message
Andreas Hasenack (ahasenack) wrote :

supported_esm() currently also includes trusty and xenial. Should we test for those as well, or are you worried about the test becoming wrong once those become unsupported via esm?

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

> supported_esm() currently also includes trusty and xenial. Should we test for
> those as well, or are you worried about the test becoming wrong once those
> become unsupported via esm?

Exactly, this is destined to bitrot but by ignoring those two I'm hoping to put that day off a few years. :-)

For purposes of the code, checking just one of them is going to be sufficient, and bionic will be around the longest of the three.

Revision history for this message
Andreas Hasenack (ahasenack) wrote :

Thanks, +1

review: Approve
Revision history for this message
Sebastien Bacher (seb128) wrote :

seems also fine to me thanks!

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

Thanks, landed:

Total 0 (delta 0), reused 0 (delta 0), pack-reused 0
To git+ssh://git.launchpad.net/ppa-dev-tools
   a2a04a8..3ce7aaf main -> main

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/scripts/ppa b/scripts/ppa
2index f252363..16a7456 100755
3--- a/scripts/ppa
4+++ b/scripts/ppa
5@@ -940,6 +940,7 @@ def command_tests(lp: Lp, config: dict[str, str]) -> int:
6 if releases is None:
7 udi = UbuntuDistroInfo()
8 releases = udi.supported()
9+ releases.extend(r for r in udi.supported_esm() if r not in releases)
10 if isinstance(releases, str):
11 releases = list(unpack_to_dict(releases).keys())
12 if not isinstance(releases, list):
13diff --git a/tests/test_scripts_ppa.py b/tests/test_scripts_ppa.py
14index 4052172..07a1b32 100644
15--- a/tests/test_scripts_ppa.py
16+++ b/tests/test_scripts_ppa.py
17@@ -742,22 +742,55 @@ def test_command_show(fake_config):
18 assert script.command_show(lp, fake_config) == 0
19
20
21-@pytest.mark.parametrize('params, format, expected_in_stdout', [
22+@pytest.mark.parametrize('params, pubs, format, expected_in_stdout', [
23 (
24+ # General behavior
25 {
26 'releases': None,
27 'architectures': 'amd64',
28 'show_urls': True
29 },
30+ [('x', '1', 'Published', 'jammy')],
31 'plain',
32 'arch=amd64&trigger=x%2F1&ppa=me%2Ftesting'
33 ),
34+ (
35+ # Specified release
36+ {
37+ 'releases': 'focal',
38+ 'architectures': 'amd64',
39+ 'show_urls': True
40+ },
41+ [
42+ ('x', '3.2.1', 'Published', 'jammy'),
43+ ('x', '1.2.3', 'Published', 'focal'),
44+ ('x', '1.0.0', 'Published', 'bionic'),
45+ ],
46+ 'plain',
47+ 'arch=amd64&trigger=x%2F1.2.3&ppa=me%2Ftesting'
48+ ),
49+ (
50+ # Default behavior should include ESM releases (LP: #2038651)
51+ {
52+ 'releases': None,
53+ 'architectures': 'amd64',
54+ 'show_urls': True
55+ },
56+ [
57+ ('x', '3.2.1', 'Published', 'jammy'),
58+ ('x', '1.2.3', 'Published', 'focal'),
59+ ('x', '1.0.0', 'Published', 'bionic'),
60+ ],
61+ 'plain',
62+ 'arch=amd64&trigger=x%2F1.0.0&ppa=me%2Ftesting'
63+ ),
64 ])
65 @patch('urllib.request.urlopen')
66 @patch('ppa.io.open_url')
67 def test_command_tests(urlopen_mock,
68 open_url_mock,
69- fake_config, capfd, params, format, expected_in_stdout):
70+ fake_config, capfd,
71+ params, pubs, format, expected_in_stdout):
72 '''Checks that the tests command retrieves and displays correct results.'''
73 lp = LpServiceMock()
74 urlopen_mock.return_value = "{}"
75@@ -770,7 +803,8 @@ def test_command_tests(urlopen_mock,
76 the_ppa = lp.me.getPPAByName(fake_config['ppa_name'])
77
78 # Add some fake publications
79- the_ppa.published_sources.append(PublicationMock('x', '1', 'Published', 'jammy'))
80+ for pub in pubs:
81+ the_ppa.published_sources.append(PublicationMock(*pub))
82
83 config = {**fake_config, **params}
84 assert script.command_tests(lp, config) == 0

Subscribers

People subscribed via source and target branches

to all changes: