Merge lp:~mvo/software-center/fix-more-dep8 into lp:software-center

[Michael Vogt]

Some fixes related to the dep8 jenkins failures from:
https://jenkins.qa.ubuntu.com/job/quantal-adt-software-center/

This fixes the failures releated to the fact that adt runs the tests
as root so some stuff like "os.access()" will always work

[Gary Lasker (gary-lasker)]

This is nice and clean. I really like how unittest.skipIf supports adding a description for the reason that a test is being skipped (and I like the descriptions you've used for them). Very nice, thank you, Michael!

review: Approve

=== modified file 'softwarecenter/backend/installbackend_impl/aptd.py'
--- softwarecenter/backend/installbackend_impl/aptd.py	2012-08-23 12:25:44 +0000
+++ softwarecenter/backend/installbackend_impl/aptd.py	2012-08-24 09:09:38 +0000
@@ -87,6 +87,7 @@
 
 
 class AptdaemonTransaction(BaseTransaction):
+
     def __init__(self, trans):
         self._trans = trans
 
@@ -210,6 +211,8 @@
                                         (GObject.TYPE_PYOBJECT, bool,)),
                     }
 
+    LICENSE_KEY_SERVER = "ubuntu-production"
+
     def __init__(self):
         GObject.GObject.__init__(self)
 
@@ -258,7 +261,7 @@
                 TransactionTypes.REPAIR)
             yield self._run_transaction(trans, None, None, None)
         except Exception as error:
-            self._on_trans_error(error)
+            self._on_trans_error(error, trans)
 
     @inline_callbacks
     def fix_incomplete_install(self):
@@ -268,7 +271,7 @@
                       TransactionTypes.REPAIR)
             yield self._run_transaction(trans, None, None, None)
         except Exception as error:
-            self._on_trans_error(error)
+            self._on_trans_error(error, trans)
 
     # FIXME: upgrade add-ons here
     @inline_callbacks
@@ -285,7 +288,7 @@
             yield self._run_transaction(trans, pkgname, appname, iconname,
                 metadata)
         except Exception as error:
-            self._on_trans_error(error, pkgname)
+            self._on_trans_error(error, trans, pkgname)
 
 # broken
 #    @inline_callbacks
@@ -327,7 +330,7 @@
             yield self._run_transaction(trans, pkgname, appname, iconname,
                 metadata)
         except Exception as error:
-            self._on_trans_error(error, pkgname)
+            self._on_trans_error(error, trans, pkgname)
 
     @inline_callbacks
     def remove_multiple(self, apps, iconnames, addons_install=[],
@@ -371,7 +374,7 @@
             yield self._run_transaction(
                 trans, pkgname, appname, iconname, metadata)
         except Exception as error:
-            self._on_trans_error(error, pkgname)
+            self._on_trans_error(error, trans, pkgname)
 
     @inline_callbacks
     def install_multiple(self, apps, iconnames, addons_install=[],
@@ -401,7 +404,7 @@
                 TransactionTypes.APPLY)
             yield self._run_transaction(trans, pkgname, appname, iconname)
         except Exception as error:
-            self._on_trans_error(error)
+            self._on_trans_error(error, trans)
 
     @inline_callbacks
     def reload(self, sources_list=None, metadata=None):
@@ -420,7 +423,7 @@
                 sources_list=sources_list, defer=True)
             yield self._run_transaction(trans, None, None, None, metadata)
         except Exception as error:
-            self._on_trans_error(error)
+            self._on_trans_error(error, trans)
         # note that the cache re-open will happen via the connected
         # "transaction-finished" signal
 
@@ -433,7 +436,7 @@
             # signals
             yield trans.run(defer=True)
         except Exception as error:
-            self._on_trans_error(error, component)
+            self._on_trans_error(error, trans, component)
             return_value(None)
         # now update the cache
         yield self.reload()
@@ -472,7 +475,7 @@
                 keyid, keyserver, defer=True)
             yield self._run_transaction(trans, None, None, None, metadata)
         except Exception as error:
-            self._on_trans_error(error)
+            self._on_trans_error(error, trans)
 
     @inline_callbacks
     def add_sources_list_entry(self, source_entry, sourcepart=None):
@@ -533,9 +536,8 @@
             # system-wide keys
             try:
                 self._logger.info("adding license key for '%s'" % pkgname)
-                server = "ubuntu-production"
                 trans = yield self.aptd_client.add_license_key(
-                    pkgname, license_key_oauth, server)
+                    pkgname, license_key_oauth, self.LICENSE_KEY_SERVER)
                 yield self._run_transaction(trans, None, None, None)
             except Exception as e:
                 self._logger.error("add_license_key: '%s'" % e)
@@ -697,7 +699,7 @@
                 yield self._run_transaction(trans, app.pkgname, app.appname,
                                             "", metadata)
             except Exception as error:
-                self._on_trans_error(error, app.pkgname)
+                self._on_trans_error(error, trans, app.pkgname)
             # add license_key
             # FIXME: aptd fails if there is a license_key_path already
             #        but I wonder if we should ease that restriction
@@ -959,7 +961,7 @@
                 yield trans.set_meta_data(defer=True, **metadata)
             yield trans.run(defer=True)
         except Exception as error:
-            self._on_trans_error(error, pkgname)
+            self._on_trans_error(error, trans, pkgname)
             # on error we need to clean the pending purchases
             self._clean_pending_purchases(pkgname)
         # on success the pending purchase is cleaned when the package
@@ -971,8 +973,9 @@
         if pkgname and pkgname in self.pending_purchases:
             del self.pending_purchases[pkgname]
 
-    def _on_trans_error(self, error, pkgname=None):
-        self._logger.warn("_on_trans_error: %s", error)
+    def _on_trans_error(self, error, trans, pkgname=None):
+        self._logger.warn("_on_trans_error: '%s' '%s' '%s'" % (
+                error, trans, pkgname))
         # re-enable the action button again if anything went wrong
         result = TransactionFinishedResult(None, False)
         result.pkgname = pkgname

=== modified file 'tests/test_aptd.py'
--- tests/test_aptd.py	2012-08-23 12:18:43 +0000
+++ tests/test_aptd.py	2012-08-24 09:09:38 +0000
@@ -9,6 +9,8 @@
 
 import dbus
 
+from gi.repository import GObject
+
 from tests.utils import (
     setup_test_env,
 )
@@ -42,6 +44,7 @@
         data = "some-data"
         # test HOME
         target = "~/.fasfasdfsdafdfsdafdsfa"
+        self.addCleanup(lambda: os.remove(os.path.expanduser(target)))
         pkgname = "2vcard"
         json_auth = ""
         yield self.aptd.add_license_key(data, target, json_auth, pkgname)
@@ -50,26 +53,29 @@
         data2 = "other-data"
         yield self.aptd.add_license_key(data2, target, json_auth, pkgname)
         self.assertEqual(open(os.path.expanduser(target)).read(), data)
-        # cleanup
-        os.remove(os.path.expanduser(target))
 
+    @unittest.skipIf(os.getuid() != 0, 
+                     "test_add_license_key_opt test needs to run as root")
+    @unittest.skipIf(not "SC_TEST_JSON" in os.environ,
+                     "Need a SC_TEST_JSON environment with the credentials")
     def test_add_license_key_opt(self):
-        if os.getuid() != 0:
-            logging.info("skipping add_license_key_opt test")
-            return
         # test /opt
-        data = "some-data"
+        license_key = "some-data"
         pkgname = "hellox"
         path = "/opt/hellox/conf/license-key.txt"
+        self.addCleanup(lambda: os.remove(path))
         json_auth = os.environ.get("SC_TEST_JSON") or "no-json-auth"
         def _error(*args):
             print "errror", args
         self.aptd.ui = Mock()
+        self.aptd.LICENSE_KEY_SERVER = "ubuntu-staging"
         self.aptd.ui.error = _error
         @inline_callbacks
         def run():
-            yield self.aptd.add_license_key(data, path, json_auth, pkgname)
-            aptdaemon.loop.mainloop.quit()
+            yield self.aptd.add_license_key(
+                license_key, path, json_auth, pkgname)
+            # ensure signals get delivered before quit()
+            GObject.timeout_add(500, lambda: aptdaemon.loop.mainloop.quit())
         # run the callback
         run()
         aptdaemon.loop.mainloop.run()
@@ -111,7 +117,6 @@
         addons_remove = ["gimp-plugin-registry"]
         yield self.aptd.apply_changes(pkgname, appname ,iconname, addons_install, addons_remove)
 
-
     def test_trans_error_ui_display(self):
         """ test if the right error ui is displayed for various dbus 
             errors
@@ -127,7 +132,7 @@
                 error_ui_mock.reset()
                 dbus_name_mock.return_value = dbus_name
                 error.get_dbus_name = dbus_name_mock
-                self.aptd._on_trans_error(error)
+                self.aptd._on_trans_error(error, Mock())
                 self.assertEqual(error_ui_mock.called, show_error_ui)
 
     @inline_callbacks
@@ -164,4 +169,5 @@
 
 
 if __name__ == "__main__":
+    logging.basicConfig(level=logging.INFO)
     unittest.main()

=== modified file 'tests/test_logging.py'
--- tests/test_logging.py	2012-05-30 18:39:55 +0000
+++ tests/test_logging.py	2012-08-24 09:09:38 +0000
@@ -17,6 +17,9 @@
 class TestLogging(unittest.TestCase):
     """Tests for the sc logging facilities."""
 
+    @unittest.skipIf(
+        os.getuid() == 0, 
+        "fails when run as root as os.access() is always successful")
     def test_no_write_access_for_cache_dir(self):
         """Test for bug LP: #688682."""
         cache_dir = './foo'

=== modified file 'tests/test_utils.py'
--- tests/test_utils.py	2012-08-21 09:05:53 +0000
+++ tests/test_utils.py	2012-08-24 09:09:38 +0000
@@ -189,6 +189,9 @@
         # ensure it does not crash for empty strings, LP: #1002271
         self.assertEqual(capitalize_first_word(""), "")
 
+    @unittest.skipIf(
+        os.getuid() == 0, 
+        "fails when run as root as os.access() is always successful")
     def test_ensure_file_writable_and_delete_if_not(self):
         # first test that a non-writable file (0400) is deleted
         test_file_not_writeable = NamedTemporaryFile()
@@ -203,6 +206,9 @@
         ensure_file_writable_and_delete_if_not(test_file_writeable.name)
         self.assertTrue(os.path.exists(test_file_writeable.name))
 
+    @unittest.skipIf(
+        os.getuid() == 0, 
+        "fails when run as root as os.chmod() is always successful")
     def test_safe_makedirs(self):
         tmp = mkdtemp()
         # create base dir