9:12 a.m.
Hello gang,
while I was playing around with ResultsDB, I've come to some issues,
which bothered me to the level I wrote a fix :)
1) Common base class for the tests
-----------------------------------
[patch 0001 - lib/python/test.py]
While rewriting the tests, so they store the results in resultsdb, i
found myself doing repetitive work - i needed to write the same code to
__all__ the test files, which is a boring job, which i do not want to do
ever again :)
Fortunately this can be solved by creating a common parent class (lets
call it BaseTest) for all the tests. This class will then implement all
the repetitive code (e.g. sending the email, loading config...), and
only local "specialities" need to be handled in the test.
The main thought behind the BaseTest is that if we setup few common
variables (result, summary, highlights, outputs) as instance variables,
we can then use the postprocess function (which is called after the
run_once successfully [and note the 'successfully' for later] ends).
[note]
This is also a pre-step to switching to ResultsDB as all the changes
required to add basic resultsdb storage is about 7 lines in the BaseTest.
[/note]
[patch 0002]
For the email's sake in current 'send email to results list' model, the
postprocess fuction implements the email-sending. Summary is the subject
and highlights + outputs are body. The self.mail_to list is filled in
the tests which have the "inform package owners" fuctionality.
2) ExceptionCatcher decorator
-----------------------------
[patch 0001 - lib/python/decorators.py]
What the heck is this?!? Well, you remember, that the 'postprocess'
function is called only if the run_once ends well (i.e. no exception is
thrown from the run_once). This, of course, is absolutely not OK, if one
wants to have some common "out" point - e.g. call the postprocess function.
This decorator then watches over the decorated function, and if
exception is raised, the decorator stores it, calls the
'exception_happened' function (which is given as parameter to the
decorator, see the docstring for more detail), and then re-raises the
exception.
This practically gives us the opportunity to call the postprocess
method. In fact, I call "FOOBAR_failed" method, which potentially sets
the result & summary (if not already set) and then calls the postprocess.
Using the decorator has also a small drawback - **kwargs needs to be set
as last parameter of the decorated function. This is because autotest
does some nasty magic with introspection to find out which parameters
the function (initialize, run_once) has and it fails on the decorated
function, so it just blindly uses all the parameters specified in the
control file.
3) Tests & templates rewritten
------------------------------
[both patches]
I also rewrote the tests & templates so they implement this
functionality, I did not have time to test it yet, so it's probable,
that there are some typing errors and stuff, but i think it's important
to show you, how will the new style of test writing work, once this
patch is pushed.
Please comment, ask questions, share ideas - I believe that this is an
important issue and would like all of you to be happy about it.
joza
3:44 p.m.
On Fri, 2010-07-09 at 16:12 +0200, Josef Skladanka wrote:
Hello gang,
while I was playing around with ResultsDB, I've come to some issues,
which bothered me to the level I wrote a fix :)
Heh. I'll need a little more time to review the patches more closely but
I just wanted to say that I really like where this is going - this has
been bothering me as well. Thanks for getting in there and doing
something about it!
-w
3:07 a.m.
I'd be happy to go through the patch with you (maybe over the phone?) if
you'd like.
j.
On 07/09/2010 10:44 PM, Will Woods wrote:
On Fri, 2010-07-09 at 16:12 +0200, Josef Skladanka wrote:
> Hello gang,
>
> while I was playing around with ResultsDB, I've come to some issues,
> which bothered me to the level I wrote a fix :)
Heh. I'll need a little more time to review the patches more closely but
I just wanted to say that I really like where this is going - this has
been bothering me as well. Thanks for getting in there and doing
something about it!
-w
_______________________________________________
autoqa-devel mailing list
autoqa-devel(a)lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/autoqa-devel
2:14 p.m.
On Fri, 2010-07-09 at 16:12 +0200, Josef Skladanka wrote:
Hello gang,
while I was playing around with ResultsDB, I've come to some issues,
which bothered me to the level I wrote a fix :)
1) Common base class for the tests
-----------------------------------
This part is fine - nothing to talk about here, really.
[patch 0002]
For the email's sake in current 'send email to results list' model, the
postprocess fuction implements the email-sending. Summary is the subject
and highlights + outputs are body. The self.mail_to list is filled in
the tests which have the "inform package owners" fuctionality.
This part is fine too.
2) ExceptionCatcher decorator
-----------------------------
[patch 0001 - lib/python/decorators.py]
What the heck is this?!? Well, you remember, that the 'postprocess'
function is called only if the run_once ends well (i.e. no exception is
thrown from the run_once). This, of course, is absolutely not OK, if one
wants to have some common "out" point - e.g. call the postprocess function.
This decorator then watches over the decorated function, and if
exception is raised, the decorator stores it, calls the
'exception_happened' function (which is given as parameter to the
decorator, see the docstring for more detail), and then re-raises the
exception.
I definitely like the decorator idea, but I have some questions about
the implementation. It's interesting that you chose to implement this as
a class, rather than a function - although decorators-with-args are kind
of a brain-buster either way, really. Heh.
As I understand from reading the code, it seems you want to pass a class
method (like 'self.run_once_failed') to trigger if an exception is
raised, which is a good idea. But it seems like you could do this just
by doing, say:
def initialize_failed(self):
...
@ExceptionCatcher(initialize_failed)
def initialize(self, config, **kwargs):
self.config = config_loader(config, self.tmpdir)
...
i.e. pass the function directly, rather than using the tricky
manipulation of string-name-to-callable.
Given that the first argument will be a callable, it seems like you
could define the decorator in the more traditional way, something like
this:
def ExceptionCatcher(on_exception, *oe_args, **oe_kwargs):
'''docs go here'''
def _wrap(f): # this wraps the function we're decorating
def _inner(*args, **kwargs):
exc_info = None
try:
r = f(*args, **kwargs) # decorated function
except:
exc_info = sys.exc_info() # save exc info
finally:
if exc_info is not None:
on_exception(*oe_args, **oe_kwargs)
raise exc_info[1], None, exc_info[2]
return r
return _inner
return _wrap
(see http://www.kylev.com/2009/05/28/a-brief-python-decorator-primer/
for examples of the traditional double-function way of doing decorators
with arguments)
Would something like that work, or would that mess up autotest's
introspection?
3) Tests & templates rewritten
------------------------------
This is (of course) reasonable, given the other patches. We'll want to
test it out a bit to be sure, though.
-w
6:22 a.m.
On 07/13/2010 09:14 PM, Will Woods wrote:
> 2) ExceptionCatcher decorator
> -----------------------------
>
> [patch 0001 - lib/python/decorators.py]
>
> What the heck is this?!? Well, you remember, that the 'postprocess'
> function is called only if the run_once ends well (i.e. no exception is
> thrown from the run_once). This, of course, is absolutely not OK, if one
> wants to have some common "out" point - e.g. call the postprocess
function.
>
> This decorator then watches over the decorated function, and if
> exception is raised, the decorator stores it, calls the
> 'exception_happened' function (which is given as parameter to the
> decorator, see the docstring for more detail), and then re-raises the
> exception.
I definitely like the decorator idea, but I have some questions about
the implementation. It's interesting that you chose to implement this as
a class, rather than a function - although decorators-with-args are kind
of a brain-buster either way, really. Heh.
Well, i like classes more, if I ever find myself in need to adjust
stuff/add functionality, class gives me a better substructure to work
on, so it's a personal preference. Other than that, the
decorator-with-args always seemed to be more 'straightforward' maybe
while one tries to decipher the magic :)
As I understand from reading the code, it seems you want to pass a
class
method (like 'self.run_once_failed') to trigger if an exception is
raised, which is a good idea. But it seems like you could do this just
by doing, say:
def initialize_failed(self):
...
@ExceptionCatcher(initialize_failed)
def initialize(self, config, **kwargs):
self.config = config_loader(config, self.tmpdir)
...
i.e. pass the function directly, rather than using the tricky
manipulation of string-name-to-callable.
Given that the first argument will be a callable, it seems like you
could define the decorator in the more traditional way, something like
this:
def ExceptionCatcher(on_exception, *oe_args, **oe_kwargs):
'''docs go here'''
def _wrap(f): # this wraps the function we're decorating
def _inner(*args, **kwargs):
exc_info = None
try:
r = f(*args, **kwargs) # decorated function
except:
exc_info = sys.exc_info() # save exc info
finally:
if exc_info is not None:
on_exception(*oe_args, **oe_kwargs)
raise exc_info[1], None, exc_info[2]
return r
return _inner
return _wrap
(see http://www.kylev.com/2009/05/28/a-brief-python-decorator-primer/
for examples of the traditional double-function way of doing decorators
with arguments)
Well, I understand what you're saying, but it has one tiny little
problem - you need to get the "self" reference in there. This is not a
big deal, really because you actually have the "self" as a first
parameter *args. But then we would need to decide, that the
"on_exception" method always has to be method of the respective class in
which the decorator was used (i'm not sure if it's clear, so lets shoot
some examples :)
Assumption: the decorated method is a class method, on_exception method
is a class method of the same class as the decorated method:
=====================
def ExceptionCatcher(on_exception, *oe_args, **oe_kwargs):
'''docs go here'''
def _wrap(f): # this wraps the function we're decorating
def _inner(*args, **kwargs):
exc_info = None
try:
r = f(*args, **kwargs) # decorated function
except:
exc_info = sys.exc_info() # save exc info
finally:
if exc_info is not None:
# <--- notice the args[0] ~ 'self'
on_exception(args[0], *oe_args, **oe_kwargs)
raise exc_info[1], None, exc_info[2]
return r
return _inner
return _wrap
##### (1) Only this will work #####
class MyClass(object):
def exc_handler(self):
print "ALL YOUR BASE ARE BELONG TO US!"
@ExceptionCatcher(exc_handler)
def failing(self):
raise Exception("I'm naughty method!")
#### (2) But we won't be ever able to do this ####
from my_awesome_library import my_exc_handler
class MyClass2(object):
@ExceptionCatcher(my_exc_handler)
def failing(self):
raise Exception("I'm naughty method!")
#### (3) Neither this #####
def standalone_catcher():
print "I'm Great!"
@ExceptionCatcher(standalone_catcher)
def foo(bar = 1):
raise Exception("I'd like to use the decorator outside classes")
=================================================
So the question basically is - is the __getattribute__ really so evil,
that we do want the decorator to be very single-targeted? If so, then I
do not really have a problem with the second approach, but it kind of
seemed more reasonable to have a generaly-working decorator at the cost
of maybe more 'hackish' code.
So these are my $.02 on the matter. Looking forward to your reply.
Would something like that work, or would that mess up autotest's
introspection?
Well, the introspection is broken once you use **kwargs somewhere in the
decorator (and decorators can not function without it), so the 'last arg
of the function has to be **kwargs when we decorate inside our tests'
still applies.
joza
5:26 p.m.
On Wed, 2010-07-14 at 13:22 +0200, Josef Skladanka wrote:
Well, I understand what you're saying, but it has one tiny little
problem - you need to get the "self" reference in there. This is not a
big deal, really because you actually have the "self" as a first
parameter *args. But then we would need to decide, that the
"on_exception" method always has to be method of the respective class in
which the decorator was used (i'm not sure if it's clear, so lets shoot
some examples :)
Ahhh, okay. I read your examples (and played around a while) and I
finally understand how this works. Thanks for being patient with me!
Heh.
There's really no way we can define the handler to tell it to use the
instance method, since the object instance doesn't exist - there's no
'self.run_once_failed' until there's a 'self', and there's no
'self'
when you're defining the class.
So now that I understand the advantage of using a class and overriding
__call__(), I think that's a fine (and actually really clever) way to
handle this - so after my one last question below I think we'll be ready
to merge this into master.
> Would something like that work, or would that mess up
autotest's
> introspection?
Well, the introspection is broken once you use **kwargs somewhere in the
decorator (and decorators can not function without it), so the 'last arg
of the function has to be **kwargs when we decorate inside our tests'
still applies.
Interesting. Good to know.
So my last question is this: Should we rename (or subclass) BaseTest to
AutoQATest, or something like that? If plan to start adding
AutoQA-specific stuff to the class (automatic resultdb reporting, for
instance) I think it might be smart to change the name, to keep the
division between our autoqa extensions and the upstream autotest stuff
kind of clear. (Or at least no more muddy than it already is.)
And if we're going to change the class name, we should probably do that
before the merge. But maybe I'm just being nitpicky and we don't need to
worry about this. What do you think?
-w
1:57 a.m.
----- "Will Woods" <wwoods(a)redhat.com> wrote:
On Wed, 2010-07-14 at 13:22 +0200, Josef Skladanka wrote:
Ahhh, okay. I read your examples (and played around a while) and I
finally understand how this works. Thanks for being patient with me!
Heh.
Oh, no need to thank me, I know, that this piece of code is a killer :)
Thank you for reviewing it so fast!
There's really no way we can define the handler to tell it to use
the
instance method, since the object instance doesn't exist - there's no
'self.run_once_failed' until there's a 'self', and there's no
'self'
when you're defining the class.
So now that I understand the advantage of using a class and
overriding
__call__(), I think that's a fine (and actually really clever) way to
handle this - so after my one last question below I think we'll be
ready
to merge this into master.
/me blushes
> > Would something like that work, or would that mess up
autotest's
> > introspection?
>
> Well, the introspection is broken once you use **kwargs somewhere in
the
> decorator (and decorators can not function without it), so the 'last
arg
> of the function has to be **kwargs when we decorate inside our
tests'
> still applies.
Interesting. Good to know.
Yes, it's because the whole method, which is 'handed' to autotest is
already decorated. The autotest introspection looks at the method,
and extracts the names of keyword arguments using some nasty magic
(if only i could remember where i found it, so i could give you
the pointer... hate my selective memory).
And because the decorated method actually has only *args, **kwargs
as parameters, it has no way to decipher it. I also tried to
re-run the introspecting code inside the decorator, but it seems,
that the call-stack inside autotest is a bit more complicated,
than one would anticipate, so after few hours of failing to repeat
magic, i decided to go KISS and just add the **kwargs parameter...
So my last question is this: Should we rename (or subclass) BaseTest
to
AutoQATest, or something like that? If plan to start adding
AutoQA-specific stuff to the class (automatic resultdb reporting, for
instance) I think it might be smart to change the name, to keep the
division between our autoqa extensions and the upstream autotest
stuff
kind of clear. (Or at least no more muddy than it already is.)
And if we're going to change the class name, we should probably do
that
before the merge. But maybe I'm just being nitpicky and we don't need
to
worry about this. What do you think?
-w
Yes, I'm deffinitely for the idea of renaming it to AutoQATest. I'm not
really good with naming stuff, so the BaseTest was more of a concept
name. And yes, lets do that before the merge, it would be unreasonable
to do it later.
The only drawback is, that i'm leaving in few hours and will be OOO
up until monday's evening, so if you could rename it, and push the first
patch to master, that would be great! And i'll test the second patch
more thoroughly during tuesday, and provide patch.
Does this seem reasonable?
Joza
5044
days inactive
5050
days old
autoqa-devel@lists.fedorahosted.org
6 comments
2 participants
participants (2)
-
Josef Skladanka -
Will Woods