Merge lp:~mvo/software-center/fix-more-dep8 into lp:software-center
- fix-more-dep8
- Merge into trunk
Proposed by
Michael Vogt
Status: | Merged |
---|---|
Merged at revision: | 3127 |
Proposed branch: | lp:~mvo/software-center/fix-more-dep8 |
Merge into: | lp:software-center |
Diff against target: |
262 lines (+43/-25) 4 files modified
softwarecenter/backend/installbackend_impl/aptd.py (+18/-15) tests/test_aptd.py (+16/-10) tests/test_logging.py (+3/-0) tests/test_utils.py (+6/-0) |
To merge this branch: | bzr merge lp:~mvo/software-center/fix-more-dep8 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Gary Lasker (community) | Approve | ||
Review via email: mp+121138@code.launchpad.net |
Commit message
Description of the change
Some fixes related to the dep8 jenkins failures from:
https:/
This fixes the failures releated to the fact that adt runs the tests
as root so some stuff like "os.access()" will always work
To post a comment you must log in.
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | === modified file 'softwarecenter/backend/installbackend_impl/aptd.py' | |||
2 | --- softwarecenter/backend/installbackend_impl/aptd.py 2012-08-23 12:25:44 +0000 | |||
3 | +++ softwarecenter/backend/installbackend_impl/aptd.py 2012-08-24 09:09:38 +0000 | |||
4 | @@ -87,6 +87,7 @@ | |||
5 | 87 | 87 | ||
6 | 88 | 88 | ||
7 | 89 | class AptdaemonTransaction(BaseTransaction): | 89 | class AptdaemonTransaction(BaseTransaction): |
8 | 90 | |||
9 | 90 | def __init__(self, trans): | 91 | def __init__(self, trans): |
10 | 91 | self._trans = trans | 92 | self._trans = trans |
11 | 92 | 93 | ||
12 | @@ -210,6 +211,8 @@ | |||
13 | 210 | (GObject.TYPE_PYOBJECT, bool,)), | 211 | (GObject.TYPE_PYOBJECT, bool,)), |
14 | 211 | } | 212 | } |
15 | 212 | 213 | ||
16 | 214 | LICENSE_KEY_SERVER = "ubuntu-production" | ||
17 | 215 | |||
18 | 213 | def __init__(self): | 216 | def __init__(self): |
19 | 214 | GObject.GObject.__init__(self) | 217 | GObject.GObject.__init__(self) |
20 | 215 | 218 | ||
21 | @@ -258,7 +261,7 @@ | |||
22 | 258 | TransactionTypes.REPAIR) | 261 | TransactionTypes.REPAIR) |
23 | 259 | yield self._run_transaction(trans, None, None, None) | 262 | yield self._run_transaction(trans, None, None, None) |
24 | 260 | except Exception as error: | 263 | except Exception as error: |
26 | 261 | self._on_trans_error(error) | 264 | self._on_trans_error(error, trans) |
27 | 262 | 265 | ||
28 | 263 | @inline_callbacks | 266 | @inline_callbacks |
29 | 264 | def fix_incomplete_install(self): | 267 | def fix_incomplete_install(self): |
30 | @@ -268,7 +271,7 @@ | |||
31 | 268 | TransactionTypes.REPAIR) | 271 | TransactionTypes.REPAIR) |
32 | 269 | yield self._run_transaction(trans, None, None, None) | 272 | yield self._run_transaction(trans, None, None, None) |
33 | 270 | except Exception as error: | 273 | except Exception as error: |
35 | 271 | self._on_trans_error(error) | 274 | self._on_trans_error(error, trans) |
36 | 272 | 275 | ||
37 | 273 | # FIXME: upgrade add-ons here | 276 | # FIXME: upgrade add-ons here |
38 | 274 | @inline_callbacks | 277 | @inline_callbacks |
39 | @@ -285,7 +288,7 @@ | |||
40 | 285 | yield self._run_transaction(trans, pkgname, appname, iconname, | 288 | yield self._run_transaction(trans, pkgname, appname, iconname, |
41 | 286 | metadata) | 289 | metadata) |
42 | 287 | except Exception as error: | 290 | except Exception as error: |
44 | 288 | self._on_trans_error(error, pkgname) | 291 | self._on_trans_error(error, trans, pkgname) |
45 | 289 | 292 | ||
46 | 290 | # broken | 293 | # broken |
47 | 291 | # @inline_callbacks | 294 | # @inline_callbacks |
48 | @@ -327,7 +330,7 @@ | |||
49 | 327 | yield self._run_transaction(trans, pkgname, appname, iconname, | 330 | yield self._run_transaction(trans, pkgname, appname, iconname, |
50 | 328 | metadata) | 331 | metadata) |
51 | 329 | except Exception as error: | 332 | except Exception as error: |
53 | 330 | self._on_trans_error(error, pkgname) | 333 | self._on_trans_error(error, trans, pkgname) |
54 | 331 | 334 | ||
55 | 332 | @inline_callbacks | 335 | @inline_callbacks |
56 | 333 | def remove_multiple(self, apps, iconnames, addons_install=[], | 336 | def remove_multiple(self, apps, iconnames, addons_install=[], |
57 | @@ -371,7 +374,7 @@ | |||
58 | 371 | yield self._run_transaction( | 374 | yield self._run_transaction( |
59 | 372 | trans, pkgname, appname, iconname, metadata) | 375 | trans, pkgname, appname, iconname, metadata) |
60 | 373 | except Exception as error: | 376 | except Exception as error: |
62 | 374 | self._on_trans_error(error, pkgname) | 377 | self._on_trans_error(error, trans, pkgname) |
63 | 375 | 378 | ||
64 | 376 | @inline_callbacks | 379 | @inline_callbacks |
65 | 377 | def install_multiple(self, apps, iconnames, addons_install=[], | 380 | def install_multiple(self, apps, iconnames, addons_install=[], |
66 | @@ -401,7 +404,7 @@ | |||
67 | 401 | TransactionTypes.APPLY) | 404 | TransactionTypes.APPLY) |
68 | 402 | yield self._run_transaction(trans, pkgname, appname, iconname) | 405 | yield self._run_transaction(trans, pkgname, appname, iconname) |
69 | 403 | except Exception as error: | 406 | except Exception as error: |
71 | 404 | self._on_trans_error(error) | 407 | self._on_trans_error(error, trans) |
72 | 405 | 408 | ||
73 | 406 | @inline_callbacks | 409 | @inline_callbacks |
74 | 407 | def reload(self, sources_list=None, metadata=None): | 410 | def reload(self, sources_list=None, metadata=None): |
75 | @@ -420,7 +423,7 @@ | |||
76 | 420 | sources_list=sources_list, defer=True) | 423 | sources_list=sources_list, defer=True) |
77 | 421 | yield self._run_transaction(trans, None, None, None, metadata) | 424 | yield self._run_transaction(trans, None, None, None, metadata) |
78 | 422 | except Exception as error: | 425 | except Exception as error: |
80 | 423 | self._on_trans_error(error) | 426 | self._on_trans_error(error, trans) |
81 | 424 | # note that the cache re-open will happen via the connected | 427 | # note that the cache re-open will happen via the connected |
82 | 425 | # "transaction-finished" signal | 428 | # "transaction-finished" signal |
83 | 426 | 429 | ||
84 | @@ -433,7 +436,7 @@ | |||
85 | 433 | # signals | 436 | # signals |
86 | 434 | yield trans.run(defer=True) | 437 | yield trans.run(defer=True) |
87 | 435 | except Exception as error: | 438 | except Exception as error: |
89 | 436 | self._on_trans_error(error, component) | 439 | self._on_trans_error(error, trans, component) |
90 | 437 | return_value(None) | 440 | return_value(None) |
91 | 438 | # now update the cache | 441 | # now update the cache |
92 | 439 | yield self.reload() | 442 | yield self.reload() |
93 | @@ -472,7 +475,7 @@ | |||
94 | 472 | keyid, keyserver, defer=True) | 475 | keyid, keyserver, defer=True) |
95 | 473 | yield self._run_transaction(trans, None, None, None, metadata) | 476 | yield self._run_transaction(trans, None, None, None, metadata) |
96 | 474 | except Exception as error: | 477 | except Exception as error: |
98 | 475 | self._on_trans_error(error) | 478 | self._on_trans_error(error, trans) |
99 | 476 | 479 | ||
100 | 477 | @inline_callbacks | 480 | @inline_callbacks |
101 | 478 | def add_sources_list_entry(self, source_entry, sourcepart=None): | 481 | def add_sources_list_entry(self, source_entry, sourcepart=None): |
102 | @@ -533,9 +536,8 @@ | |||
103 | 533 | # system-wide keys | 536 | # system-wide keys |
104 | 534 | try: | 537 | try: |
105 | 535 | self._logger.info("adding license key for '%s'" % pkgname) | 538 | self._logger.info("adding license key for '%s'" % pkgname) |
106 | 536 | server = "ubuntu-production" | ||
107 | 537 | trans = yield self.aptd_client.add_license_key( | 539 | trans = yield self.aptd_client.add_license_key( |
109 | 538 | pkgname, license_key_oauth, server) | 540 | pkgname, license_key_oauth, self.LICENSE_KEY_SERVER) |
110 | 539 | yield self._run_transaction(trans, None, None, None) | 541 | yield self._run_transaction(trans, None, None, None) |
111 | 540 | except Exception as e: | 542 | except Exception as e: |
112 | 541 | self._logger.error("add_license_key: '%s'" % e) | 543 | self._logger.error("add_license_key: '%s'" % e) |
113 | @@ -697,7 +699,7 @@ | |||
114 | 697 | yield self._run_transaction(trans, app.pkgname, app.appname, | 699 | yield self._run_transaction(trans, app.pkgname, app.appname, |
115 | 698 | "", metadata) | 700 | "", metadata) |
116 | 699 | except Exception as error: | 701 | except Exception as error: |
118 | 700 | self._on_trans_error(error, app.pkgname) | 702 | self._on_trans_error(error, trans, app.pkgname) |
119 | 701 | # add license_key | 703 | # add license_key |
120 | 702 | # FIXME: aptd fails if there is a license_key_path already | 704 | # FIXME: aptd fails if there is a license_key_path already |
121 | 703 | # but I wonder if we should ease that restriction | 705 | # but I wonder if we should ease that restriction |
122 | @@ -959,7 +961,7 @@ | |||
123 | 959 | yield trans.set_meta_data(defer=True, **metadata) | 961 | yield trans.set_meta_data(defer=True, **metadata) |
124 | 960 | yield trans.run(defer=True) | 962 | yield trans.run(defer=True) |
125 | 961 | except Exception as error: | 963 | except Exception as error: |
127 | 962 | self._on_trans_error(error, pkgname) | 964 | self._on_trans_error(error, trans, pkgname) |
128 | 963 | # on error we need to clean the pending purchases | 965 | # on error we need to clean the pending purchases |
129 | 964 | self._clean_pending_purchases(pkgname) | 966 | self._clean_pending_purchases(pkgname) |
130 | 965 | # on success the pending purchase is cleaned when the package | 967 | # on success the pending purchase is cleaned when the package |
131 | @@ -971,8 +973,9 @@ | |||
132 | 971 | if pkgname and pkgname in self.pending_purchases: | 973 | if pkgname and pkgname in self.pending_purchases: |
133 | 972 | del self.pending_purchases[pkgname] | 974 | del self.pending_purchases[pkgname] |
134 | 973 | 975 | ||
137 | 974 | def _on_trans_error(self, error, pkgname=None): | 976 | def _on_trans_error(self, error, trans, pkgname=None): |
138 | 975 | self._logger.warn("_on_trans_error: %s", error) | 977 | self._logger.warn("_on_trans_error: '%s' '%s' '%s'" % ( |
139 | 978 | error, trans, pkgname)) | ||
140 | 976 | # re-enable the action button again if anything went wrong | 979 | # re-enable the action button again if anything went wrong |
141 | 977 | result = TransactionFinishedResult(None, False) | 980 | result = TransactionFinishedResult(None, False) |
142 | 978 | result.pkgname = pkgname | 981 | result.pkgname = pkgname |
143 | 979 | 982 | ||
144 | === modified file 'tests/test_aptd.py' | |||
145 | --- tests/test_aptd.py 2012-08-23 12:18:43 +0000 | |||
146 | +++ tests/test_aptd.py 2012-08-24 09:09:38 +0000 | |||
147 | @@ -9,6 +9,8 @@ | |||
148 | 9 | 9 | ||
149 | 10 | import dbus | 10 | import dbus |
150 | 11 | 11 | ||
151 | 12 | from gi.repository import GObject | ||
152 | 13 | |||
153 | 12 | from tests.utils import ( | 14 | from tests.utils import ( |
154 | 13 | setup_test_env, | 15 | setup_test_env, |
155 | 14 | ) | 16 | ) |
156 | @@ -42,6 +44,7 @@ | |||
157 | 42 | data = "some-data" | 44 | data = "some-data" |
158 | 43 | # test HOME | 45 | # test HOME |
159 | 44 | target = "~/.fasfasdfsdafdfsdafdsfa" | 46 | target = "~/.fasfasdfsdafdfsdafdsfa" |
160 | 47 | self.addCleanup(lambda: os.remove(os.path.expanduser(target))) | ||
161 | 45 | pkgname = "2vcard" | 48 | pkgname = "2vcard" |
162 | 46 | json_auth = "" | 49 | json_auth = "" |
163 | 47 | yield self.aptd.add_license_key(data, target, json_auth, pkgname) | 50 | yield self.aptd.add_license_key(data, target, json_auth, pkgname) |
164 | @@ -50,26 +53,29 @@ | |||
165 | 50 | data2 = "other-data" | 53 | data2 = "other-data" |
166 | 51 | yield self.aptd.add_license_key(data2, target, json_auth, pkgname) | 54 | yield self.aptd.add_license_key(data2, target, json_auth, pkgname) |
167 | 52 | self.assertEqual(open(os.path.expanduser(target)).read(), data) | 55 | self.assertEqual(open(os.path.expanduser(target)).read(), data) |
168 | 53 | # cleanup | ||
169 | 54 | os.remove(os.path.expanduser(target)) | ||
170 | 55 | 56 | ||
171 | 57 | @unittest.skipIf(os.getuid() != 0, | ||
172 | 58 | "test_add_license_key_opt test needs to run as root") | ||
173 | 59 | @unittest.skipIf(not "SC_TEST_JSON" in os.environ, | ||
174 | 60 | "Need a SC_TEST_JSON environment with the credentials") | ||
175 | 56 | def test_add_license_key_opt(self): | 61 | def test_add_license_key_opt(self): |
176 | 57 | if os.getuid() != 0: | ||
177 | 58 | logging.info("skipping add_license_key_opt test") | ||
178 | 59 | return | ||
179 | 60 | # test /opt | 62 | # test /opt |
181 | 61 | data = "some-data" | 63 | license_key = "some-data" |
182 | 62 | pkgname = "hellox" | 64 | pkgname = "hellox" |
183 | 63 | path = "/opt/hellox/conf/license-key.txt" | 65 | path = "/opt/hellox/conf/license-key.txt" |
184 | 66 | self.addCleanup(lambda: os.remove(path)) | ||
185 | 64 | json_auth = os.environ.get("SC_TEST_JSON") or "no-json-auth" | 67 | json_auth = os.environ.get("SC_TEST_JSON") or "no-json-auth" |
186 | 65 | def _error(*args): | 68 | def _error(*args): |
187 | 66 | print "errror", args | 69 | print "errror", args |
188 | 67 | self.aptd.ui = Mock() | 70 | self.aptd.ui = Mock() |
189 | 71 | self.aptd.LICENSE_KEY_SERVER = "ubuntu-staging" | ||
190 | 68 | self.aptd.ui.error = _error | 72 | self.aptd.ui.error = _error |
191 | 69 | @inline_callbacks | 73 | @inline_callbacks |
192 | 70 | def run(): | 74 | def run(): |
195 | 71 | yield self.aptd.add_license_key(data, path, json_auth, pkgname) | 75 | yield self.aptd.add_license_key( |
196 | 72 | aptdaemon.loop.mainloop.quit() | 76 | license_key, path, json_auth, pkgname) |
197 | 77 | # ensure signals get delivered before quit() | ||
198 | 78 | GObject.timeout_add(500, lambda: aptdaemon.loop.mainloop.quit()) | ||
199 | 73 | # run the callback | 79 | # run the callback |
200 | 74 | run() | 80 | run() |
201 | 75 | aptdaemon.loop.mainloop.run() | 81 | aptdaemon.loop.mainloop.run() |
202 | @@ -111,7 +117,6 @@ | |||
203 | 111 | addons_remove = ["gimp-plugin-registry"] | 117 | addons_remove = ["gimp-plugin-registry"] |
204 | 112 | yield self.aptd.apply_changes(pkgname, appname ,iconname, addons_install, addons_remove) | 118 | yield self.aptd.apply_changes(pkgname, appname ,iconname, addons_install, addons_remove) |
205 | 113 | 119 | ||
206 | 114 | |||
207 | 115 | def test_trans_error_ui_display(self): | 120 | def test_trans_error_ui_display(self): |
208 | 116 | """ test if the right error ui is displayed for various dbus | 121 | """ test if the right error ui is displayed for various dbus |
209 | 117 | errors | 122 | errors |
210 | @@ -127,7 +132,7 @@ | |||
211 | 127 | error_ui_mock.reset() | 132 | error_ui_mock.reset() |
212 | 128 | dbus_name_mock.return_value = dbus_name | 133 | dbus_name_mock.return_value = dbus_name |
213 | 129 | error.get_dbus_name = dbus_name_mock | 134 | error.get_dbus_name = dbus_name_mock |
215 | 130 | self.aptd._on_trans_error(error) | 135 | self.aptd._on_trans_error(error, Mock()) |
216 | 131 | self.assertEqual(error_ui_mock.called, show_error_ui) | 136 | self.assertEqual(error_ui_mock.called, show_error_ui) |
217 | 132 | 137 | ||
218 | 133 | @inline_callbacks | 138 | @inline_callbacks |
219 | @@ -164,4 +169,5 @@ | |||
220 | 164 | 169 | ||
221 | 165 | 170 | ||
222 | 166 | if __name__ == "__main__": | 171 | if __name__ == "__main__": |
223 | 172 | logging.basicConfig(level=logging.INFO) | ||
224 | 167 | unittest.main() | 173 | unittest.main() |
225 | 168 | 174 | ||
226 | === modified file 'tests/test_logging.py' | |||
227 | --- tests/test_logging.py 2012-05-30 18:39:55 +0000 | |||
228 | +++ tests/test_logging.py 2012-08-24 09:09:38 +0000 | |||
229 | @@ -17,6 +17,9 @@ | |||
230 | 17 | class TestLogging(unittest.TestCase): | 17 | class TestLogging(unittest.TestCase): |
231 | 18 | """Tests for the sc logging facilities.""" | 18 | """Tests for the sc logging facilities.""" |
232 | 19 | 19 | ||
233 | 20 | @unittest.skipIf( | ||
234 | 21 | os.getuid() == 0, | ||
235 | 22 | "fails when run as root as os.access() is always successful") | ||
236 | 20 | def test_no_write_access_for_cache_dir(self): | 23 | def test_no_write_access_for_cache_dir(self): |
237 | 21 | """Test for bug LP: #688682.""" | 24 | """Test for bug LP: #688682.""" |
238 | 22 | cache_dir = './foo' | 25 | cache_dir = './foo' |
239 | 23 | 26 | ||
240 | === modified file 'tests/test_utils.py' | |||
241 | --- tests/test_utils.py 2012-08-21 09:05:53 +0000 | |||
242 | +++ tests/test_utils.py 2012-08-24 09:09:38 +0000 | |||
243 | @@ -189,6 +189,9 @@ | |||
244 | 189 | # ensure it does not crash for empty strings, LP: #1002271 | 189 | # ensure it does not crash for empty strings, LP: #1002271 |
245 | 190 | self.assertEqual(capitalize_first_word(""), "") | 190 | self.assertEqual(capitalize_first_word(""), "") |
246 | 191 | 191 | ||
247 | 192 | @unittest.skipIf( | ||
248 | 193 | os.getuid() == 0, | ||
249 | 194 | "fails when run as root as os.access() is always successful") | ||
250 | 192 | def test_ensure_file_writable_and_delete_if_not(self): | 195 | def test_ensure_file_writable_and_delete_if_not(self): |
251 | 193 | # first test that a non-writable file (0400) is deleted | 196 | # first test that a non-writable file (0400) is deleted |
252 | 194 | test_file_not_writeable = NamedTemporaryFile() | 197 | test_file_not_writeable = NamedTemporaryFile() |
253 | @@ -203,6 +206,9 @@ | |||
254 | 203 | ensure_file_writable_and_delete_if_not(test_file_writeable.name) | 206 | ensure_file_writable_and_delete_if_not(test_file_writeable.name) |
255 | 204 | self.assertTrue(os.path.exists(test_file_writeable.name)) | 207 | self.assertTrue(os.path.exists(test_file_writeable.name)) |
256 | 205 | 208 | ||
257 | 209 | @unittest.skipIf( | ||
258 | 210 | os.getuid() == 0, | ||
259 | 211 | "fails when run as root as os.chmod() is always successful") | ||
260 | 206 | def test_safe_makedirs(self): | 212 | def test_safe_makedirs(self): |
261 | 207 | tmp = mkdtemp() | 213 | tmp = mkdtemp() |
262 | 208 | # create base dir | 214 | # create base dir |
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!