launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04612
[Merge] lp:~jtv/launchpad/bug-826659 into lp:launchpad
Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-826659 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #826659 in Launchpad itself: "Sync errors for packages in +missingpackages don't add a failure comment"
https://bugs.launchpad.net/launchpad/+bug/826659
For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-826659/+merge/71636
= Summary =
The feature: we use package copy jobs (PCJs) to sync packages from a parent distroseries into a derived series. This satisfies the corresponding DistroSeriesDifference (DSDs) on the derived series. A DSDs can be of one of three types: present in the parent but not in the derived series ("missing package"); present in the derived series but not in the parent series ("unique to the derived series"); or present but different in both ("different versions"). If the copy fails, then the job runner looks up the DSD(s) matching the PCJ, and registers the failure there as a comment.
The symptom: failures for "different versions" DSDs are registered properly, but failures for "missing package" DSDs are not. The error handling code is executed, but no comment is created. Turns out to be because the reporting code does not find any DSDs to match the PCJ.
The cause: IDistroSeriesDifferenceSource.getForDistroSeries accepts a variety of filter arguments. For instance, you may pass one or more statuses to limit the DSDs you're interested in. Similarly there is a filter argument for the DSD type it's looking for. That filter argument defaults to None. What this means is not exactly documented; test documentation says that by default, results are not filtered by anything. But the test is hard to figure out because it relies on implicit connections between complex sample data defined elsewhere, and the output in the test. I can't stress enough what a bad idea that is. And guess what? The method interprets a type filter of None as filtering for just the "different versions" type. I can't decide which is worse. I wonder if any of these decisions was made by a younger, less experienced me.
== Proposed fix ==
Make the type filter take None for "do not filter by type," as one would expect. This does not affect the documentation in the interface, because the documentation tacitly suggests that this is the way it works already.
This kind of change is scary to make, because it's hard to see how far the consequences go. There was no test for the broken functionality as such, but given the intertwining of sample data and multiple tests, any change at all will break otherwise unrelated unit tests for the same method. (Ironically, about the only unit test I didn't break was the one for this filter parameter).
But no other tests in the entire test suite break with the new behaviour, so we may just have caught this one in time. We really need to go back to in-depth code reviews if we want to avoid sliding back into the situation where these were routine problems!
== Pre-implementation notes ==
Julian points out that the tests' main sin is doing far too much in their single sample data generation method. Indeed, the multiple-data/single-execution style of testing can work just fine. But I would like to formulate a few requirements:
1. Definition of test data must be visible near the definition of expected outputs.
2. Relationships between input data and output must be explicit.
3. Missing output items must be justified just as much as actual output items.
4. Input data must be regular, according to the ceteris paribus rule.
5. Input data should be complete along non-contiguous axes of their value space.
6. Unit tests should minimize the number of dimensions of input data they span.
As penance, I expect the perpetrator to say an infinite loop of hail marys.
== Implementation details ==
I wrested some of the tests free from their implicit reliance on the insane test data, and then extended the test data so that it at least covers the "missing package" type of DSD. This type happened to be missing, and with this test style, adding test data almost always breaks something.
== Tests ==
{{{
./bin/test -vvc lp.registry.tests.test_distroseriesdifference
}}}
== Demo and Q/A ==
>From Julian's recipe on the bug:
* Be an admin on dogfood, or get yourself anointed by one.
* Find the DSD for package "adapt" in https://dogfood.launchpad.net/ubuntu/oneiric/+missingpackages
* Sync it to oneiric (select its checkbox and hit the Sync button at the bottom of the form).
* Run cronscripts/process-job-source.py -vv IPlainPackageCopyJobSource
* Look up the PCJ that this produces on https://dogfood.launchpad.net/ubuntu/oneiric/+queue
* Accept it (select its checkbox and hit the Accept button at the bottom of the form).
* Run cronscripts/process-job-source.py -vv IPlainPackageCopyJobSource again; this fails.
* Look again on https://dogfood.launchpad.net/ubuntu/oneiric/+missingpackages and see the failure attached to the DSD as a comment.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/registry/tests/test_distroseriesdifference.py
lib/lp/registry/model/distroseriesdifference.py
--
https://code.launchpad.net/~jtv/launchpad/bug-826659/+merge/71636
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-826659 into lp:launchpad.
=== modified file 'lib/lp/registry/model/distroseriesdifference.py'
--- lib/lp/registry/model/distroseriesdifference.py 2011-08-01 16:06:14 +0000
+++ lib/lp/registry/model/distroseriesdifference.py 2011-08-16 02:52:07 +0000
@@ -443,8 +443,6 @@
child_version_higher=False, parent_series=None,
packagesets=None, changed_by=None):
"""See `IDistroSeriesDifferenceSource`."""
- if difference_type is None:
- difference_type = DistroSeriesDifferenceType.DIFFERENT_VERSIONS
if status is None:
status = (DistroSeriesDifferenceStatus.NEEDS_ATTENTION,)
elif isinstance(status, DBItem):
@@ -462,10 +460,11 @@
conditions = [
DSD.derived_series == distro_series,
- DSD.difference_type == difference_type,
DSD.source_package_name == SPN.id, # For ordering.
DSD.status.is_in(status),
]
+ if difference_type is not None:
+ conditions.append(DSD.difference_type == difference_type)
if child_version_higher:
conditions.append(DSD.source_version > DSD.parent_source_version)
=== modified file 'lib/lp/registry/tests/test_distroseriesdifference.py'
--- lib/lp/registry/tests/test_distroseriesdifference.py 2011-08-01 16:06:14 +0000
+++ lib/lp/registry/tests/test_distroseriesdifference.py 2011-08-16 02:52:07 +0000
@@ -967,6 +967,7 @@
diffs = {
'normal': [],
'unique': [],
+ 'missing': [],
'ignored': [],
}
diffs['normal'].append(
@@ -978,6 +979,12 @@
parent_series=parent_series,
difference_type=(
DistroSeriesDifferenceType.UNIQUE_TO_DERIVED_SERIES)))
+ diffs['missing'].append(
+ self.factory.makeDistroSeriesDifference(
+ derived_series=derived_series,
+ parent_series=parent_series,
+ difference_type=(
+ DistroSeriesDifferenceType.MISSING_FROM_DERIVED_SERIES)))
diffs['ignored'].append(
self.factory.makeDistroSeriesDifference(
derived_series=derived_series,
@@ -1029,7 +1036,7 @@
derived_series)
self.assertContentEqual(
- diffs['normal'], result)
+ diffs['normal'] + diffs['unique'] + diffs['missing'], result)
def test_getForDistroSeries_filters_by_distroseries(self):
# Differences for other series are not included.
@@ -1042,6 +1049,16 @@
self.assertFalse(diff_for_other_series in results)
+ def test_getForDistroSeries_does_not_filter_dsd_type_by_default(self):
+ derived_series = self.makeDerivedSeries()
+ dsds = [
+ self.factory.makeDistroSeriesDifference(
+ derived_series, difference_type=diff_type)
+ for diff_type in DistroSeriesDifferenceType.items]
+ dsd_source = getUtility(IDistroSeriesDifferenceSource)
+ self.assertContentEqual(
+ dsds, dsd_source.getForDistroSeries(derived_series))
+
def test_getForDistroSeries_filters_by_type(self):
# Only differences for the specified types are returned.
derived_series = self.makeDerivedSeries()
@@ -1067,16 +1084,21 @@
def test_getForDistroSeries_filters_by_multiple_statuses(self):
# Multiple statuses can be passed for filtering.
derived_series = self.makeDerivedSeries()
- diffs = self.makeDiffsForDistroSeries(derived_series)
-
- result = getUtility(IDistroSeriesDifferenceSource).getForDistroSeries(
- derived_series,
- status=(
- DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT,
- DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
- ))
-
- self.assertContentEqual(diffs['normal'] + diffs['ignored'], result)
+ for status in DistroSeriesDifferenceStatus.items:
+ self.factory.makeDistroSeriesDifference(
+ derived_series, status=status)
+
+ statuses = (
+ DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT,
+ DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
+ )
+
+ dsd_source = getUtility(IDistroSeriesDifferenceSource)
+ self.assertContentEqual(
+ statuses, [
+ dsd.status
+ for dsd in dsd_source.getForDistroSeries(
+ derived_series, status=statuses)])
def test_getForDistroSeries_matches_by_package_name(self):
dsd = self.factory.makeDistroSeriesDifference()
@@ -1137,28 +1159,20 @@
def test_getForDistroSeries_filters_by_parent(self):
# The differences can be filtered by parent series.
- dsp = self.factory.makeDistroSeriesParent()
- derived_series = dsp.derived_series
- parent_series = dsp.parent_series
-
- # Add another parent to this series.
- parent_series2 = self.factory.makeDistroSeriesParent(
- derived_series=derived_series).parent_series
-
- diffs = self.makeDiffsForDistroSeries(
- derived_series, parent_series=parent_series)
- diffs2 = self.makeDiffsForDistroSeries(
- derived_series, parent_series=parent_series2)
-
- results = getUtility(
- IDistroSeriesDifferenceSource).getForDistroSeries(
- derived_series, parent_series=parent_series)
- results2 = getUtility(
- IDistroSeriesDifferenceSource).getForDistroSeries(
- derived_series, parent_series=parent_series2)
-
- self.assertContentEqual(diffs['normal'], results)
- self.assertContentEqual(diffs2['normal'], results2)
+ derived_series = self.factory.makeDistroSeries()
+ dsps = [
+ self.factory.makeDistroSeriesParent(derived_series=derived_series)
+ for counter in xrange(2)]
+ dsds = [
+ self.factory.makeDistroSeriesDifference(
+ parent_series=dsp.parent_series,
+ derived_series=derived_series)
+ for dsp in dsps]
+ dsd_source = getUtility(IDistroSeriesDifferenceSource)
+ self.assertContentEqual(
+ [dsds[0]],
+ dsd_source.getForDistroSeries(
+ derived_series, parent_series=dsps[0].parent_series))
def test_getForDistroSeries_matches_packageset(self):
dsd = self.factory.makeDistroSeriesDifference()
