[Merge] lp:~laney/britney/autopkgtest-no-delay-if-never-passed into lp:~ubuntu-release/britney/britney2-ubuntu
Martin Pitt
martin.pitt at ubuntu.com
Tue Nov 17 12:31:04 UTC 2015
Review: Needs Fixing
Thanks for working on this! There's a couple of issues here, but the overall direction is sound. Looking forward to this, especially in times like these when the queues are super full.
Diff comments:
> === modified file 'autopkgtest.py'
> --- autopkgtest.py 2015-11-17 08:15:31 +0000
> +++ autopkgtest.py 2015-11-17 11:07:07 +0000
> @@ -713,9 +719,12 @@
> (testsrc, testver, arch, trigger))
> continue
>
> - pkg_arch_result.setdefault((testsrc, testver), {})[arch] = result
> + pkg_arch_result.setdefault((testsrc, testver), {})[arch] = (result, passed)
This changes the data structure of pkg_arch_result, and also looks redundant. If you want to keep it as a tuple, please update the comment above pkg_arch_result. If you do this, then please put "ever_passed" into the tuple instead, drop "passed", and then also move the REGRESSION/ALWAYSFAILED into the code below, so that the ever_passed evaluation stays in one place.
However, I'd actually suggest to introduce a new status RUNNING_NEVERPASS or similar, which encodes this information similar to ALWAYSFAIL vs. REGRESSION. Please don't forget to add it to EXCUSES_LABELS in excuses.py as well, so that it gets shown properly, perhaps as "Test in progress (never passed"). Then this keeps the current code/data structure and also points out the difference in excuses.html. I'm also fine with RUNNING_NP to avoid making this overly long.
>
> for ((testsrc, testver), arch_results) in pkg_arch_result.items():
> - r = arch_results.values()
> - passed = 'REGRESSION' not in r and 'RUNNING' not in r
You don't have the ever_passed value here any more, so indeed you either need to pass it along in pkg_arch_results, or introduce the new status.
> - yield (passed, testsrc, testver, arch_results)
> + # extract "passed"
> + ps = [p for (_, p) in arch_results.values()]
> + # and then strip them out - these aren't part of our API
> + arch_results = {arch: r for arch, (r, p) in arch_results.items()}
Yeah, this looks ugly..
> + # we've passed if all results are True
> + yield (False not in ps, testsrc, testver, arch_results)
>
> === modified file 'tests/test_autopkgtest.py'
> --- tests/test_autopkgtest.py 2015-11-06 22:51:55 +0000
> +++ tests/test_autopkgtest.py 2015-11-17 11:07:07 +0000
> @@ -169,9 +173,48 @@
> self.assertEqual(self.pending_requests, '')
> self.assertEqual(self.amqp_requests, set())
>
> + def test_no_wait_for_always_failed_test(self):
> + '''We do not need to wait for results for tests which have always
> + failed, but the tests should still be triggered.'''
The (short) test description must be a single line, otherwise this gets cut off and looks ugly in the output. You can add a new multi-line paragraph below for further details, though.
> +
> + self.swift.set_results({'autopkgtest-series': {
> + 'series/i386/d/darkred/20150101_100000@': (4, 'darkred 1'),
> + }})
Just a nitpick, but any particular reason why this needs a new darkred, instead of just using libgreen1 as most of the other tests?
> +
> + exc = self.do_test(
> + [('darkred', {'Version': '2'}, 'autopkgtest')],
> + {'darkred': (True, {'darkred 2': {'i386': 'RUNNING'}})},
> + {'darkred': [('old-version', '1'), ('new-version', '2'),
> + ('excuses', 'Valid candidate')
Note that the "True" two lines above already checks candidacy, so this line is redundant.
> + ]
> + })[1]
> +
> + self.assertEqual(exc['darkred']['tests'], {'autopkgtest':
> + {'darkred 2': {
> + 'amd64': ['RUNNING',
> + 'http://autopkgtest.ubuntu.com/packages/d/darkred/series/amd64'],
> + 'i386': ['RUNNING',
> + 'http://autopkgtest.ubuntu.com/packages/d/darkred/series/i386']}}})
> +
> + self.assertEqual(
> + self.pending_requests, dedent('''\
Using dedent here is indeed a nice touch, thanks for the suggestion. (We should use that in other tests too, but no need to cram it into this MP).
> + darkred 2 amd64 darkred 2
> + darkred 2 i386 darkred 2
> + '''))
> + self.assertEqual(
> + self.amqp_requests,
> + set(['debci-series-amd64:darkred {"triggers": ["darkred/2"]}',
> + 'debci-series-i386:darkred {"triggers": ["darkred/2"]}']))
> +
> +
> def test_multi_rdepends_with_tests_all_running(self):
> '''Multiple reverse dependencies with tests (all running)'''
>
> + # green has passed before
> + self.swift.set_results({'autopkgtest-series': {
> + 'series/i386/g/green/20150101_100000@': (0, 'green 1'),
> + }})
> +
Wouldn't this still be considered as RUNNING_NEVERPASSED on amd64 then? NB that we track the status per package+architecture, not just per package. At second sight I suppose this is actually a good thing, as it ensures that any RUNNING with a previous result will stop the package from being a valid candidate. Adding a comment about that might not hurt.
Also, would it be possible for you to reshuffle the commits so that the first commit adds these new results? Then the tests should all pass still, and the second commit becomes much smaller and simpler to review. (But if it's too much work, it's ok).
> self.do_test(
> [('libgreen1', {'Version': '2', 'Source': 'green', 'Depends': 'libc6'}, 'autopkgtest')],
> {'green': (False, {'green 2': {'amd64': 'RUNNING', 'i386': 'RUNNING'},
> @@ -796,9 +868,9 @@
>
> self.do_test(
> [('libgreen1', {'Version': '2', 'Source': 'newgreen', 'Depends': 'libc6'}, 'autopkgtest')],
> - {'newgreen': (False, {'newgreen 2': {'amd64': 'RUNNING', 'i386': 'RUNNING'},
> - 'lightgreen 1': {'amd64': 'RUNNING', 'i386': 'RUNNING'},
> - 'darkgreen 1': {'amd64': 'RUNNING', 'i386': 'RUNNING'},
> + {'newgreen': (True, {'newgreen 2': {'amd64': 'RUNNING', 'i386': 'RUNNING'},
> + 'lightgreen 1': {'amd64': 'RUNNING', 'i386': 'RUNNING'},
> + 'darkgreen 1': {'amd64': 'RUNNING', 'i386': 'RUNNING'},
So you want to change this test_binary_from_new_source_package_running test case to cover the new status as this doesn't add old results. That's fine, but please add a comment here so that it's clear why this now becomes a valid candidate. Alternatively, if you go with the new status name (RUNNING_NEVERPASSED), this will appear here and thus will be self-explanatory.
> }),
> },
> {'newgreen': [('old-version', '-'), ('new-version', '2')]})
> @@ -1286,9 +1377,9 @@
> ('linux-image-grumpy-generic', {'Source': 'linux-meta-lts-grumpy'}, None),
> ('linux-image-64only', {'Source': 'linux-meta-64only', 'Architecture': 'amd64'}, None),
> ],
> - {'linux-meta': (False, {'fancy 1': {'amd64': 'RUNNING', 'i386': 'RUNNING'}}),
> - 'linux-meta-lts-grumpy': (False, {'fancy 1': {'amd64': 'RUNNING', 'i386': 'RUNNING'}}),
> - 'linux-meta-64only': (False, {'fancy 1': {'amd64': 'RUNNING'}}),
> + {'linux-meta': (True, {'fancy 1': {'amd64': 'RUNNING', 'i386': 'RUNNING'}}),
> + 'linux-meta-lts-grumpy': (True, {'fancy 1': {'amd64': 'RUNNING', 'i386': 'RUNNING'}}),
> + 'linux-meta-64only': (True, {'fancy 1': {'amd64': 'RUNNING'}}),
Same here.
> })
>
> # one separate test should be triggered for each kernel
> @@ -1318,6 +1409,7 @@
> # works against linux-meta and -64only, fails against grumpy i386, no
> # result yet for grumpy amd64
> self.swift.set_results({'autopkgtest-series': {
> + 'series/amd64/f/fancy/20150101_100301@': (0, 'fancy 1', tr('fancy/1')),
Why do we need this? This test runs the fancy package for some kernel triggers, but not for "itself".
> 'series/i386/f/fancy/20150101_100101@': (0, 'fancy 1', tr('linux-meta/1')),
> 'series/amd64/f/fancy/20150101_100101@': (0, 'fancy 1', tr('linux-meta/1')),
> 'series/amd64/f/fancy/20150101_100201@': (0, 'fancy 1', tr('linux-meta-64only/1')),
> @@ -1368,7 +1460,7 @@
>
> self.assertEqual(self.pending_requests, '')
>
> - def test_kernel_triggerered_tests(self):
> + def test_kernel_triggered_tests(self):
Whoops :)
> '''linux, lxc, glibc tests get triggered by linux-meta* uploads'''
>
> self.data.remove_all(False)
> @@ -1404,12 +1500,13 @@
> '''linux waits on linux-meta'''
>
> self.data.add('dkms', False, {})
> - self.data.add('fancy-dkms', False, {'Source': 'fancy', 'Depends': 'dkms (>= 1)'})
> + self.data.add('fancy-dkms', False, {'Source': 'fancy', 'Depends': 'dkms (>= 1)'}, testsuite='autopkgtest')
Please don't do this. We expect DKMS packages to be picked up by autodep8 and to figure out that it has an autodep8 test by heuristics in AutoPackageTest.has_autodep8(). Adding "testsuite=" here circumvents that code path and we stop testing it.
> self.data.add('linux-image-generic', False, {'Version': '0.1', 'Source': 'linux-meta', 'Depends': 'linux-image-1'})
> self.data.add('linux-image-1', False, {'Source': 'linux'}, testsuite='autopkgtest')
> self.data.add('linux-firmware', False, {'Source': 'linux-firmware'}, testsuite='autopkgtest')
>
> self.swift.set_results({'autopkgtest-series': {
> + 'series/i386/f/fancy/20150101_090000@': (0, 'fancy 0.5', tr('fancy/0.5')),
Same question as above, why is this necessary?
> 'series/i386/l/linux/20150101_100000@': (0, 'linux 2', tr('linux-meta/0.2')),
> 'series/amd64/l/linux/20150101_100000@': (0, 'linux 2', tr('linux-meta/0.2')),
> 'series/i386/l/linux-firmware/20150101_100000@': (0, 'linux-firmware 2', tr('linux-firmware/2')),
--
https://code.launchpad.net/~laney/britney/autopkgtest-no-delay-if-never-passed/+merge/277672
Your team Ubuntu Package Archive Administrators is requested to review the proposed merge of lp:~laney/britney/autopkgtest-no-delay-if-never-passed into lp:~ubuntu-release/britney/britney2-ubuntu.
More information about the ubuntu-archive
mailing list