Merge lp:~benji/landscape-client/bug-1548946-xenial-test-failures into lp:~landscape/landscape-client/trunk

[Benji York]

This branch fixes two test failures and one non-failure on Xenial.

The simplest failure was a simple difference in deb stanza ordering
that caused a failure of test_get_package_stanza. I changed the test
to store the stanzas in sets and compared those instead so as to be
order-agnostic.

The next failure was caused by a change in the way the apt cache works.
The test_reload_channels_not_refetch_package_index test was using a
change in package visibility to the apt cache to tell when the cache's
update() method had been called. The cache behaviour has changed so
that package existance changes are now reflected earlier than before.
Therefore, I added a test-only subclass of the cache which records the
fact that it's update method has been called, which is the operative
piece of information for the test.

The change to the cache behaviour also meant that
test_reload_channels_force_reload_binaries would pass even if the call
to update() were removed, so I used the cache subclass in that test as
well.

This branch fixes two test failures and one non-failure on Xenial.

The simplest failure was a simple difference in deb stanza ordering
that caused a failure of test_get_package_stanza. I changed the test
to store the stanzas in sets and compared those instead so as to be
order-agnostic.

The next failure was caused by a change in the way the apt cache works.
The test_reload_channels_not_refetch_package_index test was using a
change in package visibility to the apt cache to tell when the cache's
update() method had been called. The cache behaviour has changed so
that package existance changes are now reflected earlier than before.
Therefore, I added a test-only subclass of the cache which records the
fact that it's update method has been called, which is the operative
piece of information for the test.

The change to the cache behaviour also meant that
test_reload_channels_force_reload_binaries would pass even if the call
to update() were removed, so I used the cache subclass in that test as
well.

Testing instructions:

Before this branch running

    trial landscape.package.tests.test_facade.AptFacadeTest

would result in failing tests. Now it shouldn't.

To ensure the test that never failed before will actually fail, remove
or comment out the calls to self._cache.update() in
landscape/package/facade.py and invoke this test run:

    trial landscape.package.tests.test_facade.AptFacadeTest.test_reload_channels_force_reload_binaries

[lp:~benji/landscape-client/bug-1548946-xenial-test-failures]

832. By Benji York on 2016-03-04

Fix tests

[Free Ekanayaka (free.ekanayaka)]

I'm basically +1 with a non-blocking nitpick.

Just marking as Needs information because of the following question.

Since it seems you investigated the issue in depth and possible looked at the changed behavior in the apt module, do you think it would be possible to avoid using the TestCache and change the test to adapt to the new behavior somehow? Or is it now plain impossible for calling code to have visibility on that?

review: Needs Information

[Benji York (benji)]

> I'm basically +1 with a non-blocking nitpick.
>
> Just marking as Needs information because of the following question.
>
> Since it seems you investigated the issue in depth and possible looked at the
> changed behavior in the apt module, do you think it would be possible to avoid
> using the TestCache and change the test to adapt to the new behavior somehow?
> Or is it now plain impossible for calling code to have visibility on that?

I spent a (timeboxed) bit of time trying to discern exactly how the cache's behaviour has changed, and I couldn't figure enough out to exploit it for our tests.

Arguably, exploiting said details is what got us into this situation, but I wouldn't argue that too strenuously.

[lp:~benji/landscape-client/bug-1548946-xenial-test-failures]

833. By Benji York on 2016-03-09

switch from comparing sets to comparing lists in an order-independent way

[Free Ekanayaka (free.ekanayaka)]

> > I'm basically +1 with a non-blocking nitpick.
> >
> > Just marking as Needs information because of the following question.
> >
> > Since it seems you investigated the issue in depth and possible looked at
> the
> > changed behavior in the apt module, do you think it would be possible to
> avoid
> > using the TestCache and change the test to adapt to the new behavior
> somehow?
> > Or is it now plain impossible for calling code to have visibility on that?
>
> I spent a (timeboxed) bit of time trying to discern exactly how the cache's
> behaviour has changed, and I couldn't figure enough out to exploit it for our
> tests.
>
> Arguably, exploiting said details is what got us into this situation, but I
> wouldn't argue that too strenuously.

Right, has all integration-level tests they have a cost and they get to failure situations when the other party changes, but flip side is that they increase confidence in the behavior of the external system (the apt python library in this case) and the way your code in interacts with it.

I'm approving this branch, but I'd suggest to file a bug about this and put an # XXX comment pointing to the bug inside the class docstring of the newly introduced TestCache.

+1

review: Approve

[Free Ekanayaka (free.ekanayaka)]

> > > I'm basically +1 with a non-blocking nitpick.
> > >
> > > Just marking as Needs information because of the following question.
> > >
> > > Since it seems you investigated the issue in depth and possible looked at
> > the
> > > changed behavior in the apt module, do you think it would be possible to
> > avoid
> > > using the TestCache and change the test to adapt to the new behavior
> > somehow?
> > > Or is it now plain impossible for calling code to have visibility on that?
> >
> > I spent a (timeboxed) bit of time trying to discern exactly how the cache's
> > behaviour has changed, and I couldn't figure enough out to exploit it for
> our
> > tests.
> >
> > Arguably, exploiting said details is what got us into this situation, but I
> > wouldn't argue that too strenuously.
>
> Right, has all integration-level tests they have a cost and they get to
> failure situations when the other party changes, but flip side is that they
> increase confidence in the behavior of the external system (the apt python
> library in this case) and the way your code in interacts with it.
>
> I'm approving this branch, but I'd suggest to file a bug about this and put an
> # XXX comment pointing to the bug inside the class docstring of the newly
> introduced TestCache.
>
> +1

Ideally the class docstring of TestCache should also explain the background of why it was introduced, summarizing the content of the bug its pointing to (in case one wants to get the full context).

[lp:~benji/landscape-client/bug-1548946-xenial-test-failures]

834. By Benji York on 2016-03-11

add docstring describing the origin of a test helper

[Chris Glass (tribaal)]

+1!

review: Approve

=== modified file 'landscape/package/tests/helpers.py'
--- landscape/package/tests/helpers.py	2015-03-03 14:16:05 +0000
+++ landscape/package/tests/helpers.py	2016-03-11 12:06:26 +0000
@@ -86,7 +86,7 @@
             version=version, control_fields=control_fields)
 
     def _touch_packages_file(self, deb_dir):
-        """Make sure the Packages file get a newer mtime value.
+        """Make sure the Packages file gets a newer mtime value.
 
         If we rely on simply writing to the file to update the mtime, we
         might end up with the same as before, since the resolution is

=== modified file 'landscape/package/tests/test_facade.py'
--- landscape/package/tests/test_facade.py	2015-04-15 13:16:20 +0000
+++ landscape/package/tests/test_facade.py	2016-03-11 12:06:26 +0000
@@ -3,6 +3,7 @@
 import textwrap
 import tempfile
 
+import apt
 import apt_pkg
 from apt.package import Package
 from aptsources.sourceslist import SourcesList
@@ -43,6 +44,23 @@
         self.description = description
 
 
+class TestCache(apt.cache.Cache):
+    """An apt cache wrapper which we can tell has been updated.
+
+    When updating the client to work with Xenial, apt.cache.Cache behaviour
+    which tests depended on changed in such a way that we could no longer
+    easily tell from the outside if the cache had been updated or not.  This
+    wrapper was introduced to regain that ability.  See bug 1548946 for more
+    information.
+    """
+
+    _update_called = False
+
+    def update(self):
+        self._update_called = True
+        return super(TestCache, self).update()
+
+
 class AptFacadeTest(LandscapeTest):
 
     helpers = [AptFacadeHelper, EnvironSaverHelper]
@@ -267,10 +285,10 @@
         deb_dir = self.makeDir()
         create_deb(deb_dir, PKGNAME1, PKGDEB1)
         deb_file = os.path.join(deb_dir, PKGNAME1)
-        stanza = self.facade.get_package_stanza(deb_file)
+        stanza = self.facade.get_package_stanza(deb_file).split("\n")
         SHA256 = (
             "f899cba22b79780dbe9bbbb802ff901b7e432425c264dc72e6bb20c0061e4f26")
-        self.assertEqual(textwrap.dedent("""\
+        self.assertItemsEqual(textwrap.dedent("""\
             Package: name1
             Priority: optional
             Section: Group1
@@ -291,7 +309,7 @@
             SHA256: %(sha256)s
             Description: Summary1
              Description1
-            """ % {"filename": PKGNAME1, "sha256": SHA256}),
+            """ % {"filename": PKGNAME1, "sha256": SHA256}).split("\n"),
             stanza)
 
     def test_add_channel_deb_dir_creates_packages_file(self):
@@ -418,14 +436,12 @@
         self.facade.add_channel_apt_deb("file://%s" % deb_dir, "./")
         self.facade.reload_channels()
         new_facade = AptFacade(root=self.apt_root)
-        self._add_package_to_deb_dir(deb_dir, "bar")
+        self._add_package_to_deb_dir(deb_dir, "foo", version="2.0")
         self._touch_packages_file(deb_dir)
         new_facade.refetch_package_index = False
+        new_facade._cache = TestCache(rootdir=new_facade._root)
         new_facade.reload_channels()
-        self.assertEqual(
-            ["foo"],
-            sorted(version.package.name
-                   for version in new_facade.get_packages()))
+        self.assertFalse(new_facade._cache._update_called)
 
     def test_reload_channels_force_reload_binaries(self):
         """
@@ -440,11 +456,9 @@
         self._add_package_to_deb_dir(deb_dir, "bar")
         self._touch_packages_file(deb_dir)
         self.facade.refetch_package_index = False
+        self.facade._cache = TestCache(rootdir=self.facade._root)
         self.facade.reload_channels(force_reload_binaries=True)
-        self.assertEqual(
-            ["bar", "foo"],
-            sorted(version.package.name
-                   for version in self.facade.get_packages()))
+        self.assertTrue(self.facade._cache._update_called)
 
     def test_reload_channels_no_force_reload_binaries(self):
         """