launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-757493 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #757493 in Launchpad itself: "+localpackagediffs shows identical versions as differences where the SPR ID is different"
  https://bugs.launchpad.net/launchpad/+bug/757493

For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-757493/+merge/57302

= Summary =

We're getting some inappropriate DistroSeriesDifferences because the script that populates the table compares releases by ID, not by version string.  Sometimes two SPPHs for a parent and derived distroseries can have separate SPRs for the same package version.  These are actually considered identical, but they are shown as version differences.


== Proposed fix ==

Compare the latest SPRs for a sourcepackagename between a parent and derived series on version, rather than on id.


== Pre-implementation notes ==

StevenK addressed the same issue in the script that fills out the differences, and also compared on version.  In an earlier discussion we had expected SPR.id to be as valid a comparison as SPR.version, but it turned out we were optimistic.


== Implementation details ==

The new comparison may seem oddly phrased.  That's because "parent.version = derived.version" produces a tri-state boolean: true (equal), false (not equal), or null (not both present).  The "is not true" matches both on the case where version strings differ and on the case where one or the other is null, but not on the case where the version strings are equal.


== Tests ==

This already exercises the cases for same SPR; different versions; and missing packages.  I added a test for the case where a package is present on both sides, with separate SPRs, but the same version string.

{{{
./bin/test -vvc lp.registry.scripts.tests.test_populate_distroseriesdiff
}}}


== Demo and Q/A ==

The populate-distroseriesdiff script (scripts/populate-distroseriesdiff.py) will no longer create DSDs for packages that are released separately in a parent and derived distroseries with the same version number.  It should, however, still create them for packages that are only in the parent series; only in the derived series; or actually different between the two.


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/scripts/populate_distroseriesdiff.py
  lib/lp/registry/scripts/tests/test_populate_distroseriesdiff.py


Jeroen
-- 
https://code.launchpad.net/~jtv/launchpad/bug-757493/+merge/57302
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-757493 into lp:launchpad.

=== modified file 'lib/lp/registry/scripts/populate_distroseriesdiff.py'
--- lib/lp/registry/scripts/populate_distroseriesdiff.py	2011-03-14 12:03:57 +0000
+++ lib/lp/registry/scripts/populate_distroseriesdiff.py	2011-04-12 10:58:37 +0000
@@ -106,9 +106,7 @@
         FROM (%(parent_query)s) AS parent
         FULL OUTER JOIN (%(derived_query)s) AS derived
         ON derived.sourcepackagename = parent.sourcepackagename
-        WHERE
-            derived.sourcepackagerelease IS DISTINCT FROM
-                parent.sourcepackagerelease
+        WHERE (derived.version = parent.version) IS NOT TRUE
         """ % parameters
 
 

=== modified file 'lib/lp/registry/scripts/tests/test_populate_distroseriesdiff.py'
--- lib/lp/registry/scripts/tests/test_populate_distroseriesdiff.py	2011-03-14 12:03:57 +0000
+++ lib/lp/registry/scripts/tests/test_populate_distroseriesdiff.py	2011-04-12 10:58:37 +0000
@@ -243,6 +243,24 @@
         query = compose_sql_find_differences(distroseries)
         self.assertContentEqual([], Store.of(distroseries).execute(query))
 
+    def test_ignores_releases_for_same_version(self):
+        derived_series = self.makeDerivedDistroSeries()
+        version_string = self.factory.getUniqueString()
+        parent_series = derived_series.parent_series
+        package = self.factory.makeSourcePackageName()
+        self.makeSPPH(
+            distroseries=derived_series,
+            sourcepackagerelease=self.factory.makeSourcePackageRelease(
+                sourcepackagename=package, distroseries=derived_series,
+                version=version_string))
+        self.makeSPPH(
+            distroseries=parent_series,
+            sourcepackagerelease=self.factory.makeSourcePackageRelease(
+                sourcepackagename=package, distroseries=parent_series,
+                version=version_string))
+        query = compose_sql_find_differences(derived_series)
+        self.assertContentEqual([], Store.of(derived_series).execute(query))
+
     def test_finds_release_missing_in_derived_series(self):
         distroseries = self.makeDerivedDistroSeries()
         spph = self.makeSPPH(distroseries=distroseries.parent_series)