[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