Merge lp:~mfisch/ubuntu-accomplishments-daemon/ubuntu-accomplishments-daemon-parse-fix into lp:ubuntu-accomplishments-daemon
- ubuntu-accomplishments-daemon-parse-fix
- Merge into accomplishments-daemon
Proposed by
Matt Fischer
Status: | Merged |
---|---|
Merged at revision: | 117 |
Proposed branch: | lp:~mfisch/ubuntu-accomplishments-daemon/ubuntu-accomplishments-daemon-parse-fix |
Merge into: | lp:ubuntu-accomplishments-daemon |
Diff against target: |
239 lines (+51/-49) 2 files modified
accomplishments/daemon/api.py (+38/-20) accomplishments/daemon/tests/tests.py (+13/-29) |
To merge this branch: | bzr merge lp:~mfisch/ubuntu-accomplishments-daemon/ubuntu-accomplishments-daemon-parse-fix |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Ubuntu Accomplishments Daemon Developers | Pending | ||
Review via email: mp+117307@code.launchpad.net |
Commit message
Description of the change
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.
To post a comment you must log in.
- 118. By Matt Fischer
-
minor fix to the error message
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | === modified file 'accomplishments/daemon/api.py' | |||
2 | --- accomplishments/daemon/api.py 2012-07-23 16:27:15 +0000 | |||
3 | +++ accomplishments/daemon/api.py 2012-07-30 17:26:22 +0000 | |||
4 | @@ -938,24 +938,24 @@ | |||
5 | 938 | if collection in self.accDB: | 938 | if collection in self.accDB: |
6 | 939 | # This collection has already been loaded from another install path! | 939 | # This collection has already been loaded from another install path! |
7 | 940 | continue | 940 | continue |
9 | 941 | 941 | ||
10 | 942 | collpath = os.path.join(path,collection) | 942 | collpath = os.path.join(path,collection) |
11 | 943 | aboutpath = os.path.join(collpath,'ABOUT') | 943 | aboutpath = os.path.join(collpath,'ABOUT') |
13 | 944 | 944 | ||
14 | 945 | # Load data from ABOUT file | 945 | # Load data from ABOUT file |
15 | 946 | cfg = ConfigParser.RawConfigParser() | 946 | cfg = ConfigParser.RawConfigParser() |
16 | 947 | cfg.read(aboutpath) | 947 | cfg.read(aboutpath) |
18 | 948 | 948 | ||
19 | 949 | if not (cfg.has_option("general","langdefault") and cfg.has_option("general","name")): | 949 | if not (cfg.has_option("general","langdefault") and cfg.has_option("general","name")): |
20 | 950 | print aboutpath | 950 | print aboutpath |
21 | 951 | raise LookupError("Accomplishment collection with invalid ABOUT file ") | 951 | raise LookupError("Accomplishment collection with invalid ABOUT file ") |
23 | 952 | 952 | ||
24 | 953 | langdefault = cfg.get("general","langdefault") | 953 | langdefault = cfg.get("general","langdefault") |
25 | 954 | collectionname = cfg.get("general","name") | 954 | collectionname = cfg.get("general","name") |
27 | 955 | 955 | ||
28 | 956 | collauthors = set() | 956 | collauthors = set() |
29 | 957 | collcategories = {} | 957 | collcategories = {} |
31 | 958 | 958 | ||
32 | 959 | langdefaultpath = os.path.join(collpath,langdefault) | 959 | langdefaultpath = os.path.join(collpath,langdefault) |
33 | 960 | setsslist = os.listdir(langdefaultpath) | 960 | setsslist = os.listdir(langdefaultpath) |
34 | 961 | accno = 0 | 961 | accno = 0 |
35 | @@ -968,18 +968,27 @@ | |||
36 | 968 | translatedpath = os.path.join(os.path.join(collpath,self.lang),accomset) | 968 | translatedpath = os.path.join(os.path.join(collpath,self.lang),accomset) |
37 | 969 | if os.path.exists(translatedpath): | 969 | if os.path.exists(translatedpath): |
38 | 970 | # yes, so use the translated file | 970 | # yes, so use the translated file |
40 | 971 | accomcfg.read(translatedpath) | 971 | readpath = translatedpath |
41 | 972 | langused = self.lang | 972 | langused = self.lang |
42 | 973 | else: | 973 | else: |
43 | 974 | # no. maybe there is a shorter language code? | 974 | # no. maybe there is a shorter language code? |
44 | 975 | translatedpath = os.path.join(os.path.join(collpath,self.lang.split("_")[0]),accomset) | 975 | translatedpath = os.path.join(os.path.join(collpath,self.lang.split("_")[0]),accomset) |
45 | 976 | if os.path.exists(translatedpath): | 976 | if os.path.exists(translatedpath): |
47 | 977 | accomcfg.read(translatedpath) | 977 | readpath = translatedpath |
48 | 978 | langused = self.lang.split("_")[0] | 978 | langused = self.lang.split("_")[0] |
49 | 979 | else: | 979 | else: |
50 | 980 | # no. fallback to default one | 980 | # no. fallback to default one |
52 | 981 | accomcfg.read(accompath) | 981 | readpath = accompath |
53 | 982 | langused = langdefault | 982 | langused = langdefault |
54 | 983 | |||
55 | 984 | # do the parse | ||
56 | 985 | try: | ||
57 | 986 | accomcfg.read(readpath) | ||
58 | 987 | except ConfigParser.ParsingError, e: | ||
59 | 988 | log.msg("Parse error for %s. Skipping."\ | ||
60 | 989 | "Parse error is: %s" % (readpath, e.message)) | ||
61 | 990 | continue | ||
62 | 991 | |||
63 | 983 | accomdata = dict(accomcfg._sections["accomplishment"]) | 992 | accomdata = dict(accomcfg._sections["accomplishment"]) |
64 | 984 | accomID = collection + "/" + accomset[:-15] | 993 | accomID = collection + "/" + accomset[:-15] |
65 | 985 | if 'author' in accomdata: | 994 | if 'author' in accomdata: |
66 | @@ -1026,18 +1035,27 @@ | |||
67 | 1026 | translatedpath = os.path.join(os.path.join(collpath,self.lang),os.path.join(accomset,accomfile)) | 1035 | translatedpath = os.path.join(os.path.join(collpath,self.lang),os.path.join(accomset,accomfile)) |
68 | 1027 | if os.path.exists(translatedpath): | 1036 | if os.path.exists(translatedpath): |
69 | 1028 | # yes, so use the translated file | 1037 | # yes, so use the translated file |
71 | 1029 | accomcfg.read(translatedpath) | 1038 | readpath = translatedpath |
72 | 1030 | langused = self.lang | 1039 | langused = self.lang |
73 | 1031 | else: | 1040 | else: |
74 | 1032 | # no. maybe there is a shorter language code? | 1041 | # no. maybe there is a shorter language code? |
75 | 1033 | translatedpath = os.path.join(os.path.join(collpath,self.lang.split("_")[0]),os.path.join(accomset,accomfile)) | 1042 | translatedpath = os.path.join(os.path.join(collpath,self.lang.split("_")[0]),os.path.join(accomset,accomfile)) |
76 | 1034 | if os.path.exists(translatedpath): | 1043 | if os.path.exists(translatedpath): |
78 | 1035 | accomcfg.read(translatedpath) | 1044 | readpath = translatedpath |
79 | 1036 | langused = self.lang.split("_")[0] | 1045 | langused = self.lang.split("_")[0] |
80 | 1037 | else: | 1046 | else: |
81 | 1038 | # no. fallback to default one | 1047 | # no. fallback to default one |
83 | 1039 | accomcfg.read(accompath) | 1048 | readpath = accompath |
84 | 1040 | langused = langdefault | 1049 | langused = langdefault |
85 | 1050 | |||
86 | 1051 | # do the parse | ||
87 | 1052 | try: | ||
88 | 1053 | accomcfg.read(readpath) | ||
89 | 1054 | except ConfigParser.ParsingError, e: | ||
90 | 1055 | log.msg("Parse error for %s. Skipping."\ | ||
91 | 1056 | "Parse error is: %s" % (readpath,e.message)) | ||
92 | 1057 | continue | ||
93 | 1058 | |||
94 | 1041 | accomdata = dict(accomcfg._sections["accomplishment"]) | 1059 | accomdata = dict(accomcfg._sections["accomplishment"]) |
95 | 1042 | accomID = collection + "/" + accomfile[:-15] | 1060 | accomID = collection + "/" + accomfile[:-15] |
96 | 1043 | if 'author' in accomdata: | 1061 | if 'author' in accomdata: |
97 | @@ -1069,7 +1087,7 @@ | |||
98 | 1069 | accomdata['categories'] = [] | 1087 | accomdata['categories'] = [] |
99 | 1070 | self.accDB[accomID] = accomdata | 1088 | self.accDB[accomID] = accomdata |
100 | 1071 | accno = accno + 1 | 1089 | accno = accno + 1 |
102 | 1072 | 1090 | ||
103 | 1073 | # Look for extrainformation dir | 1091 | # Look for extrainformation dir |
104 | 1074 | extrainfodir = os.path.join(collpath,"extrainformation") | 1092 | extrainfodir = os.path.join(collpath,"extrainformation") |
105 | 1075 | extrainfolist = os.listdir(extrainfodir) | 1093 | extrainfolist = os.listdir(extrainfodir) |
106 | @@ -1092,7 +1110,7 @@ | |||
107 | 1092 | description = eicfg.get("description",self.lang.split("_")[0]) | 1110 | description = eicfg.get("description",self.lang.split("_")[0]) |
108 | 1093 | else: | 1111 | else: |
109 | 1094 | description = eicfg.get("description",langdefault) | 1112 | description = eicfg.get("description",langdefault) |
111 | 1095 | 1113 | ||
112 | 1096 | if eicfg.has_option("example", self.lang): | 1114 | if eicfg.has_option("example", self.lang): |
113 | 1097 | example = eicfg.get("example", self.lang) | 1115 | example = eicfg.get("example", self.lang) |
114 | 1098 | elif eicfg.has_option("example", self.lang.split("_")[0]): | 1116 | elif eicfg.has_option("example", self.lang.split("_")[0]): |
115 | @@ -1101,29 +1119,29 @@ | |||
116 | 1101 | example = eicfg.get("example", langdefault) | 1119 | example = eicfg.get("example", langdefault) |
117 | 1102 | else: | 1120 | else: |
118 | 1103 | example = None | 1121 | example = None |
120 | 1104 | 1122 | ||
121 | 1105 | if eicfg.has_option("regex", "value"): | 1123 | if eicfg.has_option("regex", "value"): |
122 | 1106 | regex = eicfg.get("regex", "value") | 1124 | regex = eicfg.get("regex", "value") |
123 | 1107 | else: | 1125 | else: |
124 | 1108 | regex = None | 1126 | regex = None |
126 | 1109 | 1127 | ||
127 | 1110 | extrainfo[extrainfofile] = { | 1128 | extrainfo[extrainfofile] = { |
128 | 1111 | 'label': label, | 1129 | 'label': label, |
129 | 1112 | 'description': description, | 1130 | 'description': description, |
130 | 1113 | 'example': example, | 1131 | 'example': example, |
131 | 1114 | 'regex': regex, | 1132 | 'regex': regex, |
132 | 1115 | } | 1133 | } |
134 | 1116 | 1134 | ||
135 | 1117 | # Store data about this colection | 1135 | # Store data about this colection |
136 | 1118 | collectiondata = {'langdefault':langdefault,'name':collectionname, 'acc_num':accno, 'type':"collection", 'base-path': collpath, 'categories' : collcategories, 'extra-information': extrainfo, 'authors':collauthors} | 1136 | collectiondata = {'langdefault':langdefault,'name':collectionname, 'acc_num':accno, 'type':"collection", 'base-path': collpath, 'categories' : collcategories, 'extra-information': extrainfo, 'authors':collauthors} |
137 | 1119 | self.accDB[collection] = collectiondata | 1137 | self.accDB[collection] = collectiondata |
139 | 1120 | 1138 | ||
140 | 1121 | self._update_all_locked_and_completed_statuses() | 1139 | self._update_all_locked_and_completed_statuses() |
141 | 1122 | # Uncomment following for debugging | 1140 | # Uncomment following for debugging |
142 | 1123 | # print self.accDB\ | 1141 | # print self.accDB\ |
144 | 1124 | 1142 | ||
145 | 1125 | # ======= Access functions ======= | 1143 | # ======= Access functions ======= |
147 | 1126 | 1144 | ||
148 | 1127 | def get_acc_data(self,accomID): | 1145 | def get_acc_data(self,accomID): |
149 | 1128 | return self.accDB[accomID] | 1146 | return self.accDB[accomID] |
150 | 1129 | 1147 | ||
151 | 1130 | 1148 | ||
152 | === modified file 'accomplishments/daemon/tests/tests.py' | |||
153 | --- accomplishments/daemon/tests/tests.py 2012-07-19 19:47:52 +0000 | |||
154 | +++ accomplishments/daemon/tests/tests.py 2012-07-30 17:26:22 +0000 | |||
155 | @@ -137,37 +137,33 @@ | |||
156 | 137 | shutil.rmtree(self.td) | 137 | shutil.rmtree(self.td) |
157 | 138 | 138 | ||
158 | 139 | def test_run_scripts(self): | 139 | def test_run_scripts(self): |
159 | 140 | # due to LP1030208, if the daemon is running (like on a dev box) | ||
160 | 141 | # it will pop off the test, but in a pbuilder or build system | ||
161 | 142 | # there will be no daemon, so we can just ensure that this | ||
162 | 143 | # doesn't crash | ||
163 | 140 | self.util_copy_accomp(self.accomp_dir, "third") | 144 | self.util_copy_accomp(self.accomp_dir, "third") |
164 | 141 | a = api.Accomplishments(None, None, True) | 145 | a = api.Accomplishments(None, None, True) |
165 | 142 | 146 | ||
166 | 143 | # pass in None | 147 | # pass in None |
167 | 144 | self.assertEqual(a.run_scripts(None), None) | 148 | self.assertEqual(a.run_scripts(None), None) |
168 | 145 | self.assertEqual(a.scripts_queue, deque(["%s/third" % self.ACCOMP_SET])) | ||
169 | 146 | self.assertEqual(a.scripts_queue.popleft(), | ||
170 | 147 | "%s/third" % self.ACCOMP_SET) | ||
171 | 148 | 149 | ||
172 | 149 | # pass in a bad arg | 150 | # pass in a bad arg |
173 | 150 | self.assertEqual(a.run_scripts(122), None) | 151 | self.assertEqual(a.run_scripts(122), None) |
174 | 151 | self.assertEqual(a.scripts_queue, deque(["%s/third" % self.ACCOMP_SET])) | ||
175 | 152 | self.assertEqual(a.scripts_queue.popleft(), | ||
176 | 153 | "%s/third" % self.ACCOMP_SET) | ||
177 | 154 | 152 | ||
178 | 155 | # pass in a specific item | 153 | # pass in a specific item |
179 | 156 | self.assertEqual(a.run_scripts(["%s/third" % self.ACCOMP_SET]), None) | 154 | self.assertEqual(a.run_scripts(["%s/third" % self.ACCOMP_SET]), None) |
180 | 157 | self.assertEqual(a.scripts_queue, deque(["%s/third" % self.ACCOMP_SET])) | ||
181 | 158 | self.assertEqual(a.scripts_queue.popleft(), | ||
182 | 159 | "%s/third" % self.ACCOMP_SET) | ||
183 | 160 | 155 | ||
184 | 161 | def test_run_script(self): | 156 | def test_run_script(self): |
185 | 157 | # due to LP1030208, if the daemon is running (like on a dev box) | ||
186 | 158 | # it will pop off the test, but in a pbuilder or build system | ||
187 | 159 | # there will be no daemon, so we can just ensure that this | ||
188 | 160 | # doesn't crash | ||
189 | 162 | self.util_copy_accomp(self.accomp_dir, "third") | 161 | self.util_copy_accomp(self.accomp_dir, "third") |
190 | 163 | a = api.Accomplishments(None, None, True) | 162 | a = api.Accomplishments(None, None, True) |
191 | 164 | self.assertEqual(a.run_script("%s/wrong" % self.ACCOMP_SET), None) | 163 | self.assertEqual(a.run_script("%s/wrong" % self.ACCOMP_SET), None) |
192 | 165 | self.assertEqual(a.scripts_queue, deque([])) | ||
193 | 166 | self.assertEqual(a.run_script("wrong"), None) | 164 | self.assertEqual(a.run_script("wrong"), None) |
194 | 167 | self.assertEqual(a.scripts_queue, deque([])) | ||
195 | 168 | 165 | ||
196 | 169 | self.assertEqual(a.run_script("%s/third" % self.ACCOMP_SET), None) | 166 | self.assertEqual(a.run_script("%s/third" % self.ACCOMP_SET), None) |
197 | 170 | self.assertEqual(a.scripts_queue, deque(["%s/third" % self.ACCOMP_SET])) | ||
198 | 171 | 167 | ||
199 | 172 | def test_get_acc_date_completed(self): | 168 | def test_get_acc_date_completed(self): |
200 | 173 | self.util_remove_all_accomps(self.accomp_dir) | 169 | self.util_remove_all_accomps(self.accomp_dir) |
201 | @@ -524,7 +520,6 @@ | |||
202 | 524 | # put the file back | 520 | # put the file back |
203 | 525 | self.util_write_about_file(self.accomp_root) | 521 | self.util_write_about_file(self.accomp_root) |
204 | 526 | 522 | ||
205 | 527 | @unittest.skip("waiting for LP:1024041 to be fixed") | ||
206 | 528 | def test_bad_accomplishment_list(self): | 523 | def test_bad_accomplishment_list(self): |
207 | 529 | # this test ensures that a bad accompishment doesn't crash the | 524 | # this test ensures that a bad accompishment doesn't crash the |
208 | 530 | # daemon or get into the list | 525 | # daemon or get into the list |
209 | @@ -540,25 +535,14 @@ | |||
210 | 540 | a.reload_accom_database() | 535 | a.reload_accom_database() |
211 | 541 | self.assertEqual(len(a.list_accomplishments()), 1) | 536 | self.assertEqual(len(a.list_accomplishments()), 1) |
212 | 542 | 537 | ||
213 | 538 | self.util_write_file(self.accomp_dir, "bad.accomplishment", | ||
214 | 539 | "descriptionbad desc\n") | ||
215 | 540 | a.reload_accom_database() | ||
216 | 541 | self.assertEqual(len(a.list_accomplishments()), 1) | ||
217 | 542 | |||
218 | 543 | # cleanup | 543 | # cleanup |
219 | 544 | self.util_remove_all_accomps(self.accomp_dir) | 544 | self.util_remove_all_accomps(self.accomp_dir) |
220 | 545 | 545 | ||
221 | 546 | def test_bad_accomplishment_parse(self): | ||
222 | 547 | self.util_write_file(self.accomp_dir, "bad.accomplishment", | ||
223 | 548 | "[accomplishment]\n"\ | ||
224 | 549 | "descriptionbad desc\n") | ||
225 | 550 | self.assertRaises(ConfigParser.ParsingError, api.Accomplishments, None, | ||
226 | 551 | None, True) | ||
227 | 552 | os.remove(os.path.join(self.accomp_dir, "bad.accomplishment")) | ||
228 | 553 | |||
229 | 554 | self.util_write_file(self.accomp_dir, "bad.accomplishment", | ||
230 | 555 | "[accomplishment]\n"\ | ||
231 | 556 | "titlewhatever\n"\ | ||
232 | 557 | "description=bad desc\n") | ||
233 | 558 | self.assertRaises(ConfigParser.ParsingError, api.Accomplishments, None, | ||
234 | 559 | None, True) | ||
235 | 560 | os.remove(os.path.join(self.accomp_dir, "bad.accomplishment")) | ||
236 | 561 | |||
237 | 562 | # also tests get_config_value() | 546 | # also tests get_config_value() |
238 | 563 | def test_write_config_file_item(self): | 547 | def test_write_config_file_item(self): |
239 | 564 | a = api.Accomplishments(None, None, True) | 548 | a = api.Accomplishments(None, None, True) |