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

[Christian Boltz]

Hello,

[sorry for the slightly broken quoting - KMail needs some improvement 
when quoting overlong lines ;-) ]

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? ;-)


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.

If you don't want to mix execution and result testing in one function, a 
separate function run_cmd_checkstatus() that calls testlib.run_cmd() and 
checks the return code also works. 

Even if it only saves one line per test - you have lots of tests ;-) 
It would remove some noise from all tests and makes sure the returncode 
is checked in every test. (With the current way, I wouldn't be too 
surprised if one of the tests forgets to check the returncode.)

> +    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()

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.

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


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>


Regards,

Christian Boltz

[1] you know I'm good at reminding you ;-)
-- 
Ugly doesn't even begin to describe the knoppix init script system. [..]
Some people should just be strung up by their short hairs and made to
walk in the steps of those who must follow them before being allowed to
code such monstrosities again.   [Robison, Jonathon (M.)]