On March 21st, 2014, 12:11 p.m. UTC, Michal Minar wrote:

src/account/test/TestAccountInvalidEtc.py (Diff revision 1)
		395	class TestAccountInvalidEtc(AccountBase):

IMHO there is a lot of redundancy in this file. Having 50+ methods looking alike is very tedious to read (and actually no one bothers). Moreover I certainly would not like to write them. If you really need to have one method for each particular mini-test-case, try to generate them dynamically:

def gen_cripple_test(file_name, test_info):
    """
    Generator of test methods. Each call to this function adds
    one test method to TestAccountInvalidEtc.

    :param string file_name: File to cripple.
    :param test_info: Either a string identifying test or a pair
        ``(string_identifying_test, append)``.
    """
    if isinstance(test_info, tuple):
        test_name, append = test_info
    else:
        test_name, append = test_info, False
    kwargs = {}
    if append:
        kwargs['op'] = '+'
    def _new_cripple_test(self):
        self.crippler.cripple(file_name, test_id, **kwargs)
        self.assertSane(self)
    meth_name = 'test_' + ('append_' if append else '') + name
    _new_cripple_test.__name__ = meth_name
    TestAccountInvalidEtc[meth_name] = _new_cripple_test

# Tests for /etc/group file
for test_info in (
        'empty_fields',
        'one_field',
        'less_fields',
        # ...
        ('empty', True),
        ('root', True),
        # ...
        ):
    gen_cripple_test('/etc/group', test_info)

# Tests for /etc/passwd file
for test_info in (
        'empty_fields',
        'one_field',
        'less_fields',
        # ...
        ('empty', True),
        ('root', True),
        # ...
        ):
    gen_cripple_test('/etc/passwd', test_info)



And if you expose cases of PasswdCrippler class statically, it would get even shorter:

for test_file, cases in PasswdCrippler.CASES.items():
    for test_info in cases:  # cases is a dictionary
        gen_cripple_test(test_file, test_info)

You would also need another loop for *append* methods. IMHO 2000+ lines reduced to 30 is worth it.

Thanks= you for your feedback, I rewrote the code to add methods dynamically, in a similar way as you posted.  ...is it better?

- Alois

On March 24th, 2014, 3:16 p.m. UTC, Alois Mahdal wrote:

account: Add tests for invalid system files

Each test creates an instance it LMI_Account, then intentionally
cripples one of system files related to users/groups, and performs
a simple sanity test that checks if instance properties are readable.