Merge lp:~mfisch/ubuntu-accomplishments-daemon/ubuntu-accomplishments-daemon-parse-fix into lp:ubuntu-accomplishments-daemon

[Matt Fischer]

Fixes the broken unit tests, which behaved differently depending on whether you had a daemon running on your system. (LP: 1030208

Fixes the daemon crash when there is a parse error in the .accomplishment file. I've modified the unit tests to account for this. You can see where I wrapped the Parse exceptions with a try/except block. Ideally I think we could move large chunks of this piece of code to separate functions.

If you test this, please run the unit tests with and without a daemon running on your system.

[lp:~mfisch/ubuntu-accomplishments-daemon/ubuntu-accomplishments-daemon-parse-fix]

118. By Matt Fischer on 2012-07-30

minor fix to the error message

=== modified file 'accomplishments/daemon/api.py'
--- accomplishments/daemon/api.py	2012-07-23 16:27:15 +0000
+++ accomplishments/daemon/api.py	2012-07-30 17:26:22 +0000
@@ -938,24 +938,24 @@
                 if collection in self.accDB:
                     # This collection has already been loaded from another install path!
                     continue
-                
+
                 collpath = os.path.join(path,collection)
                 aboutpath = os.path.join(collpath,'ABOUT')
-                
+
                 # Load data from ABOUT file
                 cfg = ConfigParser.RawConfigParser()
                 cfg.read(aboutpath)
-                
+
                 if not (cfg.has_option("general","langdefault") and cfg.has_option("general","name")):
                     print aboutpath
                     raise LookupError("Accomplishment collection with invalid ABOUT file ")
-                
+
                 langdefault = cfg.get("general","langdefault")
                 collectionname = cfg.get("general","name")
-                
+
                 collauthors = set()
                 collcategories = {}
-                
+
                 langdefaultpath = os.path.join(collpath,langdefault)
                 setsslist = os.listdir(langdefaultpath)
                 accno = 0
@@ -968,18 +968,27 @@
                         translatedpath = os.path.join(os.path.join(collpath,self.lang),accomset)
                         if os.path.exists(translatedpath):
                             # yes, so use the translated file
-                            accomcfg.read(translatedpath)
+                            readpath = translatedpath
                             langused = self.lang
                         else:
                             # no. maybe there is a shorter language code?
                             translatedpath = os.path.join(os.path.join(collpath,self.lang.split("_")[0]),accomset)
                             if os.path.exists(translatedpath):
-                                accomcfg.read(translatedpath)
+                                readpath = translatedpath
                                 langused = self.lang.split("_")[0]
                             else:
                                 # no. fallback to default one
-                                accomcfg.read(accompath)
+                                readpath = accompath
                                 langused = langdefault
+
+                        # do the parse
+                        try:
+                            accomcfg.read(readpath)
+                        except ConfigParser.ParsingError, e:
+                            log.msg("Parse error for %s.  Skipping."\
+                                "Parse error is: %s" % (readpath, e.message))
+                            continue
+
                         accomdata = dict(accomcfg._sections["accomplishment"])
                         accomID = collection + "/" + accomset[:-15]
                         if 'author' in accomdata:
@@ -1026,18 +1035,27 @@
                             translatedpath = os.path.join(os.path.join(collpath,self.lang),os.path.join(accomset,accomfile))
                             if os.path.exists(translatedpath):
                                 # yes, so use the translated file
-                                accomcfg.read(translatedpath)
+                                readpath = translatedpath
                                 langused = self.lang
                             else:
                                 # no. maybe there is a shorter language code?
                                 translatedpath = os.path.join(os.path.join(collpath,self.lang.split("_")[0]),os.path.join(accomset,accomfile))
                                 if os.path.exists(translatedpath):
-                                    accomcfg.read(translatedpath)
+                                    readpath = translatedpath
                                     langused = self.lang.split("_")[0]
                                 else:
                                     # no. fallback to default one
-                                    accomcfg.read(accompath)
+                                    readpath = accompath
                                     langused = langdefault
+
+                            # do the parse
+                            try:
+                                accomcfg.read(readpath)
+                            except ConfigParser.ParsingError, e:
+                                log.msg("Parse error for %s.  Skipping."\
+                                    "Parse error is: %s" % (readpath,e.message))
+                                continue
+
                             accomdata = dict(accomcfg._sections["accomplishment"])
                             accomID = collection + "/" + accomfile[:-15]
                             if 'author' in accomdata:
@@ -1069,7 +1087,7 @@
                                 accomdata['categories'] = []
                             self.accDB[accomID] = accomdata
                             accno = accno + 1
-                            
+
                 # Look for extrainformation dir
                 extrainfodir = os.path.join(collpath,"extrainformation")
                 extrainfolist = os.listdir(extrainfodir)
@@ -1092,7 +1110,7 @@
                         description = eicfg.get("description",self.lang.split("_")[0])
                     else:
                         description = eicfg.get("description",langdefault)
-                        
+
                     if eicfg.has_option("example", self.lang):
                         example = eicfg.get("example", self.lang)
                     elif eicfg.has_option("example", self.lang.split("_")[0]):
@@ -1101,29 +1119,29 @@
                         example = eicfg.get("example", langdefault)
                     else:
                         example = None
-                        
+
                     if eicfg.has_option("regex", "value"):
                         regex = eicfg.get("regex", "value")
                     else:
                         regex = None
-                        
+
                     extrainfo[extrainfofile] = {
                             'label': label,
                             'description': description,
                             'example': example,
                             'regex': regex,
                             }
-                
+
                 # Store data about this colection
                 collectiondata = {'langdefault':langdefault,'name':collectionname, 'acc_num':accno, 'type':"collection", 'base-path': collpath, 'categories' : collcategories, 'extra-information': extrainfo, 'authors':collauthors}
                 self.accDB[collection] = collectiondata
-          
+
         self._update_all_locked_and_completed_statuses()
         # Uncomment following for debugging
         # print self.accDB\
-        
+
     # ======= Access functions =======
-        
+
     def get_acc_data(self,accomID):
         return self.accDB[accomID]
         

=== modified file 'accomplishments/daemon/tests/tests.py'
--- accomplishments/daemon/tests/tests.py	2012-07-19 19:47:52 +0000
+++ accomplishments/daemon/tests/tests.py	2012-07-30 17:26:22 +0000
@@ -137,37 +137,33 @@
         shutil.rmtree(self.td)
 
     def test_run_scripts(self):
+        # due to LP1030208, if the daemon is running (like on a dev box)
+        # it will pop off the test, but in a pbuilder or build system
+        # there will be no daemon, so we can just ensure that this
+        # doesn't crash
         self.util_copy_accomp(self.accomp_dir, "third")
         a = api.Accomplishments(None, None, True)
 
         # pass in None
         self.assertEqual(a.run_scripts(None), None)
-        self.assertEqual(a.scripts_queue, deque(["%s/third" % self.ACCOMP_SET]))
-        self.assertEqual(a.scripts_queue.popleft(),
-            "%s/third" % self.ACCOMP_SET)
 
         # pass in a bad arg
         self.assertEqual(a.run_scripts(122), None)
-        self.assertEqual(a.scripts_queue, deque(["%s/third" % self.ACCOMP_SET]))
-        self.assertEqual(a.scripts_queue.popleft(),
-            "%s/third" % self.ACCOMP_SET)
 
         # pass in a specific item
         self.assertEqual(a.run_scripts(["%s/third" % self.ACCOMP_SET]), None)
-        self.assertEqual(a.scripts_queue, deque(["%s/third" % self.ACCOMP_SET]))
-        self.assertEqual(a.scripts_queue.popleft(),
-            "%s/third" % self.ACCOMP_SET)
 
     def test_run_script(self):
+        # due to LP1030208, if the daemon is running (like on a dev box)
+        # it will pop off the test, but in a pbuilder or build system
+        # there will be no daemon, so we can just ensure that this
+        # doesn't crash
         self.util_copy_accomp(self.accomp_dir, "third")
         a = api.Accomplishments(None, None, True)
         self.assertEqual(a.run_script("%s/wrong" % self.ACCOMP_SET), None)
-        self.assertEqual(a.scripts_queue, deque([]))
         self.assertEqual(a.run_script("wrong"), None)
-        self.assertEqual(a.scripts_queue, deque([]))
 
         self.assertEqual(a.run_script("%s/third" % self.ACCOMP_SET), None)
-        self.assertEqual(a.scripts_queue, deque(["%s/third" % self.ACCOMP_SET]))
 
     def test_get_acc_date_completed(self):
         self.util_remove_all_accomps(self.accomp_dir)
@@ -524,7 +520,6 @@
         # put the file back
         self.util_write_about_file(self.accomp_root)
 
-    @unittest.skip("waiting for LP:1024041 to be fixed")
     def test_bad_accomplishment_list(self):
         # this test ensures that a bad accompishment doesn't crash the
         # daemon or get into the list
@@ -540,25 +535,14 @@
         a.reload_accom_database()
         self.assertEqual(len(a.list_accomplishments()), 1)
 
+        self.util_write_file(self.accomp_dir, "bad.accomplishment",
+            "descriptionbad desc\n")
+        a.reload_accom_database()
+        self.assertEqual(len(a.list_accomplishments()), 1)
+
         # cleanup
         self.util_remove_all_accomps(self.accomp_dir)
 
-    def test_bad_accomplishment_parse(self):
-        self.util_write_file(self.accomp_dir, "bad.accomplishment",
-            "[accomplishment]\n"\
-            "descriptionbad desc\n")
-        self.assertRaises(ConfigParser.ParsingError, api.Accomplishments, None,
-            None, True)
-        os.remove(os.path.join(self.accomp_dir, "bad.accomplishment"))
-
-        self.util_write_file(self.accomp_dir, "bad.accomplishment",
-            "[accomplishment]\n"\
-            "titlewhatever\n"\
-            "description=bad desc\n")
-        self.assertRaises(ConfigParser.ParsingError, api.Accomplishments, None,
-            None, True)
-        os.remove(os.path.join(self.accomp_dir, "bad.accomplishment"))
-
     # also tests get_config_value()
     def test_write_config_file_item(self):
         a = api.Accomplishments(None, None, True)