Merge lp:~elopio/u1-test-utils/logger_decorator into lp:u1-test-utils
- logger_decorator
- Merge into trunk
Status: | Merged |
---|---|
Approved by: | Natalia Bidart |
Approved revision: | 55 |
Merged at revision: | 45 |
Proposed branch: | lp:~elopio/u1-test-utils/logger_decorator |
Merge into: | lp:u1-test-utils |
Prerequisite: | lp:~elopio/u1-test-utils/assert_page_path |
Diff against target: |
221 lines (+99/-1) 3 files modified
u1testutils/sst/__init__.py (+24/-0) u1testutils/sst/selftests/unit/test_pages.py (+60/-1) u1testutils/sst/sso/pages.py (+15/-0) |
To merge this branch: | bzr merge lp:~elopio/u1-test-utils/logger_decorator |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Natalia Bidart (community) | Approve | ||
Review via email: mp+157925@code.launchpad.net |
Commit message
Added the actions logger decorator for the page objects.
Description of the change
- 49. By Leo Arias
-
Improved the logger using a more explicit approach with the instance as parameter.
- 50. By Leo Arias
-
Improve the logger decorator to receive the logging funciton.
- 51. By Leo Arias
-
Removed the log_func default.
- 52. By Leo Arias
-
Remove the unnecessary test.
- 53. By Leo Arias
-
Fixed lint.
Natalia Bidart (nataliabidart) wrote : | # |
I made some tweaks to this logging function and pasted them here:
https:/
I added comments inline explaining the changes, but will duplicate those here for the sake of the review:
"Creating several strings depending on conditions is expensive.
Also, is better to have non-chaging log messages to be able to
grep for specific keywords when debugging, so, I prefer to always
log the args and kwargs even if they are not set, since the fact
that no args nor kwargs were given is an important info to log.
Is particularly important to use %r and not %s.
You should not format the string to log outside the logging
function, for mainly 2 reasons: the formatting may not be needed
since the currently set log level may be higher than the level of
log_func, and because there may be a formatting error that could
raise an exception making the whole call to 'f' fail (and we want
to avoid that)."
Natalia Bidart (nataliabidart) wrote : | # |
Looks great! Thanks a lot.
Preview Diff
1 | === modified file 'u1testutils/sst/__init__.py' | |||
2 | --- u1testutils/sst/__init__.py 2013-04-10 19:25:21 +0000 | |||
3 | +++ u1testutils/sst/__init__.py 2013-04-12 15:18:27 +0000 | |||
4 | @@ -18,10 +18,34 @@ | |||
5 | 18 | 18 | ||
6 | 19 | import sst.actions | 19 | import sst.actions |
7 | 20 | 20 | ||
8 | 21 | from functools import wraps | ||
9 | 22 | |||
10 | 21 | 23 | ||
11 | 22 | logger = logging.getLogger('User test') | 24 | logger = logging.getLogger('User test') |
12 | 23 | 25 | ||
13 | 24 | 26 | ||
14 | 27 | def log_action(log_func): | ||
15 | 28 | """Decorator to log the call of an action method.""" | ||
16 | 29 | |||
17 | 30 | def middle(f): | ||
18 | 31 | |||
19 | 32 | @wraps(f) | ||
20 | 33 | def inner(instance, *args, **kwargs): | ||
21 | 34 | class_name = str(instance.__class__.__name__) | ||
22 | 35 | docstring = f.__doc__ | ||
23 | 36 | if docstring: | ||
24 | 37 | docstring = docstring.split('\n')[0].strip() | ||
25 | 38 | else: | ||
26 | 39 | docstring = f.__name__ | ||
27 | 40 | log_line = '%r: %r. Arguments %r. Keyword arguments: %r.' | ||
28 | 41 | log_func(log_line, class_name, docstring, args, kwargs) | ||
29 | 42 | return f(instance, *args, **kwargs) | ||
30 | 43 | |||
31 | 44 | return inner | ||
32 | 45 | |||
33 | 46 | return middle | ||
34 | 47 | |||
35 | 48 | |||
36 | 25 | class Page(object): | 49 | class Page(object): |
37 | 26 | """Base class for the page objects used in acceptance testing. | 50 | """Base class for the page objects used in acceptance testing. |
38 | 27 | 51 | ||
39 | 28 | 52 | ||
40 | === modified file 'u1testutils/sst/selftests/unit/test_pages.py' | |||
41 | --- u1testutils/sst/selftests/unit/test_pages.py 2013-04-08 18:43:37 +0000 | |||
42 | +++ u1testutils/sst/selftests/unit/test_pages.py 2013-04-12 15:18:27 +0000 | |||
43 | @@ -12,10 +12,13 @@ | |||
44 | 12 | # You should have received a copy of the GNU General Public License along | 12 | # You should have received a copy of the GNU General Public License along |
45 | 13 | # with this program. If not, see <http://www.gnu.org/licenses/>. | 13 | # with this program. If not, see <http://www.gnu.org/licenses/>. |
46 | 14 | 14 | ||
47 | 15 | import logging | ||
48 | 15 | import mock | 16 | import mock |
49 | 16 | import testtools | 17 | import testtools |
50 | 17 | 18 | ||
52 | 18 | from u1testutils.sst import Page | 19 | import u1testutils.logging |
53 | 20 | |||
54 | 21 | from u1testutils.sst import log_action, Page | ||
55 | 19 | 22 | ||
56 | 20 | 23 | ||
57 | 21 | class PageTestCase(testtools.TestCase): | 24 | class PageTestCase(testtools.TestCase): |
58 | @@ -58,3 +61,59 @@ | |||
59 | 58 | page.headings2 = [] | 61 | page.headings2 = [] |
60 | 59 | page.assert_page_is_open() | 62 | page.assert_page_is_open() |
61 | 60 | assert not mock_assert.called | 63 | assert not mock_assert.called |
62 | 64 | |||
63 | 65 | |||
64 | 66 | class PageWithLogDecorator(Page): | ||
65 | 67 | |||
66 | 68 | @log_action(logging.info) | ||
67 | 69 | def do_something_without_docstring(self, *args, **kwargs): | ||
68 | 70 | pass | ||
69 | 71 | |||
70 | 72 | @log_action(logging.info) | ||
71 | 73 | def do_something_with_docstring(self, *args, **kwargs): | ||
72 | 74 | """Do something with docstring.""" | ||
73 | 75 | pass | ||
74 | 76 | |||
75 | 77 | @log_action(logging.info) | ||
76 | 78 | def do_something_with_multiline_docstring(self, *args, **kwargs): | ||
77 | 79 | """Do something with a multiline docstring. | ||
78 | 80 | |||
79 | 81 | This should not be logged. | ||
80 | 82 | """ | ||
81 | 83 | pass | ||
82 | 84 | |||
83 | 85 | |||
84 | 86 | class PageLoggingTestCase(u1testutils.logging.LogHandlerTestCase): | ||
85 | 87 | |||
86 | 88 | def setUp(self): | ||
87 | 89 | super(PageLoggingTestCase, self).setUp() | ||
88 | 90 | self.root_logger.setLevel(logging.INFO) | ||
89 | 91 | self.page = PageWithLogDecorator(check=False) | ||
90 | 92 | |||
91 | 93 | def test_logged_action_without_docstring(self): | ||
92 | 94 | self.page.do_something_without_docstring( | ||
93 | 95 | 'arg1', 'arg2', arg3='arg3', arg4='arg4') | ||
94 | 96 | self.assertLogLevelContains( | ||
95 | 97 | 'INFO', | ||
96 | 98 | "'PageWithLogDecorator': 'do_something_without_docstring'. " | ||
97 | 99 | "Arguments ('arg1', 'arg2'). " | ||
98 | 100 | "Keyword arguments: {'arg3': 'arg3', 'arg4': 'arg4'}.") | ||
99 | 101 | |||
100 | 102 | def test_logged_action_with_docstring(self): | ||
101 | 103 | self.page.do_something_with_docstring( | ||
102 | 104 | 'arg1', 'arg2', arg3='arg3', arg4='arg4') | ||
103 | 105 | self.assertLogLevelContains( | ||
104 | 106 | 'INFO', | ||
105 | 107 | "'PageWithLogDecorator': 'Do something with docstring.'. " | ||
106 | 108 | "Arguments ('arg1', 'arg2'). " | ||
107 | 109 | "Keyword arguments: {'arg3': 'arg3', 'arg4': 'arg4'}.") | ||
108 | 110 | |||
109 | 111 | def test_logged_action_with_multiline_docstring(self): | ||
110 | 112 | self.page.do_something_with_multiline_docstring( | ||
111 | 113 | 'arg1', 'arg2', arg3='arg3', arg4='arg4') | ||
112 | 114 | self.assertLogLevelContains( | ||
113 | 115 | 'INFO', | ||
114 | 116 | "'PageWithLogDecorator': " | ||
115 | 117 | "'Do something with a multiline docstring.'. " | ||
116 | 118 | "Arguments ('arg1', 'arg2'). " | ||
117 | 119 | "Keyword arguments: {'arg3': 'arg3', 'arg4': 'arg4'}.") | ||
118 | 61 | 120 | ||
119 | === modified file 'u1testutils/sst/sso/pages.py' | |||
120 | --- u1testutils/sst/sso/pages.py 2013-04-10 19:47:20 +0000 | |||
121 | +++ u1testutils/sst/sso/pages.py 2013-04-12 15:18:27 +0000 | |||
122 | @@ -12,10 +12,14 @@ | |||
123 | 12 | # You should have received a copy of the GNU General Public License along | 12 | # You should have received a copy of the GNU General Public License along |
124 | 13 | # with this program. If not, see <http://www.gnu.org/licenses/>. | 13 | # with this program. If not, see <http://www.gnu.org/licenses/>. |
125 | 14 | 14 | ||
126 | 15 | import logging | ||
127 | 16 | |||
128 | 15 | import sst.actions | 17 | import sst.actions |
129 | 16 | 18 | ||
130 | 17 | import u1testutils.sst | 19 | import u1testutils.sst |
131 | 18 | 20 | ||
132 | 21 | from u1testutils.sst import log_action | ||
133 | 22 | |||
134 | 19 | 23 | ||
135 | 20 | class LogIn(u1testutils.sst.Page): | 24 | class LogIn(u1testutils.sst.Page): |
136 | 21 | """Log in page of the Ubuntu Single Sign On website. | 25 | """Log in page of the Ubuntu Single Sign On website. |
137 | @@ -39,6 +43,7 @@ | |||
138 | 39 | sst.actions.assert_element(tag='h1', text='Ubuntu Single Sign On') | 43 | sst.actions.assert_element(tag='h1', text='Ubuntu Single Sign On') |
139 | 40 | sst.actions.assert_element(tag='h2', text='Are you new?') | 44 | sst.actions.assert_element(tag='h2', text='Are you new?') |
140 | 41 | 45 | ||
141 | 46 | @log_action(logging.info) | ||
142 | 42 | def log_in_to_site_recognized(self, user=None): | 47 | def log_in_to_site_recognized(self, user=None): |
143 | 43 | """Fill the log in form and continue to the site that requested it. | 48 | """Fill the log in form and continue to the site that requested it. |
144 | 44 | 49 | ||
145 | @@ -51,6 +56,7 @@ | |||
146 | 51 | """ | 56 | """ |
147 | 52 | self._log_in(user) | 57 | self._log_in(user) |
148 | 53 | 58 | ||
149 | 59 | @log_action(logging.info) | ||
150 | 54 | def log_in_to_site_not_recognized(self, user=None): | 60 | def log_in_to_site_not_recognized(self, user=None): |
151 | 55 | """Fill the log in form and continue to the next step. | 61 | """Fill the log in form and continue to the next step. |
152 | 56 | 62 | ||
153 | @@ -87,6 +93,7 @@ | |||
154 | 87 | css_class='btn', name='continue') | 93 | css_class='btn', name='continue') |
155 | 88 | sst.actions.click_button(continue_button) | 94 | sst.actions.click_button(continue_button) |
156 | 89 | 95 | ||
157 | 96 | @log_action(logging.info) | ||
158 | 90 | def go_to_create_new_account(self): | 97 | def go_to_create_new_account(self): |
159 | 91 | """Go to the Create new account page.""" | 98 | """Go to the Create new account page.""" |
160 | 92 | new_account_link = sst.actions.get_element(id='new-account-link') | 99 | new_account_link = sst.actions.get_element(id='new-account-link') |
161 | @@ -109,6 +116,7 @@ | |||
162 | 109 | title = 'Create account' | 116 | title = 'Create account' |
163 | 110 | url_path = '/\+new_account' | 117 | url_path = '/\+new_account' |
164 | 111 | 118 | ||
165 | 119 | @log_action(logging.info) | ||
166 | 112 | def create_ubuntu_sso_account(self, user): | 120 | def create_ubuntu_sso_account(self, user): |
167 | 113 | """Fill the new account form and continue to the next step. | 121 | """Fill the new account form and continue to the next step. |
168 | 114 | 122 | ||
169 | @@ -147,6 +155,7 @@ | |||
170 | 147 | title = 'Registration mail sent' | 155 | title = 'Registration mail sent' |
171 | 148 | url_path = '/\+new_account' | 156 | url_path = '/\+new_account' |
172 | 149 | 157 | ||
173 | 158 | @log_action(logging.info) | ||
174 | 150 | def confirm_email_to_site_recognized(self, confirmation_code): | 159 | def confirm_email_to_site_recognized(self, confirmation_code): |
175 | 151 | """Confirm email and continue to the site that requested the log in. | 160 | """Confirm email and continue to the site that requested the log in. |
176 | 152 | 161 | ||
177 | @@ -157,6 +166,7 @@ | |||
178 | 157 | """ | 166 | """ |
179 | 158 | self._confirm_email(confirmation_code) | 167 | self._confirm_email(confirmation_code) |
180 | 159 | 168 | ||
181 | 169 | @log_action(logging.info) | ||
182 | 160 | def confirm_email_to_site_not_recognized(self, confirmation_code): | 170 | def confirm_email_to_site_not_recognized(self, confirmation_code): |
183 | 161 | """Enter the confirmation code and continue to the next step. | 171 | """Enter the confirmation code and continue to the next step. |
184 | 162 | 172 | ||
185 | @@ -215,10 +225,12 @@ | |||
186 | 215 | cancel_link = sst.actions.get_element(tag='a', text='cancel') | 225 | cancel_link = sst.actions.get_element(tag='a', text='cancel') |
187 | 216 | sst.actions.click_link(cancel_link) | 226 | sst.actions.click_link(cancel_link) |
188 | 217 | 227 | ||
189 | 228 | @log_action(logging.info) | ||
190 | 218 | def confirm(self): | 229 | def confirm(self): |
191 | 219 | self._click_continue_button() | 230 | self._click_continue_button() |
192 | 220 | return YourAccount(self.user_name) | 231 | return YourAccount(self.user_name) |
193 | 221 | 232 | ||
194 | 233 | @log_action(logging.info) | ||
195 | 222 | def cancel(self): | 234 | def cancel(self): |
196 | 223 | return YourAccount(self.user_name) | 235 | return YourAccount(self.user_name) |
197 | 224 | 236 | ||
198 | @@ -249,6 +261,7 @@ | |||
199 | 249 | """ | 261 | """ |
200 | 250 | sst.actions.assert_title_contains(self.title, regex=True) | 262 | sst.actions.assert_title_contains(self.title, regex=True) |
201 | 251 | 263 | ||
202 | 264 | @log_action(logging.info) | ||
203 | 252 | def make_all_information_available_to_website(self): | 265 | def make_all_information_available_to_website(self): |
204 | 253 | """Select all the user available information. | 266 | """Select all the user available information. |
205 | 254 | 267 | ||
206 | @@ -265,6 +278,7 @@ | |||
207 | 265 | 'form[name="decideform"] > .info-items > .list > li > ' | 278 | 'form[name="decideform"] > .info-items > .list > li > ' |
208 | 266 | 'input[type="checkbox"]') | 279 | 'input[type="checkbox"]') |
209 | 267 | 280 | ||
210 | 281 | @log_action(logging.info) | ||
211 | 268 | def yes_sign_me_in(self): | 282 | def yes_sign_me_in(self): |
212 | 269 | """Accept to sign in to the site not recognized and go back to it.""" | 283 | """Accept to sign in to the site not recognized and go back to it.""" |
213 | 270 | sign_me_in_button = sst.actions.get_element(css_class='btn', | 284 | sign_me_in_button = sst.actions.get_element(css_class='btn', |
214 | @@ -299,6 +313,7 @@ | |||
215 | 299 | 313 | ||
216 | 300 | class _UserSubHeader(object): | 314 | class _UserSubHeader(object): |
217 | 301 | 315 | ||
218 | 316 | @log_action(logging.info) | ||
219 | 302 | def log_out(self): | 317 | def log_out(self): |
220 | 303 | """Log out from the web site.""" | 318 | """Log out from the web site.""" |
221 | 304 | sst.actions.click_link('logout-link') | 319 | sst.actions.click_link('logout-link') |
Looks good. About the decorator, instead of
11 + def log_and_call(*args, **kwargs): 0].__class_ _.__name_ _)
12 + class_name = str(args[
I would suggest using this approach, which is more explicit:
11 + def log_and_ call(instance, *args, **kwargs): __class_ _.__name_ _)
12 + class_name = str(instance.
...
25 + return method(instance, *args, **kwargs)
Futhermore, I would suggest adding a decorator that will receive the logging function as a param, thus allowing the caller to log using different levels. For example:
http:// bazaar. launchpad. net/~pycassos/ pycasa/ trunk/view/ head:/pycasa/ utils.py# L120
Nitpicking detail: there should be an empty blank line between the import <module> block and the from <module> import <feature> block.