[apparmor] [patch 2/2] add tests for RE_PROFILE_START_2 and parse_profile_start_line()

Steve Beattie steve at nxnw.org
Tue Mar 31 22:46:12 UTC 2015


Bah, sent before completing my thoughts...

On Tue, Mar 31, 2015 at 03:32:00PM -0700, Steve Beattie wrote:
> On Wed, Mar 04, 2015 at 10:41:10PM +0100, Christian Boltz wrote:
> > this patch add tests for RE_PROFILE_START_2 and 
> > parse_profile_start_line().
> > 
> > It also adds AANamedRegexTest class that can be used to test a regex
> > with named match groups.
> > 
> > I propose this patch for trunk and 2.9.
> > 
> > 
> > [ test-re_profile_start_2-and-parse_profile_start_line.diff ]
> 
> I'm mostly okay with this patch, just a couple of comments below.
> 
> I will say again, this is one of those situations where I'd like
> nose to be able to detect generative tests like these, as the ones
> added here don't add to the overall test coverage (despite the
> test themselves adding value), but I'd like to be able to use nose
> to generate coverage information on just the added test classes,
> so that I can see if there's any cases that are missed.
> 
> > --- .utils/test/test-regex_matches.py 2015-03-04 22:00:48.857858576 +0100
> > +++ utils/test/test-regex_matches.py    2015-03-04 22:16:48.509700824 +0100
> > @@ -13,13 +13,38 @@ import apparmor.aa as aa
> >  import unittest
> >  import sys
> >  from common_test import AATest, setup_all_tests
> > +from apparmor.common import AppArmorBug
> > +
> > +from apparmor.regex import strip_quotes, parse_profile_start_line, RE_PROFILE_START_2
> >  
> > -from apparmor.regex import strip_quotes
> >  
> >  class AARegexTest(AATest):
> >      def _run_test(self, params, expected):
> >          return regex_test(self, params, expected)
> >  
> > +class AANamedRegexTest(AATest):
> > +    def _run_test(self, line, expected):
> > +        '''Run a line through self.regex.search() and verify the result
> > +
> > +        Keyword arguments:
> > +        line -- the line to search
> > +        expected -- False if the search isn't expected to match or, if the search
> > +                    is expected to match, a tuple of expected match groups.
> > +        '''
> > +        matches = self.regex.search(line)
> > +        if not expected:
> > +            self.assertFalse(matches)
> > +            return
> > +
> > +        self.assertTrue(matches)
> > +
> > +        for exp in expected:
> > +            match = matches.group(exp)
> > +            if match:
> > +                match = match
> > +            self.assertEqual(match, expected[exp], 'Group %s mismatch in rule %s' % (exp,line))
> > +
> > +
> >  
> >  class AARegexHasComma(AATest):
> >      '''Tests for apparmor.aa.RE_RULE_HAS_COMMA'''
> > @@ -368,6 +393,77 @@ class AARegexUnix(AARegexTest):
> >          ('deny unixlike,', False),
> >      ]
> >  
> > +class AANamedRegexProfileStart_2(AANamedRegexTest):
> > +    '''Tests for RE_PROFILE_START_2'''
> > +
> > +    def setUp(self):
> > +        self.regex = RE_PROFILE_START_2
> > +
> > +    tests = [
> > +        ('/bin/foo ', False), # no '{'
> > +        ('/bin/foo /bin/bar', False), # missing 'profile' keyword
> > +        ('profile {', False), # no attachment
> > +        ('   profile foo bar /foo {', False), # missing quotes around "foo bar"
> > +
> > +        ('   /foo {',                     { 'plainprofile':         '/foo',         'namedprofile': None,          'attachment': None,     'flags': None,       'comment': None }),
> > +        ('   "/foo" {',                   { 'plainprofile':         '"/foo"',       'namedprofile': None,          'attachment': None,     'flags': None,       'comment': None }),
> > +        ('   profile /foo {',             { 'plainprofile':         None,           'namedprofile': '/foo',        'attachment': None,     'flags': None,       'comment': None }),
> > +        ('   profile "/foo" {',           { 'plainprofile':         None,           'namedprofile': '"/foo"',      'attachment': None,     'flags': None,       'comment': None }),
> > +        ('   profile foo /foo {',         { 'plainprofile':         None,           'namedprofile': 'foo',         'attachment': '/foo',   'flags': None,       'comment': None }),
> > +        ('   profile foo /foo (audit) {', { 'plainprofile':         None,           'namedprofile': 'foo',         'attachment': '/foo',   'flags': 'audit',    'comment': None }),
> > +        ('   profile "foo" "/foo" {',     { 'plainprofile':         None,           'namedprofile': '"foo"',       'attachment': '"/foo"', 'flags': None,       'comment': None }),
> > +        ('   profile "foo bar" /foo {',   { 'plainprofile':         None,           'namedprofile': '"foo bar"',   'attachment': '/foo',   'flags': None,       'comment': None }),
> > +        ('   /foo (complain) {',          { 'plainprofile':         '/foo',         'namedprofile': None,          'attachment': None,     'flags': 'complain', 'comment': None }),
> > +        ('   /foo flags=(complain) {',    { 'plainprofile':         '/foo',         'namedprofile': None,          'attachment': None,     'flags': 'complain', 'comment': None }),
> > +        ('   /foo (complain) { # x',      { 'plainprofile':         '/foo',         'namedprofile': None,          'attachment': None,     'flags': 'complain', 'comment': '# x'}),
> 
> Can we be a little horizontally space efficient here? I don't think the
> space after 'plainprofile': adds anything, and perhaps can we bring the
> expected result down to a slightly indented newline? I don't expect
> everything to fit into 80 columns, but going over 130-140 starts to get
> a bit obnoxious.
> 
> > +
> > +        ('   /foo {',                     { 'plainprofile':         '/foo', 'leadingspace': '   ' }),
> > +        ('/foo {',                        { 'plainprofile':         '/foo', 'leadingspace': '' }),
> 
> I was initially confused that the prior tests didn't require
> 'leadingspace' fields. You might want to add leading space tests where
> 'namedprofile' is not None, too.
> 
> Both comments apply to the tests below, too.
> 
> With these addressed 

With the comments here addressed, Acked-by: Steve Beattie <steve at nxnw.org>.
Thanks.

> > +    ]
> > +
> > +
> > +class Test_parse_profile_start_line(AATest):
> > +    tests = [
> > +        ('   /foo {',                     { 'profile': '/foo',    'profile_keyword': False, 'plainprofile': '/foo', 'namedprofile': None,          'attachment': None,     'flags': None,       'comment': None }),
> > +        ('   "/foo" {',                   { 'profile': '/foo',    'profile_keyword': False, 'plainprofile': '/foo', 'namedprofile': None,          'attachment': None,     'flags': None,       'comment': None }),
> > +        ('   profile /foo {',             { 'profile': '/foo',    'profile_keyword': True,  'plainprofile': None,   'namedprofile': '/foo',        'attachment': None,     'flags': None,       'comment': None }),
> > +        ('   profile "/foo" {',           { 'profile': '/foo',    'profile_keyword': True,  'plainprofile': None,   'namedprofile': '/foo',        'attachment': None,     'flags': None,       'comment': None }),
> > +        ('   profile foo /foo {',         { 'profile': 'foo /foo','profile_keyword': True,  'plainprofile': None,   'namedprofile': 'foo',         'attachment': '/foo',   'flags': None,       'comment': None }), # XXX
> > +        ('   profile foo /foo (audit) {', { 'profile': 'foo /foo','profile_keyword': True,  'plainprofile': None,   'namedprofile': 'foo',         'attachment': '/foo',   'flags': 'audit',    'comment': None }), # XXX
> > +        ('   profile "foo" "/foo" {',     { 'profile': 'foo /foo','profile_keyword': True,  'plainprofile': None,   'namedprofile': 'foo',         'attachment': '/foo',   'flags': None,       'comment': None }), # XXX
> > +        ('   profile "foo bar" /foo {',   { 'profile': 'foo bar /foo', 'profile_keyword': True,  'plainprofile': None,   'namedprofile': 'foo bar',     'attachment': '/foo',   'flags': None,       'comment': None }), # XXX
> > +        # XXX lines marked with XXX include the "broken" behaviour for 'profile' - they need to be changed when attachment is handled correctly
> > +        ('   /foo (complain) {',          { 'profile': '/foo',    'profile_keyword': False, 'plainprofile': '/foo', 'namedprofile': None,          'attachment': None,     'flags': 'complain', 'comment': None }),
> > +        ('   /foo flags=(complain) {',    { 'profile': '/foo',    'profile_keyword': False, 'plainprofile': '/foo', 'namedprofile': None,          'attachment': None,     'flags': 'complain', 'comment': None }),
> > +        ('   /foo (complain) { # x',      { 'profile': '/foo',    'profile_keyword': False, 'plainprofile': '/foo', 'namedprofile': None,          'attachment': None,     'flags': 'complain', 'comment': '# x'}),
> > +        ('   /foo {',                     { 'profile': '/foo',    'plainprofile': '/foo', 'leadingspace': '   ' }),
> > +        ('/foo {',                        { 'profile': '/foo',    'plainprofile': '/foo', 'leadingspace': None }),
> > +    ]
> > +
> > +    def _run_test(self, line, expected):
> > +        matches = parse_profile_start_line(line, 'somefile')
> > +
> > +        self.assertTrue(matches)
> > +
> > +        for exp in expected:
> > +            self.assertEqual(matches[exp], expected[exp], 'Group %s mismatch in rule %s' % (exp,line))
> > +
> > +class TestInvalid_parse_profile_start_line(AATest):
> > +    tests = [
> > +        ('/bin/foo ', False), # no '{'
> > +        ('/bin/foo /bin/bar', False), # missing 'profile' keyword
> > +        ('profile {', False), # no attachment
> > +        ('   profile foo bar /foo {', False), # missing quotes around "foo bar"
> > +    ]
> > +
> > +    def _run_test(self, line, expected):
> > +        with self.assertRaises(AppArmorBug):
> > +            parse_profile_start_line(line, 'somefile')
> > +
> > +
> > +
> > +
> 
> We probably don't need this much whitespace between classes here.
> 
> >  class TestStripQuotes(AATest):
> >      def test_strip_quotes_01(self):
> >          self.assertEqual('foo', strip_quotes('foo'))
> 
> -- 
> Steve Beattie
> <sbeattie at ubuntu.com>
> http://NxNW.org/~steve/



> -- 
> AppArmor mailing list
> AppArmor at lists.ubuntu.com
> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor


-- 
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: 819 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20150331/ee08839f/attachment-0001.pgp>


More information about the AppArmor mailing list