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

[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. :)

> 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.

> Lots of tests (all?) have
> 
> > +        rc, report = testlib.run_cmd(cmd)
> > +        self.assertEquals(rc, 0, "Got return code %d, expected
> > 0\nOutput: %s" % (rc, report)) 
> 
> testlib.run_cmd should have a parameter expected_returncode (default 0) 
> and fail the test if it doesn't match.

Yep, this is a sensible improvement.

> > +    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.

> 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.

> > +    def test_cache_writing_updates_cache_file(self):
> ...
> > +        with open(cache_file, 'rb') as f:
> > +            cache_contents = f.read()
> > +            new_size = os.fstat(f.fileno()).st_size
> > +        # We check sizes here rather than whether the string monkey
> > is +        # in cache_contents because of the difficulty coercing
> > cache +        # file bytes into strings in python3
> > +        self.assertNotEquals(orig_size, new_size, 'Expected cache
> > file to be updated, got: \n%s' % cache_contents) 
> 
> Wait a minute - first you are afraid of diffing the file content because 
> of handling binary stuff in python, and then you dump cache_contents 
> (which contains the binary cache of the profile) to the terminal if the 
> test fails.
> 
> I'm quite sure nobody wants to have his terminal filled up with binary 
> data, and I'm also sure nobody can read the binary dump without using 
> tools.
>
> Instead, you should print both file sizes or just "file size
> differs".

Well, it's pretty unlikely that we'd have a cache file that was the
same size as a file containing the string "monkey\n", so the theory was
that if the sizes were the same, then the most reasonable explanation
was that the cache file still contained the same string contents. But
it's a fair point, I'll fix it.

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.

> This is the only critical thing - everything else is "just" cleanup 
> which can go into a follow-up patch.
> 
> With dumping the binary stuff to the terminal removed, and a promise to 
> do the cleanups in a follow-up patch [1],
>     Acked-by: Christian Boltz <apparmor at cboltz.de>

Thanks for the review!

-- 
Steve Beattie
<sbeattie at ubuntu.com>
http://NxNW.org/~steve/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20131010/447c6923/attachment-0001.pgp>