Merge lp:~nskaggs/autopilot/catch-stateerror-printtree into lp:autopilot
- catch-stateerror-printtree
- Merge into trunk
Status: | Superseded | ||||
---|---|---|---|---|---|
Proposed branch: | lp:~nskaggs/autopilot/catch-stateerror-printtree | ||||
Merge into: | lp:autopilot | ||||
Prerequisite: | lp:~thomir-deactivatedaccount/autopilot/trunk-make-print-tree-faster | ||||
Diff against target: |
68 lines (+28/-6) 2 files modified
autopilot/introspection/dbus.py (+10/-6) autopilot/tests/unit/test_introspection_features.py (+18/-0) |
||||
To merge this branch: | bzr merge lp:~nskaggs/autopilot/catch-stateerror-printtree | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Nicholas Skaggs (community) | Approve | ||
PS Jenkins bot | continuous-integration | Needs Fixing | |
Chris Gagnon | Pending | ||
Thomi Richards | Pending | ||
Max Brustkern | Pending | ||
Review via email:
|
This proposal supersedes a proposal from 2014-02-05.
Commit message
Add support to complete dump even if a StateNotFound error occurs.
Description of the change
Dumping the object tree with print_tree often fails to complete due to StateNotFound errors. This modifies the print call to catch these errors and continue to print the largest possible tree. Other errors could be considered as well to try and print as complete a dump as possible.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal | # |
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Chris Gagnon (chris.gagnon) wrote : Posted in a previous version of this proposal | # |
This needs tests
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal | # |
FAILED: Continuous integration, rev:424
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal | # |
FAILED: Continuous integration, rev:425
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal | # |
FAILED: Continuous integration, rev:426
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal | # |
FAILED: Continuous integration, rev:427
http://
Executed test runs:
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
UNSTABLE: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
Click here to trigger a rebuild:
http://
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Nicholas Skaggs (nskaggs) wrote : Posted in a previous version of this proposal | # |
@ChrisGagnon, I'll try playing with AssertRaises to add a test for this path.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Thomi Richards (thomir-deactivatedaccount) wrote : Posted in a previous version of this proposal | # |
Setting this back to a WIP MP since we all agree that it needs tests before being merged.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal | # |
FAILED: Continuous integration, rev:432
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal | # |
FAILED: Continuous integration, rev:432
http://
Executed test runs:
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
UNSTABLE: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
Click here to trigger a rebuild:
http://
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal | # |
FAILED: Continuous integration, rev:432
http://
Executed test runs:
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
UNSTABLE: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
Click here to trigger a rebuild:
http://
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal | # |
FAILED: Continuous integration, rev:432
http://
Executed test runs:
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
UNSTABLE: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
Click here to trigger a rebuild:
http://
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal | # |
FAILED: Continuous integration, rev:433
http://
Executed test runs:
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
UNSTABLE: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
Click here to trigger a rebuild:
http://
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Thomi Richards (thomir-deactivatedaccount) wrote : Posted in a previous version of this proposal | # |
Hi
at the end of your test, could you please add an assertion for the error message you expect to see?
Please set this MP back to 'Needs review' when you want another review.
Thanks
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Nicholas Skaggs (nskaggs) wrote : Posted in a previous version of this proposal | # |
Argh, bzr lost my changes I made.. they never pushed I see. I'll redo.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal | # |
FAILED: Continuous integration, rev:437
http://
Executed test runs:
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
UNSTABLE: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
Click here to trigger a rebuild:
http://
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal | # |
FAILED: Continuous integration, rev:437
http://
Executed test runs:
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
UNSTABLE: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
Click here to trigger a rebuild:
http://
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal | # |
FAILED: Continuous integration, rev:437
http://
Executed test runs:
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
UNSTABLE: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
Click here to trigger a rebuild:
http://
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Max Brustkern (nuclearbob) wrote : Posted in a previous version of this proposal | # |
Seems reasonable to me.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal | # |
FAILED: Continuous integration, rev:438
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal | # |
FAILED: Continuous integration, rev:439
http://
Executed test runs:
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
UNSTABLE: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
Click here to trigger a rebuild:
http://
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Nicholas Skaggs (nskaggs) wrote : Posted in a previous version of this proposal | # |
I have concerns this may have introduced an issue with print_tree, which is not shown by the tests. The dump seems to get stuck in the regression loop. Namely the change from
for p in sorted(
to
for prop, value in sorted(
is the likely cause.
Can someone help setup a test object that contains a larger tree to vet these changes a bit better?
We create a fake object like so:
fake_object = DBusIntrospecti
Mock()
)
I tried making the dict larger but failed. I'm missing something basic; how can i make a tree like this?
== /some/path ==
id: 123
path: '/some/path'
text: 'Hello'
== /some/path/child ==
id: 456
== /some/path/child2 ==
id: 789
== /some/other/path ==
id: 542
path: '/some/other/path'
text: 'Hello'
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Thomi Richards (thomir-deactivatedaccount) wrote : Posted in a previous version of this proposal | # |
Hi,
I doubt that turning this:
for p in sorted(
into this:
for prop, value in sorted(
Is the cause of your problems. You're correct though - we need a better way to create fake proxy objects. This is something I've been thinking about for a looong time.
I suggest that you merge in my branch, which already has many of the changes in here, and add your exception handling and test code.
- 440. By Nicholas Skaggs
-
merge thomi's tweaks
- 441. By Nicholas Skaggs
-
tweak merge
- 442. By Nicholas Skaggs
-
fix spacing :-)
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:441
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https:/
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Nicholas Skaggs (nskaggs) wrote : | # |
Something in the tweaks breaks things:
Checkout this branch
lp:~nskaggs/ubuntu-calendar-app/test-print-tree
Running calendar_
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:442
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https:/
http://
Executed test runs:
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
UNSTABLE: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
Click here to trigger a rebuild:
http://
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Nicholas Skaggs (nskaggs) wrote : | # |
Thomi says I'm dreaming and it works for him, so +1.
Unmerged revisions
Preview Diff
1 | === modified file 'autopilot/introspection/dbus.py' |
2 | --- autopilot/introspection/dbus.py 2014-03-20 21:26:27 +0000 |
3 | +++ autopilot/introspection/dbus.py 2014-03-20 21:26:27 +0000 |
4 | @@ -262,6 +262,7 @@ |
5 | # Thomi: 2014-03-20: There used to be a call to 'self.refresh_state()' |
6 | # here. That's not needed, since the only thing we use is the proxy |
7 | # path, which isn't affected by the current state. |
8 | + |
9 | query = self.get_class_query_string() + "/*" |
10 | state_dicts = self.get_state_by_path(query) |
11 | children = [self.make_introspection_object(i) for i in state_dicts] |
12 | @@ -661,12 +662,15 @@ |
13 | else: |
14 | properties = self.get_properties() |
15 | # print properties |
16 | - for key in sorted(properties.keys()): |
17 | - output.write("%s%s: %r\n" % (indent, key, properties[key])) |
18 | - # print children |
19 | - if maxdepth is None or _curdepth < maxdepth: |
20 | - for c in self.get_children(): |
21 | - c.print_tree(output, maxdepth, _curdepth + 1) |
22 | + try: |
23 | + for key in sorted(properties.keys()): |
24 | + output.write("%s%s: %r\n" % (indent, key, properties[key])) |
25 | + # print children |
26 | + if maxdepth is None or _curdepth < maxdepth: |
27 | + for c in self.get_children(): |
28 | + c.print_tree(output, maxdepth, _curdepth + 1) |
29 | + except StateNotFoundError as error: |
30 | + output.write("%sError: %s\n" % (indent, error)) |
31 | |
32 | @contextmanager |
33 | def no_automatic_refreshing(self): |
34 | |
35 | === modified file 'autopilot/tests/unit/test_introspection_features.py' |
36 | --- autopilot/tests/unit/test_introspection_features.py 2014-03-11 17:36:53 +0000 |
37 | +++ autopilot/tests/unit/test_introspection_features.py 2014-03-20 21:26:27 +0000 |
38 | @@ -55,6 +55,7 @@ |
39 | _object_passes_filters, |
40 | CustomEmulatorBase, |
41 | DBusIntrospectionObject, |
42 | + StateNotFoundError, |
43 | ) |
44 | from autopilot.utilities import sleep |
45 | |
46 | @@ -305,6 +306,23 @@ |
47 | text: 'Hello' |
48 | """)) |
49 | |
50 | + def test_print_tree_exception(self): |
51 | + """print_tree with StateNotFound exception""" |
52 | + |
53 | + fake_object = self._print_test_fake_object() |
54 | + child = Mock() |
55 | + child.print_tree.side_effect = StateNotFoundError('child') |
56 | + |
57 | + with patch.object(fake_object, 'get_children', return_value=[child]): |
58 | + out = StringIO() |
59 | + print_func = lambda: fake_object.print_tree(out) |
60 | + self.assertThat(print_func, Not(Raises(StateNotFoundError))) |
61 | + self.assertEqual(out.getvalue(), dedent("""\ |
62 | + == /some/path == |
63 | + id: 123 |
64 | + Error: Object not found with name 'child'. |
65 | + """)) |
66 | + |
67 | def test_print_tree_fileobj(self): |
68 | """print_tree with file object output""" |
69 |
FAILED: Continuous integration, rev:421 /code.launchpad .net/~nskaggs/ autopilot/ catch-stateerro r-printtree/ +merge/ 204976/ +edit-commit- message
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https:/
http:// jenkins. qa.ubuntu. com/job/ autopilot- ci/462/ jenkins. qa.ubuntu. com/job/ autopilot- trusty- amd64-ci/ 188/console jenkins. qa.ubuntu. com/job/ autopilot- trusty- armhf-ci/ 188/console jenkins. qa.ubuntu. com/job/ generic- mediumtests- trusty/ 2917/console jenkins. qa.ubuntu. com/job/ generic- mediumtests- builder- trusty- amd64/2919/ console
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild: s-jenkins. ubuntu- ci:8080/ job/autopilot- ci/462/ rebuild
http://