[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:32:00 UTC 2015
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
> + ]
> +
> +
> +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/
-------------- 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/fe19fdaf/attachment.pgp>
More information about the AppArmor
mailing list