Merge lp:~canonical-platform-qa/autopilot/cpo-base-classes-fix into lp:autopilot
- cpo-base-classes-fix
- Merge into trunk
Status: | Merged |
---|---|
Approved by: | Christopher Lee |
Approved revision: | 578 |
Merged at revision: | 554 |
Proposed branch: | lp:~canonical-platform-qa/autopilot/cpo-base-classes-fix |
Merge into: | lp:autopilot |
Diff against target: |
590 lines (+399/-24) 9 files modified
apparmor/click.rules (+0/-12) autopilot/introspection/_object_registry.py (+68/-6) autopilot/introspection/_search.py (+35/-0) autopilot/tests/functional/test_introspection_features.py (+47/-2) autopilot/tests/unit/test_introspection_object_registry.py (+147/-1) autopilot/tests/unit/test_introspection_search.py (+94/-0) debian/autopilot-touch.dirs (+0/-1) debian/autopilot-touch.install (+0/-1) debian/changelog (+8/-1) |
To merge this branch: | bzr merge lp:~canonical-platform-qa/autopilot/cpo-base-classes-fix |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
PS Jenkins bot | continuous-integration | Approve | |
Christopher Lee (community) | Needs Information | ||
Review via email: mp+257623@code.launchpad.net |
Commit message
CPO base classes fix
Description of the change
CPO base classes fix
Christopher Lee (veebers) wrote : | # |
Federico Gimenez (fgimenez) wrote : | # |
Hi Chris, yes, I totally agree that adding the MRO stuff to the combine method is changing its initial behaviour. I added it after trying to make the current tests pass, I've commented in the code trying to make it clear.
Using the combine method to put together arbitrary tuples was giving at first an error about a cyclic dependency, one of the elements of the tuple was the class itself.
TypeError: a __bases__ item causes an inheritance cycle
This can be solved by passing the original class itself to the method, and checking that it is not a member of the collections being merged. At this point, we could continue treating the combine method as an abstract information merger. But after this, a second error arose:
TypeError: Cannot create a consistent method resolution order (MRO)
So that, to combine them appropriately, the method has to know which is the right order, and this information can be obtained from the classes MROs.
Unless I'm missing something it seems that the combine method has to know that it is combining classes, moreover it needs to know that the return value is going to be the bases tuple of one of the parameters. IMO the responsibility is well delimited, maybe we could split it.
Anyway, I'll try to implement the tests that you mention, thanks a lot Chris!
Federico Gimenez (fgimenez) wrote : | # |
I've already added the tests for the current implementation of _combine_
While writing them I've noticed that when two classes had the same mro the order wasn't deterministic, I've added another function for having a more fine-grained control. Currently the classes in the original __bases__ tuple have lower index than the extensions in the resulting tuple, this can be changed.
Thanks,
Christopher Lee (veebers) wrote : | # |
Looking good guys, just a couple of comments inline.
As per the changes I made to day; I was thinking about how to handle detecting and choosing the correct base class when there was multiple inheritance involved (i.e. multiple CPO inheritance).
I came to the conclusion that it is bad form to change things under the cover in this fashion as it can be misleading and cause unexpected changes.
Instead I've opted for issuing a warning when this is detected with the option to upgrade this to raising an error in the new future (i.e. when we can be certain we don't break everyone with this new enforcement rule.)
I was of 2 minds making it raise an exception straight off, but I feel that it was to heavy handed at this stage.
What do you guys think?
I've done some small scale testing locally and the address book and unity8 tests pass as expected. I would love to see this go through the gatekeeper to see the results there.
Federico Gimenez (fgimenez) wrote : | # |
Thanks Chris, I've addressed the issues you mentioned, putting this in review
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:569
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
- 570. By Federico Gimenez
-
Fixed flake8
Vincent Ladeuil (vila) wrote : | # |
> Looking good guys, just a couple of comments inline.
>
> As per the changes I made to day; I was thinking about how to handle detecting
> and choosing the correct base class when there was multiple inheritance
> involved (i.e. multiple CPO inheritance).
>
> I came to the conclusion that it is bad form to change things under the cover
> in this fashion as it can be misleading and cause unexpected changes.
/me nods
> Instead I've opted for issuing a warning when this is detected with the option
> to upgrade this to raising an error in the new future (i.e. when we can be
> certain we don't break everyone with this new enforcement rule.)
I would prefer raising an error than fixing the base class too.
Misuse of the CPO is a bug, clients should be fixed.
Especially when looking at Leo's branches fixing this issue in several clients, the fix seems light enough that it's worth breaking the clients so they are fixed asap.
A warning may be missed and the client test suites will not be fixed until it's noticed.
>
> I was of 2 minds making it raise an exception straight off, but I feel that it
> was to heavy handed at this stage.
> What do you guys think?
Let's collect some test results to get a better understanding ;)
>
> I've done some small scale testing locally and the address book and unity8
> tests pass as expected. I would love to see this go through the gatekeeper to
> see the results there.
+1
Looking at http://
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:570
http://
Executed test runs:
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
FAILURE: http://
FAILURE: http://
SUCCESS: http://
deb: http://
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:569
http://
Executed test runs:
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
FAILURE: http://
FAILURE: http://
SUCCESS: http://
deb: http://
Click here to trigger a rebuild:
http://
- 571. By Federico Gimenez
-
More on comment about the classes ordering
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:571
http://
Executed test runs:
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
FAILURE: http://
FAILURE: http://
SUCCESS: http://
deb: http://
Click here to trigger a rebuild:
http://
- 572. By Federico Gimenez
-
merged vila's branch with packaging fix
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:572
http://
Executed test runs:
FAILURE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
FAILURE: http://
FAILURE: http://
SUCCESS: http://
deb: http://
Click here to trigger a rebuild:
http://
- 573. By Federico Gimenez
-
Removing validate_
dbus_object method in the extesnsions before adding to the bases to preserve the original method resolution - 574. By Federico Gimenez
-
Fixed changelog
- 575. By Federico Gimenez
-
changelog signature
- 576. By Federico Gimenez
-
Fixed unit test
Federico Gimenez (fgimenez) wrote : | # |
I've uploaded some fixes for errors spotted in the gatekeeper jobs. These were related to the breadth-first nature of method resolution: given one class that doesn't have a validate_
The solution adopted is removing the validate_
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:576
http://
Executed test runs:
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
SUCCESS: http://
deb: http://
Click here to trigger a rebuild:
http://
Christopher Lee (veebers) wrote : | # |
I'm really not a fan of modifying the classes in such a way, perhaps it's indicating that we're going down the wrong path and we need to find a better solution.
That being said the 2 tests that failed due to _try_custom_
I'm just trying to get my local devices etc. setup so I can test this, but I think that if we instead change the warning to an error (yes, I was wrong there it should have always been an error :-) ). and use the updated addresss book (i.e. the one that is passing the correct base_emulator in aka Leos branch) we shouldn't need to modify the class attributes like we are here.
I'll try to post the results when I get them, but at this rate it's going to take ages (my device is acting up.)
[1] http://
Christopher Lee (veebers) wrote : | # |
Follow up:
I ran the fixed (i.e. Leos base_emulator update branch) and non fixed address book tests, the fixed worked well, the non-fixed exhibited the error we see.
We should remove the additional code that removes the method attribute and change the warning into an error (probably RuntimeError? or ValueError?).
- 577. By Federico Gimenez
-
The validate_
dbus_object method is not removed from extensions classes before adding them to cpos; raised exception instead of logging warning when the emulator_base does not match the base extension - 578. By Federico Gimenez
-
Updated error message
Federico Gimenez (fgimenez) wrote : | # |
>
> We should remove the additional code that removes the method attribute and
> change the warning into an error (probably RuntimeError? or ValueError?).
Done, regarding the exception after talking with vila we have gone for ValueError, it can be changed of course. I've also updated the unit tests accordingly.
Cheers!
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:578
http://
Executed test runs:
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
FAILURE: http://
FAILURE: http://
SUCCESS: http://
deb: http://
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:578
http://
Executed test runs:
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
UNSTABLE: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:578
http://
Executed test runs:
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
UNSTABLE: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Preview Diff
1 | === removed directory 'apparmor' |
2 | === removed file 'apparmor/click.rules' |
3 | --- apparmor/click.rules 2014-05-23 09:35:07 +0000 |
4 | +++ apparmor/click.rules 1970-01-01 00:00:00 +0000 |
5 | @@ -1,12 +0,0 @@ |
6 | -dbus (receive, send) |
7 | - bus=session |
8 | - path=/com/canonical/Autopilot/**, |
9 | -# Allow writes to various (application-specific) XDG directories |
10 | - owner @{HOME}/autopilot/fakeenv/*/.cache/@{APP_PKGNAME}/ rw, # subdir of XDG_CACHE_HOME |
11 | - owner @{HOME}/autopilot/fakeenv/*/.cache/@{APP_PKGNAME}/** mrwkl, |
12 | - owner @{HOME}/autopilot/fakeenv/*/.config/@{APP_PKGNAME}/ rw, # subdir of XDG_CONFIG_HOME |
13 | - owner @{HOME}/autopilot/fakeenv/*/.config/@{APP_PKGNAME}/** mrwkl, |
14 | - owner @{HOME}/autopilot/fakeenv/*/.local/share/@{APP_PKGNAME}/ rw, # subdir of XDG_DATA_HOME |
15 | - owner @{HOMEDIRS}/*/autopilot/fakeenv/*/.local/share/@{APP_PKGNAME}/** mrwklix, |
16 | - owner @{HOME}/autopilot/fakeenv/*/confined/@{APP_PKGNAME}/ rw, # subdir of XDG_RUNTIME_DIR |
17 | - owner @{HOME}/autopilot/fakeenv/*/confined/@{APP_PKGNAME}/** mrwkl, |
18 | |
19 | === modified file 'autopilot/introspection/_object_registry.py' |
20 | --- autopilot/introspection/_object_registry.py 2015-02-25 21:51:40 +0000 |
21 | +++ autopilot/introspection/_object_registry.py 2015-05-06 10:03:54 +0000 |
22 | @@ -1,7 +1,7 @@ |
23 | # -*- Mode: Python; coding: utf-8; indent-tabs-mode: nil; tab-width: 4 -*- |
24 | # |
25 | # Autopilot Functional Test Tool |
26 | -# Copyright (C) 2014 Canonical |
27 | +# Copyright (C) 2014, 2015 Canonical |
28 | # |
29 | # This program is free software: you can redistribute it and/or modify |
30 | # it under the terms of the GNU General Public License as published by |
31 | @@ -134,7 +134,6 @@ |
32 | :param object_id: The _id attribute of the class doing the lookup. This is |
33 | used to index into the object registry to retrieve the dict of proxy |
34 | classes to try. |
35 | - :param default_class: default class to use if nothing in dict matches |
36 | :param path: dbus path |
37 | :param state: dbus state |
38 | :returns: appropriate custom proxy class |
39 | @@ -153,7 +152,8 @@ |
40 | """Identify which custom proxy class matches the dbus path and state. |
41 | |
42 | If more than one class in proxy_class_dict matches, raise an exception. |
43 | - :param proxy_class_dict: dict of proxy classes to try |
44 | + |
45 | + :param object_id: id to use to get the dict of proxy classes to try |
46 | :param path: dbus path |
47 | :param state: dbus state dict |
48 | :returns: matching custom proxy class |
49 | @@ -173,18 +173,80 @@ |
50 | ) |
51 | ) |
52 | if len(possible_classes) == 1: |
53 | + extended_proxy_bases = _get_proxy_bases_for_id(object_id) |
54 | + mixed = _combine_base_and_extensions( |
55 | + possible_classes[0], |
56 | + extended_proxy_bases |
57 | + ) |
58 | + possible_classes[0].__bases__ = mixed |
59 | return possible_classes[0] |
60 | return None |
61 | |
62 | |
63 | +def _combine_base_and_extensions(kls, extensions): |
64 | + """Returns the bases of the given class augmented with extensions |
65 | + |
66 | + In order to get the right bases tuple, the given class is removed |
67 | + from the result (to prevent cyclic dependencies), there's only one |
68 | + occurrence of each final base class in the result and the result |
69 | + is ordered following the inheritance order (classes lower in the |
70 | + inheritance tree are listed before in the resulting tuple) |
71 | + |
72 | + :param kls: class for which we are combining bases and extensions |
73 | + :param extensions: tuple of extensions to be added to kls' bases |
74 | + :returns: bases tuple for kls, including its former bases and the |
75 | + extensions |
76 | + |
77 | + """ |
78 | + # set of bases + extensions removing the original class to prevent |
79 | + # TypeError: a __bases__ item causes an inheritance cycle |
80 | + unique_bases = {x for x in kls.__bases__ + extensions if x != kls} |
81 | + |
82 | + # sort them taking into account inheritance to prevent |
83 | + # TypeError: Cannot create a consistent method resolution order (MRO) |
84 | + return tuple( |
85 | + sorted( |
86 | + unique_bases, |
87 | + key=lambda cls: _get_mro_sort_order(cls, extensions), |
88 | + reverse=True |
89 | + ) |
90 | + ) |
91 | + |
92 | + |
93 | +def _get_mro_sort_order(cls, promoted_collection=()): |
94 | + """Returns the comparable numerical order for the given class honouring |
95 | + its MRO |
96 | + |
97 | + It accepts an optional parameter for promoting classes in a certain |
98 | + group, this can give more control over the sorting when two classes |
99 | + have the a MRO of the same length |
100 | + |
101 | + :param cls: the subject class |
102 | + :param promoted_collection: tuple of classes which must be promoted |
103 | + :returns: comparable numerical order, higher for classes with MROs of |
104 | + greater length |
105 | + |
106 | + """ |
107 | + # Multiplying by 2 the lenght of the MRO list gives the chance to promote |
108 | + # items in the promoted_collection by adding them 1 later: non promoted |
109 | + # classes will have even scores and promoted classes with MRO of the same |
110 | + # length will have odd scores one point higher |
111 | + order = 2 * len(cls.mro()) |
112 | + |
113 | + if cls in promoted_collection: |
114 | + order += 1 |
115 | + |
116 | + return order |
117 | + |
118 | + |
119 | def _get_default_proxy_class(id, name): |
120 | """Return a custom proxy object class of the default or a base class. |
121 | |
122 | We want the object to inherit from the class that is set as the emulator |
123 | - base class, not the class that is doing the selecting (which will be the |
124 | - 'default_class' parameter). |
125 | + base class, not the class that is doing the selecting. Using the passed id |
126 | + we retrieve the relevant bases from the object registry. |
127 | |
128 | - :param default_class: default class to use if no bases match |
129 | + :param id: The object id (_id attribute) of the class doing the lookup. |
130 | :param name: name of new class |
131 | :returns: custom proxy object class |
132 | |
133 | |
134 | === modified file 'autopilot/introspection/_search.py' |
135 | --- autopilot/introspection/_search.py 2015-04-02 08:27:42 +0000 |
136 | +++ autopilot/introspection/_search.py 2015-05-06 10:03:54 +0000 |
137 | @@ -387,12 +387,15 @@ |
138 | # make sure we always have an emulator base. Either use the one the user |
139 | # gave us, or make one: |
140 | emulator_base = emulator_base or _make_default_emulator_base() |
141 | + _raise_if_base_class_not_actually_base(emulator_base) |
142 | + |
143 | # Get the dbus introspection Xml for the backend. |
144 | intro_xml = _get_introspection_xml_from_backend(dbus_address) |
145 | try: |
146 | # Figure out if the backend has any extension methods, and return |
147 | # classes that understand how to use each of those extensions: |
148 | extension_classes = _get_proxy_bases_from_introspection_xml(intro_xml) |
149 | + |
150 | # Register those base classes for everything that will derive from this |
151 | # emulator base class. |
152 | _object_registry.register_extension_classes_for_proxy_base( |
153 | @@ -427,6 +430,38 @@ |
154 | return type("DefaultEmulatorBase", (ap_dbus.DBusIntrospectionObject,), {}) |
155 | |
156 | |
157 | +WRONG_CPO_CLASS_MSG = '''\ |
158 | +base_class: {passed} does not appear to be the actual base CPO class. |
159 | +Perhaps you meant to use: {actual}.''' |
160 | + |
161 | + |
162 | +def _raise_if_base_class_not_actually_base(base_class): |
163 | + """Raises ValueError if the provided base_class is not actually the |
164 | + base_class |
165 | + |
166 | + To ensure that the expected base classes are used when creating proxy |
167 | + objects. |
168 | + |
169 | + :param base_class: The base class to check. |
170 | + :raises ValueError: The actual base class is not the one provided |
171 | + |
172 | + """ |
173 | + actual_base_class = base_class |
174 | + for cls in base_class.mro(): |
175 | + if hasattr(cls, '_id'): |
176 | + actual_base_class = cls |
177 | + |
178 | + if actual_base_class != base_class: |
179 | + raise( |
180 | + ValueError( |
181 | + WRONG_CPO_CLASS_MSG.format( |
182 | + passed=base_class, |
183 | + actual=actual_base_class |
184 | + ) |
185 | + ) |
186 | + ) |
187 | + |
188 | + |
189 | def _make_proxy_object_async( |
190 | data_source, emulator_base, reply_handler, error_handler): |
191 | """Make a proxy object for a dbus backend. |
192 | |
193 | === modified file 'autopilot/tests/functional/test_introspection_features.py' |
194 | --- autopilot/tests/functional/test_introspection_features.py 2015-02-26 00:40:54 +0000 |
195 | +++ autopilot/tests/functional/test_introspection_features.py 2015-05-06 10:03:54 +0000 |
196 | @@ -24,7 +24,7 @@ |
197 | import subprocess |
198 | import tempfile |
199 | from tempfile import mktemp |
200 | -from testtools import skip, skipIf |
201 | +from testtools import skipIf |
202 | from testtools.matchers import ( |
203 | Contains, |
204 | Equals, |
205 | @@ -36,6 +36,7 @@ |
206 | StartsWith, |
207 | ) |
208 | from textwrap import dedent |
209 | +from unittest.mock import patch |
210 | from io import StringIO |
211 | |
212 | from autopilot import platform |
213 | @@ -43,6 +44,8 @@ |
214 | from autopilot.testcase import AutopilotTestCase |
215 | from autopilot.tests.functional.fixtures import TempDesktopFile |
216 | from autopilot.introspection import CustomEmulatorBase |
217 | +from autopilot.introspection import _object_registry as object_registry |
218 | +from autopilot.introspection import _search |
219 | from autopilot.introspection.qt import QtObjectProxyMixin |
220 | from autopilot.display import Display |
221 | |
222 | @@ -99,7 +102,6 @@ |
223 | Equals(WindowMockerApp) |
224 | ) |
225 | |
226 | - @skip("Currently fails due to lp:1425721 (and lp:1376996)") |
227 | def test_customised_proxy_classes_have_extension_classes(self): |
228 | class WindowMockerApp(EmulatorBase): |
229 | @classmethod |
230 | @@ -109,6 +111,49 @@ |
231 | app = self.start_mock_app(EmulatorBase) |
232 | self.assertThat(app.__class__.__bases__, Contains(QtObjectProxyMixin)) |
233 | |
234 | + def test_customised_proxy_classes_have_multiple_extension_classes(self): |
235 | + with object_registry.patch_registry({}): |
236 | + class SecondEmulatorBase(CustomEmulatorBase): |
237 | + pass |
238 | + |
239 | + class WindowMockerApp(EmulatorBase, SecondEmulatorBase): |
240 | + @classmethod |
241 | + def validate_dbus_object(cls, path, _state): |
242 | + return path == b'/window-mocker' |
243 | + |
244 | + app = self.start_mock_app(EmulatorBase) |
245 | + self.assertThat(app.__class__.__bases__, Contains(EmulatorBase)) |
246 | + self.assertThat( |
247 | + app.__class__.__bases__, |
248 | + Contains(SecondEmulatorBase) |
249 | + ) |
250 | + |
251 | + def test_handles_using_app_cpo_base_class(self): |
252 | + # This test replicates an issue found in an application test suite |
253 | + # where using the App CPO caused an exception. |
254 | + with object_registry.patch_registry({}): |
255 | + class WindowMockerApp(CustomEmulatorBase): |
256 | + @classmethod |
257 | + def validate_dbus_object(cls, path, _state): |
258 | + return path == b'/window-mocker' |
259 | + |
260 | + self.start_mock_app(WindowMockerApp) |
261 | + |
262 | + def test_warns_when_using_incorrect_cpo_base_class(self): |
263 | + # Ensure the warning method is called when launching a proxy. |
264 | + with object_registry.patch_registry({}): |
265 | + class TestCPO(CustomEmulatorBase): |
266 | + pass |
267 | + |
268 | + class WindowMockerApp(TestCPO): |
269 | + @classmethod |
270 | + def validate_dbus_object(cls, path, _state): |
271 | + return path == b'/window-mocker' |
272 | + |
273 | + with patch.object(_search, 'logger') as p_logger: |
274 | + self.start_mock_app(WindowMockerApp) |
275 | + self.assertTrue(p_logger.warning.called) |
276 | + |
277 | def test_can_select_custom_emulators_by_name(self): |
278 | """Must be able to select a custom emulator type by name.""" |
279 | class MouseTestWidget(EmulatorBase): |
280 | |
281 | === modified file 'autopilot/tests/unit/test_introspection_object_registry.py' |
282 | --- autopilot/tests/unit/test_introspection_object_registry.py 2014-07-31 04:42:08 +0000 |
283 | +++ autopilot/tests/unit/test_introspection_object_registry.py 2015-05-06 10:03:54 +0000 |
284 | @@ -1,7 +1,7 @@ |
285 | # -*- Mode: Python; coding: utf-8; indent-tabs-mode: nil; tab-width: 4 -*- |
286 | # |
287 | # Autopilot Functional Test Tool |
288 | -# Copyright (C) 2014 Canonical |
289 | +# Copyright (C) 2014, 2015 Canonical |
290 | # |
291 | # This program is free software: you can redistribute it and/or modify |
292 | # it under the terms of the GNU General Public License as published by |
293 | @@ -263,3 +263,149 @@ |
294 | with object_registry.patch_registry(dict()): |
295 | object_registry._object_registry[token] = token |
296 | self.assertFalse(token in object_registry._object_registry) |
297 | + |
298 | + |
299 | +class CombineBasesTests(TestCase): |
300 | + def test_returns_original_if_no_extensions(self): |
301 | + class Base(): |
302 | + pass |
303 | + |
304 | + class Sub(Base): |
305 | + pass |
306 | + |
307 | + self.assertEqual( |
308 | + object_registry._combine_base_and_extensions(Sub, ()), |
309 | + (Base,) |
310 | + ) |
311 | + |
312 | + def test_returns_addition_of_extension_classes(self): |
313 | + class Base(): |
314 | + pass |
315 | + |
316 | + class Sub(Base): |
317 | + pass |
318 | + |
319 | + class Ext1(): |
320 | + pass |
321 | + |
322 | + class Ext2(): |
323 | + pass |
324 | + |
325 | + mixed = object_registry._combine_base_and_extensions(Sub, (Ext1, Ext2)) |
326 | + |
327 | + self.assertIn(Ext1, mixed) |
328 | + self.assertIn(Ext2, mixed) |
329 | + |
330 | + def test_excludes_duplication_of_base_class_within_extension_classes(self): |
331 | + class Base(): |
332 | + pass |
333 | + |
334 | + class Sub(Base): |
335 | + pass |
336 | + |
337 | + class Ext1(): |
338 | + pass |
339 | + |
340 | + class Ext2(Ext1): |
341 | + pass |
342 | + |
343 | + self.assertEqual( |
344 | + object_registry._combine_base_and_extensions(Sub, (Ext2, Base)), |
345 | + (Ext2, Base,) |
346 | + ) |
347 | + |
348 | + def test_excludes_addition_of_extension_base_classes(self): |
349 | + class Base(): |
350 | + pass |
351 | + |
352 | + class Sub(Base): |
353 | + pass |
354 | + |
355 | + class Ext1(): |
356 | + pass |
357 | + |
358 | + class Ext2(Ext1): |
359 | + pass |
360 | + |
361 | + self.assertNotIn( |
362 | + object_registry._combine_base_and_extensions(Sub, (Ext2,)), |
363 | + Ext1 |
364 | + ) |
365 | + |
366 | + def test_excludes_addition_of_subject_class(self): |
367 | + class Base(): |
368 | + pass |
369 | + |
370 | + class Sub(Base): |
371 | + pass |
372 | + |
373 | + class Ext1(): |
374 | + pass |
375 | + |
376 | + self.assertEqual( |
377 | + object_registry._combine_base_and_extensions(Sub, (Ext1, Sub)), |
378 | + (Ext1, Base) |
379 | + ) |
380 | + |
381 | + def test_maintains_mro_order(self): |
382 | + class Base(): |
383 | + pass |
384 | + |
385 | + class Sub1(Base): |
386 | + pass |
387 | + |
388 | + class Sub2(Sub1, Base): |
389 | + pass |
390 | + |
391 | + class Ext1(): |
392 | + pass |
393 | + |
394 | + mixed = object_registry._combine_base_and_extensions(Sub2, (Ext1,)) |
395 | + |
396 | + self.assertLess( |
397 | + mixed.index(Sub1), |
398 | + mixed.index(Base) |
399 | + ) |
400 | + |
401 | + |
402 | +class MROSortOrderTests(TestCase): |
403 | + def test_returns_lower_values_for_lower_classes_in_mro(self): |
404 | + class Parent(): |
405 | + pass |
406 | + |
407 | + class Child(Parent): |
408 | + pass |
409 | + |
410 | + class GrandChild(Child): |
411 | + pass |
412 | + |
413 | + self.assertGreater( |
414 | + object_registry._get_mro_sort_order(Child), |
415 | + object_registry._get_mro_sort_order(Parent), |
416 | + ) |
417 | + self.assertGreater( |
418 | + object_registry._get_mro_sort_order(GrandChild), |
419 | + object_registry._get_mro_sort_order(Child), |
420 | + ) |
421 | + |
422 | + def test_return_higher_values_for_promoted_collections_in_ties(self): |
423 | + class Parent1(): |
424 | + pass |
425 | + |
426 | + class Child1(Parent1): |
427 | + pass |
428 | + |
429 | + class Parent2(): |
430 | + pass |
431 | + |
432 | + class Child2(Parent2): |
433 | + pass |
434 | + |
435 | + promoted_collection = (Parent1, Child1) |
436 | + |
437 | + self.assertGreater( |
438 | + object_registry._get_mro_sort_order( |
439 | + Child1, promoted_collection), |
440 | + object_registry._get_mro_sort_order( |
441 | + Child2, promoted_collection), |
442 | + ) |
443 | |
444 | === modified file 'autopilot/tests/unit/test_introspection_search.py' |
445 | --- autopilot/tests/unit/test_introspection_search.py 2015-04-02 08:27:42 +0000 |
446 | +++ autopilot/tests/unit/test_introspection_search.py 2015-05-06 10:03:54 +0000 |
447 | @@ -34,6 +34,8 @@ |
448 | from autopilot.exceptions import ProcessSearchError |
449 | from autopilot.utilities import sleep |
450 | from autopilot.introspection import _search as _s |
451 | + |
452 | +from autopilot.introspection import CustomEmulatorBase |
453 | from autopilot.introspection.constants import AUTOPILOT_PATH |
454 | |
455 | |
456 | @@ -730,3 +732,95 @@ |
457 | _s._find_matching_connections(bus, lambda *args: True) |
458 | |
459 | dedupe.assert_called_once_with(["conn1"], bus) |
460 | + |
461 | + |
462 | +class ActualBaseClassTests(TestCase): |
463 | + |
464 | + def test_dont_raise_passed_base_when_is_only_base(self): |
465 | + class ActualBase(CustomEmulatorBase): |
466 | + pass |
467 | + |
468 | + try: |
469 | + _s._raise_if_base_class_not_actually_base(ActualBase) |
470 | + except ValueError: |
471 | + self.fail('Unexpected ValueError exception') |
472 | + |
473 | + def test_raises_if_passed_incorrect_base_class(self): |
474 | + class ActualBase(CustomEmulatorBase): |
475 | + pass |
476 | + |
477 | + class InheritedCPO(ActualBase): |
478 | + pass |
479 | + |
480 | + self.assertRaises( |
481 | + ValueError, |
482 | + _s._raise_if_base_class_not_actually_base, |
483 | + InheritedCPO |
484 | + ) |
485 | + |
486 | + def test_raises_parent_with_simple_non_ap_multi_inheritance(self): |
487 | + """When mixing in non-customproxy classes must return the base.""" |
488 | + |
489 | + class ActualBase(CustomEmulatorBase): |
490 | + pass |
491 | + |
492 | + class InheritedCPO(ActualBase): |
493 | + pass |
494 | + |
495 | + class TrickyOne(object): |
496 | + pass |
497 | + |
498 | + class FinalForm(InheritedCPO, TrickyOne): |
499 | + pass |
500 | + |
501 | + self.assertRaises( |
502 | + ValueError, |
503 | + _s._raise_if_base_class_not_actually_base, |
504 | + FinalForm |
505 | + ) |
506 | + |
507 | + def test_raises_parent_with_non_ap_multi_inheritance(self): |
508 | + |
509 | + class ActualBase(CustomEmulatorBase): |
510 | + pass |
511 | + |
512 | + class InheritedCPO(ActualBase): |
513 | + pass |
514 | + |
515 | + class TrickyOne(object): |
516 | + pass |
517 | + |
518 | + class FinalForm(TrickyOne, InheritedCPO): |
519 | + pass |
520 | + |
521 | + self.assertRaises( |
522 | + ValueError, |
523 | + _s._raise_if_base_class_not_actually_base, |
524 | + FinalForm |
525 | + ) |
526 | + |
527 | + def test_dont_raise_when_using_default_emulator_base(self): |
528 | + # _make_proxy_object potentially creates a default base. |
529 | + DefaultBase = _s._make_default_emulator_base() |
530 | + try: |
531 | + _s._raise_if_base_class_not_actually_base(DefaultBase) |
532 | + except ValueError: |
533 | + self.fail('Unexpected ValueError exception') |
534 | + |
535 | + def test_exception_message_contains_useful_information(self): |
536 | + class ActualBase(CustomEmulatorBase): |
537 | + pass |
538 | + |
539 | + class InheritedCPO(ActualBase): |
540 | + pass |
541 | + |
542 | + try: |
543 | + _s._raise_if_base_class_not_actually_base(InheritedCPO) |
544 | + except ValueError as err: |
545 | + self.assertEqual( |
546 | + str(err), |
547 | + _s.WRONG_CPO_CLASS_MSG.format( |
548 | + passed=InheritedCPO, |
549 | + actual=ActualBase |
550 | + ) |
551 | + ) |
552 | |
553 | === removed file 'debian/autopilot-touch.dirs' |
554 | --- debian/autopilot-touch.dirs 2013-09-20 19:06:30 +0000 |
555 | +++ debian/autopilot-touch.dirs 1970-01-01 00:00:00 +0000 |
556 | @@ -1,1 +0,0 @@ |
557 | -usr/share/autopilot-touch/apparmor |
558 | |
559 | === removed file 'debian/autopilot-touch.install' |
560 | --- debian/autopilot-touch.install 2013-09-20 19:06:30 +0000 |
561 | +++ debian/autopilot-touch.install 1970-01-01 00:00:00 +0000 |
562 | @@ -1,1 +0,0 @@ |
563 | -apparmor/click.rules usr/share/autopilot-touch/apparmor |
564 | |
565 | === modified file 'debian/changelog' |
566 | --- debian/changelog 2015-02-26 21:20:18 +0000 |
567 | +++ debian/changelog 2015-05-06 10:03:54 +0000 |
568 | @@ -1,12 +1,19 @@ |
569 | autopilot (1.5.0) UNRELEASED; urgency=medium |
570 | |
571 | + [ Christopher Lee ] |
572 | * Fix for desktop file name change (lp:1411096) |
573 | Fix for config value containing '=' char (lp:1408317) |
574 | Fix for skipped tests not appearing in log (lp:1414632) |
575 | Add json docs build (for web publish) (lp:1409778) |
576 | Documentation improvements for API, Tutorial, FAQ, and Guidelines |
577 | |
578 | - -- Christopher Lee <chris.lee@canonical.com> Fri, 27 Feb 2015 10:16:42 +1300 |
579 | + [ Vincent Ladeuil ] |
580 | + * Remove apparmor/click.rules which is now shipped by dbus-property-service. |
581 | + |
582 | + [ Federico Gimenez ] |
583 | + * Fixed CPO class resolution (lp:1425721, lp:1376996) |
584 | + |
585 | + -- Federico Gimenez <fgimenez@canonical.com> mon, 4 May 2015 12:12:42 +0200 |
586 | |
587 | autopilot (1.5.0+15.04.20141031-0ubuntu1) vivid; urgency=low |
588 | |
589 | |
590 | === modified file 'debian/rules' (properties changed: -x to +x) |
Hi Federico, you'll see that I added some tests but haven't filled them in.
While back-filling the tests but stopped short of actually implementing them; mainly because the way you've made the fix is different to what I originally had and my initial thoughts wouldn't work :-)
Could you take a look at writing the tests? My current concern with the current implementation is that _combine_ base_and_ extensions has to know about class stuff (__bases__ and mro) where as the previous method only dealt with tuples of information.
Not only does this mean that it didn't need to know more than 'merge these tuples' it was easy to write tests for (again, just dealing with tuples if data, could be strings etc.).
Also, without a test in place I'm not sure that the sorting of the mro is the correct thing to do.
While I didn't have a test in place to prove it, the intention of the purely tuple approach is that it kept the expected order of merging 2 bases.