[apparmor] [patch 05/13] parser - rewrite caching tests in python unittest

[Christian Boltz]

Hello,

Am Donnerstag, 10. Oktober 2013 schrieb Steve Beattie:
> On Fri, Oct 11, 2013 at 01:36:59AM +0200, Christian Boltz wrote:
> > [sorry for the slightly broken quoting - KMail needs some
> > improvement
> > when quoting overlong lines ;-) ]
> 
> No worries. I'm glad to see that the GSoC experience has caused you to
> be less hesitant about reviewing python code. :)

We'll see if you still like this in some months... (and I wonder if 
Kshitij smiled now that I have a new victim for my reviews ;-)

> > Am Donnerstag, 10. Oktober 2013 schrieb Steve Beattie:
> > > --- /dev/null
> > > +++ b/parser/tst/caching.py
> > > 
> > > +from optparse import OptionParser    # deprecated, should move to
> > > argparse eventually
> > 
> > I love it when a patch introduces a new file that already comes with
> > a "deprecated" notice. What about just switching to argparse? ;-)
> Welcome to python, where just when you get used to something, it
> gets deprecated for something "better". Yes, python developers have
> deprecated optparse for argparse... in python 3.3 only. I'm continuing
> to use optparse in situations where it's useful to continue support
> for python 2.7, where it's not deprecated (well, unless you consider
> all of python 2.7 deprecated, which to an extent, it is :/ ).

> I put the comment in as a reminder that there's a future cost to pay
> for supporting python 2.7. I guess I could expand the comment where
> I have it to clarify.

argparse already works with python 2.7 (Kshitij's code is using it, and 
I regularly test it with py 2.7 and 3.3), so there's no reason to stay 
with something that is deprecated ;-)

> > > +    def test_cache_not_loaded_when_features_differ(self):
> > > +        '''test cache is not loaded when features file differs'''
> > > +
> > > +        self._generate_cache_file()
> > > +
> > > +        with open(os.path.join(self.cache_dir, '.features'),
> > > 'w+') as> > 
> > >     f:
> > > +            f.write('monkey\n')
> > 
> > It seems you have lots of bananas ;-) - adding the monkeys to the
> > features file is also worth its own function IMHO.
> > 
> > An even better alternative is to add an optional "monkey" parameter
> > to _generate_cache_file()
> 
> The _generate_cache_file was me recognizing that I was writing the
> same set of code (to setup and invoke the parser) over and over again
> to get into the state where the cache directory had been populated.

And you didn't notice that you need monkeys quite often? ;-)

I usually follow this rules of thumb:
- if I need at least 5 lines a second time, I check if it's worth to 
  move them to a function
- if I need at least 2 lines more than 5 times, I also consider moving 
  them to a function

You can vary the numbers a bit, and there's still a difference between 
"consider" and "do it", but you should get my point ;-)

> > After reading the second half of the patch, I noticed that you also
> > add monkeys to other files, so maybe (untested!)
> > 
> >     def add_monkey(filename):
> >         banana = os.path.join(self.cache_dir, filename)
> >         
> >         with open(banana, 'w+') as f:
> >             f.write('monkey')
> > 
> > would be another good solution.
> 
> Yeah, I'll think about this a bit, to determine what helper
> function(s) I want to add to simplify it a bit.

I tend to the add_monkey function, because that makes the testcase 
easier to understand than adding a "true" parameter to 
_generate_cache_file.

Maybe it would be even easier with "break_file" instead of "add_monkey" 
as function name. OTOH, I'm quite sure people _will_ check what 
add_monkey() does, but nobody will read the code of break_file() ;-)


> Though, what I'd really like is to somehow set self.do_cleanup to
> False when any test fails, so that for test cases that fail, the
> temporary directory is left behind, to make diagnosing why it failed
> easier to do. I'll think about whether there's a reasonable way to do
> that.

Looks like it isn't really nice or easy, but at least possible:

http://stackoverflow.com/questions/4414234/getting-pythons-unittest-results-in-a-teardown-method

http://www.piware.de/2012/10/python-unittest-show-log-on-test-failure/ 
(the comments are also interesting)


Regards,

Christian Boltz
-- 
Ich kriege Druck von beiden Seiten. Den technisch Bewanderten ist
man immer der "Agenturheini, kein Ahnung, Hauptsache bunt" - und für
die Kreativen ist man immer "Der Langweiler, der am liebsten alles
SchwarzWeissTimesNew runterrasseln" würde. Beides macht das Web nicht
besser. [Ratti in suse-linux]