Merge lp:~michael.nelson/ubuntu-webcatalog/777871-pull-other-apps-from-apt-cache into lp:ubuntu-webcatalog

[Michael Nelson]

Enables tests to run in under 3 seconds (rather than 35) by using a mock apt-cache, and uses lucid-compat cache update.

Overview
========
This branch doesn't do what the name implies, it was going to, but got sidetracked ;)

This branch updates the tests so that they run fast again (each of the command tests was doing an apt-cache update taking 3-4 seconds), for me 35secs in total, back so that they run in under 3 seconds. I've done this by adding a mock apt-cache for the tests, but also included a switch so you can run the tests against the real apt-cache if we need to.

It also updates the creation of the on-disk apt caches so that they are only created if they don't already exist, otherwise updated.

Details
=======

After chatting with mvo:

11:54 < noodles> mvo: hi! For the web catalog, we're currently doing: cache.update(sources_list=tmp_sources_list)
11:55 < noodles> to create a cache for whichever distroseries we need... but it seems that doesn't work on lucid:
11:55 < noodles> https://pastebin.canonical.com/48646/
11:55 < noodles> Is there a way to get the same effect on lucid?
11:55 < mvo> noodles: yes
11:56 < mvo> noodles: its best to use "cache = apt.Cache(rootdir=./path/to/temp-rootdir-with-etc-apt-sources.list/)
11:56 < mvo> noodles: you can keep this dir then, i.e. instead of making it a temp one make it permanent
11:57 < mvo> noodles: this will speed up the process of updates as it will use the binary cache in there and also use if-modified-since when doing the cache.update()
11:57 < mvo> noodles: and it will work on lucid :)
11:57 < noodles> Oh, we're already setting the rootdir, was the sources_list unecessary.. OK.
11:57 < noodles> Ah, sweet, I'll update it to do that then. Thanks!

I updated the on-disk cache to be more permanent (created if it doesn't exist, but otherwise just updated).

To test: follow the readme to bootstrap, then `fab test`

[lp:~michael.nelson/ubuntu-webcatalog/777871-pull-other-apps-from-apt-cache]

30. By Michael Nelson on 2011-06-17

REFACTOR: Moved patch of setting to setup to avoid touching local dev caches.

31. By Michael Nelson on 2011-06-17

REFACTOR: cleaned up the setUp into a few helper methods.

32. By Michael Nelson on 2011-06-17

REFACTOR: enable running the tests against the real apt-cache, rather than a mock.

33. By Michael Nelson on 2011-06-17

Fixed incorrect mock assertion.

34. By Michael Nelson on 2011-06-17

Reverted changed requirement for dev version of mock.

35. By Michael Nelson on 2011-06-17

fixed typo in comment for use_mock_apt_cache attribute

36. By Michael Nelson on 2011-06-17

Extracted our code that creates the apt-cache so it reads better and also makes it easier to test whether the apt-cache-creating code was called.

[Danny Tamez (zematynnad)]

Much much faster. Works great.

review: Approve

=== modified file 'django_project/settings.py'
--- django_project/settings.py	2011-05-06 15:03:41 +0000
+++ django_project/settings.py	2011-06-17 14:39:31 +0000
@@ -147,6 +147,7 @@
 
 # Strictly for use in our dev environment:
 SERVE_SITE_MEDIA = True
+DISK_APT_CACHE_LOCATION = '/tmp/webcat_cache'
 
 try:
     from local_settings import *

=== modified file 'src/webcatalog/management/commands/import_app_install_data.py'
--- src/webcatalog/management/commands/import_app_install_data.py	2011-06-07 20:07:04 +0000
+++ src/webcatalog/management/commands/import_app_install_data.py	2011-06-17 14:39:31 +0000
@@ -32,6 +32,7 @@
 import apt
 from apt.cache import FetchFailedException
 from apt_inst import DebFile
+from django.conf import settings
 from django.core.files.images import ImageFile
 from django.forms.models import construct_instance
 from django.core.management.base import LabelCommand
@@ -52,21 +53,39 @@
     def __init__(self, distroseries):
         self.distroseries = distroseries
 
+    def create_apt_cache(self, series_cache_location):
+        os.makedirs(os.path.join(series_cache_location, 'etc', 'apt'))
+        sources_list = os.path.join(
+            series_cache_location, 'etc', 'apt', 'sources.list')
+        with open(sources_list, "w+") as sources_list_file:
+            sources_list_file.write(
+                'deb http://archive.ubuntu.com/ubuntu {0} main\n'.format(
+                    self.distroseries))
+
+    def get_apt_cache(self):
+        # Create on-disk cache for distroseries if it
+        # doesn't yet exist.
+        series_cache_location = os.path.join(
+            settings.DISK_APT_CACHE_LOCATION, self.distroseries)
+        if not os.path.exists(series_cache_location):
+            self.create_apt_cache(series_cache_location)
+
+        return apt.Cache(rootdir=series_cache_location)
+
     def __enter__(self):
         tmp_sources_dir = tempfile.mkdtemp()
         self.tmp_sources_dir = tmp_sources_dir
         os.makedirs(os.path.join(tmp_sources_dir, 'etc', 'apt'))
         tmp_sources_list = os.path.join(
             tmp_sources_dir, 'etc', 'apt', 'sources.list')
-        sources_list = open(tmp_sources_list, 'w+')
-        sources_list.write(
-            'deb http://archive.ubuntu.com/ubuntu {0} main\n'.format(
-                self.distroseries))
-        sources_list.close()
+        with open(tmp_sources_list, "w+") as sources_list:
+            sources_list.write(
+                'deb http://archive.ubuntu.com/ubuntu {0} main\n'.format(
+                    self.distroseries))
 
-        cache = apt.Cache(rootdir=tmp_sources_dir, memonly=True)
+        cache = self.get_apt_cache()
         try:
-            cache.update(sources_list=tmp_sources_list)
+            cache.update()
         except FetchFailedException:
             # Make sure __exit__ still gets called
             pass
@@ -94,6 +113,8 @@
         with Cache(distroseries_name) as cache:
             self.verbosity = int(options['verbosity'])
             data_dir = tempfile.mkdtemp()
+
+            # First create the apps based on the current cache.
             distroseries, created = DistroSeries.objects.get_or_create(
                 code_name=distroseries_name)
             if created and self.verbosity > 0:
@@ -160,24 +181,25 @@
 
         if form.is_valid():
             # Check if the app already exists in our DB.
-            existing_apps = Application.objects.filter(
-                package_name=form.cleaned_data['package_name'],
-                distroseries=distroseries)
-            if existing_apps.count() > 0:
+            try:
+                app = Application.objects.get(
+                    package_name=form.cleaned_data['package_name'],
+                    distroseries=distroseries)
                 # Force the form to update the existing application.
                 # Simply setting the forms.instance doesn't work as the
                 # form constructs its instance during is_valid, and
                 # doesn't reconstruct it to save.
-                app = construct_instance(form, existing_apps[0])
-            else:
+                app = construct_instance(form, app)
+            except Application.DoesNotExist:
                 app = form.save(commit=False)
                 app.distroseries = distroseries
+
             # we don't have the description - just the desktop data, so:
             # use the package_name to lookup the app in the cache and get
             # the descrption from there
             try:
                 cached_app = cache[app.package_name]
-                app.description = cached_app.description
+                app.description = cached_app.candidate.description
             except KeyError:
                 pass
             app.save()

=== modified file 'src/webcatalog/tests/test_commands.py'
--- src/webcatalog/tests/test_commands.py	2011-06-07 20:07:04 +0000
+++ src/webcatalog/tests/test_commands.py	2011-06-17 14:39:31 +0000
@@ -21,20 +21,28 @@
     absolute_import,
     with_statement,
     )
-from apt.cache import Cache
+import apt
 import os
 import shutil
 import tempfile
 
+from django.conf import settings
 from django.core.management import call_command
+from django.test import TestCase
 from mock import (
     patch,
     MagicMock,
     Mock,
     )
 
-from webcatalog.models import Application
-from webcatalog.management.commands.import_app_install_data import Command
+from webcatalog.models import (
+    Application,
+    DistroSeries,
+    )
+from webcatalog.management.commands.import_app_install_data import (
+    Cache as CacheContextManager,
+    Command,
+    )
 from webcatalog.tests.factory import TestCaseWithFactory
 
 __metaclass__ = type
@@ -45,6 +53,76 @@
 
 class ImportAppInstallTestCase(TestCaseWithFactory):
 
+    # If you want to run these tests against the real apt cache (which
+    # is slower, but the real deal) you can update the following
+    # attribute. Note: 3 tests will fail which make assumptions about
+    # the uri for apt-install-data or the value of descriptions for
+    # firefox and scribus in the cache.
+    use_mock_apt_cache = True
+
+    def make_mock_apt_package(self, description, uri=''):
+        """Helper to DRY up creating a mock apt package."""
+        mock_package = Mock(spec=apt.package.Package)
+        mock_package.candidate.description = description
+        mock_package.candidate.uri = uri
+        return mock_package
+
+    def make_mock_apt_cache(self):
+        """Helper creating a mock apt_cache object.
+
+        And keeping the setUp readable.
+        """
+        mock_cache = Mock()
+        mock_apt_firefox = self.make_mock_apt_package(
+            description = "Firefox description")
+        mock_apt_scribus = self.make_mock_apt_package(
+            description = "Scribus description")
+        mock_other_app = self.make_mock_apt_package(
+            description = "Otherapp description")
+        mock_app_install_data = self.make_mock_apt_package(
+            description = "App install desc.",
+            uri = 'http://example.com/app-install-1.01.deb')
+        cache_dict = {
+            'firefox': mock_apt_firefox,
+            'scribus': mock_apt_scribus,
+            'otherapp': mock_other_app,
+            'app-install-data': mock_app_install_data,
+            }
+
+        def getitem(self, key):
+            return cache_dict.get(key, None)
+
+        mock_cache.__getitem__ = getitem
+        return mock_cache
+
+    def setUp(self):
+        """Create a mock apt-cache so we don't wait for the update."""
+        super(ImportAppInstallTestCase, self).setUp()
+
+        # We use a temporary on-disk cache location for our
+        # tests.
+        self.tmp_apt_cache = tempfile.mkdtemp()
+        self.patch_cache_setting = patch.object(settings,
+            'DISK_APT_CACHE_LOCATION', self.tmp_apt_cache)
+        self.patch_cache_setting.start()
+
+        if self.use_mock_apt_cache:
+            # The apt.Cache class is patched to return our own mock which
+            # will be subscriptable with a pre-set dict with mock ock apt
+            # applications for firefox and scribus.
+            self.patch_cache = patch('apt.Cache')
+            self.mock_cache_class = self.patch_cache.start()
+            self.mock_cache = self.make_mock_apt_cache()
+            self.mock_cache_class.return_value = self.mock_cache
+
+
+    def tearDown(self):
+        self.patch_cache_setting.stop()
+        shutil.rmtree(self.tmp_apt_cache)
+        if self.use_mock_apt_cache:
+            self.patch_cache.stop()
+            super(ImportAppInstallTestCase, self).tearDown()
+
     def test_app_data_downloaded_by_default(self):
         get_data_fn = ('webcatalog.management.commands.'
                        'import_app_install_data.Command.'
@@ -58,7 +136,8 @@
         args = mock_get_data_fn.call_args[0]
         self.assertEqual('onion', args[0])
         self.assertTrue(args[1].startswith('/tmp/tmp'))
-        self.assertTrue(isinstance(args[2], Cache))
+        if self.use_mock_apt_cache:
+            self.assertEqual(self.mock_cache, args[2])
 
     def test_get_latest_app_data_for_series(self):
         # Requested and downloaded from the server.
@@ -166,20 +245,9 @@
             scribus.icon.size)
 
     def test_get_app_data_uri_for_series(self):
-        # Note: you can test this with the real cache by commenting out
-        # the mocks - it just takes 4-5 seconds or so.
-        apt_cache = (
-            'webcatalog.management.commands.import_app_install_data.'
-            'apt.Cache')
-        with patch(apt_cache) as mock_apt_cache_cls:
-            mock_cache_item = Mock()
-            mock_cache_item.candidate.uri = 'http://example.com/my.deb'
-            mock_cache = MagicMock()
-            mock_cache.__getitem__.return_value = mock_cache_item
-            mock_apt_cache_cls.return_value = mock_cache
-            uri = Command().get_app_data_uri_for_series('natty')
+        uri = Command().get_app_data_uri_for_series('natty')
 
-        self.assertEqual('http://example.com/my.deb', uri)
+        self.assertEqual('http://example.com/app-install-1.01.deb', uri)
 
     def test_description_added_to_app(self):
         self.assertEqual(0, Application.objects.count())
@@ -193,12 +261,10 @@
         self.assertEqual(2, Application.objects.count())
         firefox = Application.objects.get(package_name='firefox')
         scribus = Application.objects.get(package_name='scribus')
-        firefox_description = 'Firefox delivers safe, easy web browsing.'
-        scribus_description = 'Scribus is an open source desktop page layout'
-        self.assertTrue(firefox.description.find(firefox_description) > -1)
-        self.assertTrue(scribus.description.find(scribus_description) > -1)
+        self.assertEqual("Firefox description", firefox.description)
+        self.assertEqual("Scribus description", scribus.description)
 
-    def test_descdription_updated_in_app(self):
+    def test_description_updated_in_app(self):
         app = self.factory.make_application(package_name='scribus',
             distroseries=self.factory.make_distroseries(code_name='natty'))
 
@@ -212,5 +278,42 @@
         self.assertEqual(
             'Graphic Page Layout and Publication (Stable)',
             app_reloaded.comment)
-        scribus_description = 'Scribus is an open source desktop page layout'
-        self.assertTrue(app_reloaded.description.find(scribus_description) > -1)
+        self.assertEqual("Scribus description", app_reloaded.description)
+
+    def test_cache_context_manager_creates_sources_list(self):
+        # If the on-disk cache doesn't yet exist, it is created.
+        call_command(
+            'import_app_install_data', 'natty',
+            local_app_install_deb=self.factory.get_test_path(
+                'app-install-data-test_all.deb'),
+            verbosity=0)
+
+        natty_cache = os.path.join(self.tmp_apt_cache, 'natty')
+        self.assertTrue(os.path.exists(os.path.join(
+            natty_cache, 'etc', 'apt', 'sources.list')))
+        if self.use_mock_apt_cache:
+            self.mock_cache_class.assert_called_with(rootdir=natty_cache)
+            self.assertEqual(1, self.mock_cache.update.call_count)
+
+    def test_cache_context_manager_doesnt_overwrite_existing(self):
+        # If an on-disk cache for the distroseries already exists, a
+        # new one is *not* created.
+        series_cache = os.path.join(self.tmp_apt_cache, 'testwebcatseries')
+        if os.path.exists(series_cache):
+            shutil.rmtree(series_cache)
+        os.makedirs(series_cache)
+
+        create_apt_cache_fn = (
+            'webcatalog.management.commands.import_app_install_data.'
+            'Cache.create_apt_cache')
+        with patch(create_apt_cache_fn) as mock_create_apt_cache:
+            call_command(
+                'import_app_install_data', 'testwebcatseries',
+                local_app_install_deb=self.factory.get_test_path(
+                    'app-install-data-test_all.deb'),
+                verbosity=0)
+
+        self.assertEqual(0, mock_create_apt_cache.call_count)
+        if self.use_mock_apt_cache:
+            self.mock_cache_class.assert_called_with(rootdir=series_cache)
+            self.assertEqual(1, self.mock_cache.update.call_count)