----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/1559/ -----------------------------------------------------------
Review request for OpenLMI Developers.
Summary (updated) -----------------
Add tests for invalid system files
Repository: openlmi-providers
Description (updated) -------
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.
Diffs (updated) -----
src/account/test/TestAccountInvalidEtc.py PRE-CREATION src/account/test/common.py 2b3b423e125dad81dc8ed3385be15ebc4f785228 src/account/test/generate_cases.py PRE-CREATION
Diff: http://reviewboard-openlmi.rhcloud.com/r/1559/diff/
Testing -------
Thanks,
Alois Mahdal
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/1559/#review2359 -----------------------------------------------------------
src/account/test/TestAccountInvalidEtc.py http://reviewboard-openlmi.rhcloud.com/r/1559/#comment1255
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.
src/account/test/generate_cases.py http://reviewboard-openlmi.rhcloud.com/r/1559/#comment1256
Use dynamic generation of test methods (viz command above).
- Michal Minar
On March 20, 2014, 4:58 p.m., Alois Mahdal wrote:
This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/1559/
(Updated March 20, 2014, 4:58 p.m.)
Review request for OpenLMI Developers.
Repository: openlmi-providers
Description
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.
Diffs
src/account/test/TestAccountInvalidEtc.py PRE-CREATION src/account/test/common.py 2b3b423e125dad81dc8ed3385be15ebc4f785228 src/account/test/generate_cases.py PRE-CREATION
Diff: http://reviewboard-openlmi.rhcloud.com/r/1559/diff/
Testing
Thanks,
Alois Mahdal
On March 21, 2014, 12:11 p.m., Michal Minar wrote:
src/account/test/TestAccountInvalidEtc.py, line 395 http://reviewboard-openlmi.rhcloud.com/r/1559/diff/1/?file=8628#file8628line395
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
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/1559/#review2359 -----------------------------------------------------------
On March 24, 2014, 3:16 p.m., Alois Mahdal wrote:
This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/1559/
(Updated March 24, 2014, 3:16 p.m.)
Review request for OpenLMI Developers.
Repository: openlmi-providers
Description
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.
Diffs
src/account/test/TestAccountInvalidEtc.py PRE-CREATION src/account/test/common.py 2b3b423e125dad81dc8ed3385be15ebc4f785228
Diff: http://reviewboard-openlmi.rhcloud.com/r/1559/diff/
Testing
Thanks,
Alois Mahdal
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/1559/ -----------------------------------------------------------
(Updated March 24, 2014, 3:16 p.m.)
Review request for OpenLMI Developers.
Changes -------
Now the test module defines test methods dynamically instead of generating and appending source code.
Repository: openlmi-providers
Description -------
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.
Diffs (updated) -----
src/account/test/TestAccountInvalidEtc.py PRE-CREATION src/account/test/common.py 2b3b423e125dad81dc8ed3385be15ebc4f785228
Diff: http://reviewboard-openlmi.rhcloud.com/r/1559/diff/
Testing -------
Thanks,
Alois Mahdal
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/1559/#review2368 -----------------------------------------------------------
Ship it!
Nice! Thanks for your work.
- Michal Minar
On March 24, 2014, 3:16 p.m., Alois Mahdal wrote:
This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/1559/
(Updated March 24, 2014, 3:16 p.m.)
Review request for OpenLMI Developers.
Repository: openlmi-providers
Description
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.
Diffs
src/account/test/TestAccountInvalidEtc.py PRE-CREATION src/account/test/common.py 2b3b423e125dad81dc8ed3385be15ebc4f785228
Diff: http://reviewboard-openlmi.rhcloud.com/r/1559/diff/
Testing
Thanks,
Alois Mahdal
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/1559/ -----------------------------------------------------------
(Updated May 13, 2014, 1:09 p.m.)
Status ------
This change has been marked as submitted.
Review request for OpenLMI Developers.
Repository: openlmi-providers
Description -------
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.
Diffs -----
src/account/test/TestAccountInvalidEtc.py PRE-CREATION src/account/test/common.py 2b3b423e125dad81dc8ed3385be15ebc4f785228
Diff: http://reviewboard-openlmi.rhcloud.com/r/1559/diff/
Testing -------
Thanks,
Alois Mahdal
openlmi-reviews@lists.fedorahosted.org