From cfd607e43da4a20753744f134e201310262b827a Mon Sep 17 00:00:00 2001 From: Daniel Latypov Date: Wed, 2 Dec 2020 11:08:21 -0800 Subject: [PATCH 01/12] kunit: tool: fix unit test cleanup handling * Stop leaking file objects. * Use self.addCleanup() to ensure we call cleanup functions even if setUp() fails. * use mock.patch.stopall instead of more error-prone manual approach Signed-off-by: Daniel Latypov Reviewed-by: David Gow Tested-by: Brendan Higgins Acked-by: Brendan Higgins Signed-off-by: Shuah Khan --- tools/testing/kunit/kunit_tool_test.py | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py index b593f4448e839..9a036e9d44554 100755 --- a/tools/testing/kunit/kunit_tool_test.py +++ b/tools/testing/kunit/kunit_tool_test.py @@ -288,19 +288,17 @@ def __eq__(self, other): class KUnitMainTest(unittest.TestCase): def setUp(self): path = get_absolute_path('test_data/test_is_test_passed-all_passed.log') - file = open(path) - all_passed_log = file.readlines() - self.print_patch = mock.patch('builtins.print') - self.print_mock = self.print_patch.start() + with open(path) as file: + all_passed_log = file.readlines() + + self.print_mock = mock.patch('builtins.print').start() + self.addCleanup(mock.patch.stopall) + self.linux_source_mock = mock.Mock() self.linux_source_mock.build_reconfig = mock.Mock(return_value=True) self.linux_source_mock.build_um_kernel = mock.Mock(return_value=True) self.linux_source_mock.run_kernel = mock.Mock(return_value=all_passed_log) - def tearDown(self): - self.print_patch.stop() - pass - def test_config_passes_args_pass(self): kunit.main(['config', '--build_dir=.kunit'], self.linux_source_mock) assert self.linux_source_mock.build_reconfig.call_count == 1 From 0b3e68076bb9a8e1b1bd448994b9c57828173d8e Mon Sep 17 00:00:00 2001 From: Daniel Latypov Date: Wed, 2 Dec 2020 11:08:22 -0800 Subject: [PATCH 02/12] kunit: tool: stop using bare asserts in unit test Use self.assertEqual/assertNotEqual() instead. Besides being more appropriate in a unit test, it'll also give a better error message by show the unexpected values. Also * Delete redundant check of exception types. self.assertRaises does this. * s/kall/call. There's no reason to name it this way. * This is probably a misunderstanding from the docs which uses it since `mock.call` is in scope as `call`. Signed-off-by: Daniel Latypov Reviewed-by: David Gow Tested-by: Brendan Higgins Acked-by: Brendan Higgins Signed-off-by: Shuah Khan --- tools/testing/kunit/kunit_tool_test.py | 50 +++++++++++++------------- 1 file changed, 24 insertions(+), 26 deletions(-) diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py index 9a036e9d44554..72f6b4bb1b5a7 100755 --- a/tools/testing/kunit/kunit_tool_test.py +++ b/tools/testing/kunit/kunit_tool_test.py @@ -301,26 +301,26 @@ def setUp(self): def test_config_passes_args_pass(self): kunit.main(['config', '--build_dir=.kunit'], self.linux_source_mock) - assert self.linux_source_mock.build_reconfig.call_count == 1 - assert self.linux_source_mock.run_kernel.call_count == 0 + self.assertEqual(self.linux_source_mock.build_reconfig.call_count, 1) + self.assertEqual(self.linux_source_mock.run_kernel.call_count, 0) def test_build_passes_args_pass(self): kunit.main(['build'], self.linux_source_mock) - assert self.linux_source_mock.build_reconfig.call_count == 0 + self.assertEqual(self.linux_source_mock.build_reconfig.call_count, 0) self.linux_source_mock.build_um_kernel.assert_called_once_with(False, 8, '.kunit', None) - assert self.linux_source_mock.run_kernel.call_count == 0 + self.assertEqual(self.linux_source_mock.run_kernel.call_count, 0) def test_exec_passes_args_pass(self): kunit.main(['exec'], self.linux_source_mock) - assert self.linux_source_mock.build_reconfig.call_count == 0 - assert self.linux_source_mock.run_kernel.call_count == 1 + self.assertEqual(self.linux_source_mock.build_reconfig.call_count, 0) + self.assertEqual(self.linux_source_mock.run_kernel.call_count, 1) self.linux_source_mock.run_kernel.assert_called_once_with(build_dir='.kunit', timeout=300) self.print_mock.assert_any_call(StrContains('Testing complete.')) def test_run_passes_args_pass(self): kunit.main(['run'], self.linux_source_mock) - assert self.linux_source_mock.build_reconfig.call_count == 1 - assert self.linux_source_mock.run_kernel.call_count == 1 + self.assertEqual(self.linux_source_mock.build_reconfig.call_count, 1) + self.assertEqual(self.linux_source_mock.run_kernel.call_count, 1) self.linux_source_mock.run_kernel.assert_called_once_with( build_dir='.kunit', timeout=300) self.print_mock.assert_any_call(StrContains('Testing complete.')) @@ -329,35 +329,33 @@ def test_exec_passes_args_fail(self): self.linux_source_mock.run_kernel = mock.Mock(return_value=[]) with self.assertRaises(SystemExit) as e: kunit.main(['exec'], self.linux_source_mock) - assert type(e.exception) == SystemExit - assert e.exception.code == 1 + self.assertEqual(e.exception.code, 1) def test_run_passes_args_fail(self): self.linux_source_mock.run_kernel = mock.Mock(return_value=[]) with self.assertRaises(SystemExit) as e: kunit.main(['run'], self.linux_source_mock) - assert type(e.exception) == SystemExit - assert e.exception.code == 1 - assert self.linux_source_mock.build_reconfig.call_count == 1 - assert self.linux_source_mock.run_kernel.call_count == 1 + self.assertEqual(e.exception.code, 1) + self.assertEqual(self.linux_source_mock.build_reconfig.call_count, 1) + self.assertEqual(self.linux_source_mock.run_kernel.call_count, 1) self.print_mock.assert_any_call(StrContains(' 0 tests run')) def test_exec_raw_output(self): self.linux_source_mock.run_kernel = mock.Mock(return_value=[]) kunit.main(['exec', '--raw_output'], self.linux_source_mock) - assert self.linux_source_mock.run_kernel.call_count == 1 - for kall in self.print_mock.call_args_list: - assert kall != mock.call(StrContains('Testing complete.')) - assert kall != mock.call(StrContains(' 0 tests run')) + self.assertEqual(self.linux_source_mock.run_kernel.call_count, 1) + for call in self.print_mock.call_args_list: + self.assertNotEqual(call, mock.call(StrContains('Testing complete.'))) + self.assertNotEqual(call, mock.call(StrContains(' 0 tests run'))) def test_run_raw_output(self): self.linux_source_mock.run_kernel = mock.Mock(return_value=[]) kunit.main(['run', '--raw_output'], self.linux_source_mock) - assert self.linux_source_mock.build_reconfig.call_count == 1 - assert self.linux_source_mock.run_kernel.call_count == 1 - for kall in self.print_mock.call_args_list: - assert kall != mock.call(StrContains('Testing complete.')) - assert kall != mock.call(StrContains(' 0 tests run')) + self.assertEqual(self.linux_source_mock.build_reconfig.call_count, 1) + self.assertEqual(self.linux_source_mock.run_kernel.call_count, 1) + for call in self.print_mock.call_args_list: + self.assertNotEqual(call, mock.call(StrContains('Testing complete.'))) + self.assertNotEqual(call, mock.call(StrContains(' 0 tests run'))) def test_exec_timeout(self): timeout = 3453 @@ -368,7 +366,7 @@ def test_exec_timeout(self): def test_run_timeout(self): timeout = 3453 kunit.main(['run', '--timeout', str(timeout)], self.linux_source_mock) - assert self.linux_source_mock.build_reconfig.call_count == 1 + self.assertEqual(self.linux_source_mock.build_reconfig.call_count, 1) self.linux_source_mock.run_kernel.assert_called_once_with( build_dir='.kunit', timeout=timeout) self.print_mock.assert_any_call(StrContains('Testing complete.')) @@ -376,7 +374,7 @@ def test_run_timeout(self): def test_run_builddir(self): build_dir = '.kunit' kunit.main(['run', '--build_dir=.kunit'], self.linux_source_mock) - assert self.linux_source_mock.build_reconfig.call_count == 1 + self.assertEqual(self.linux_source_mock.build_reconfig.call_count, 1) self.linux_source_mock.run_kernel.assert_called_once_with( build_dir=build_dir, timeout=300) self.print_mock.assert_any_call(StrContains('Testing complete.')) @@ -384,7 +382,7 @@ def test_run_builddir(self): def test_config_builddir(self): build_dir = '.kunit' kunit.main(['config', '--build_dir', build_dir], self.linux_source_mock) - assert self.linux_source_mock.build_reconfig.call_count == 1 + self.assertEqual(self.linux_source_mock.build_reconfig.call_count, 1) def test_build_builddir(self): build_dir = '.kunit' From a3ece0795b9ab234ff196e74606fdef9f463ec5a Mon Sep 17 00:00:00 2001 From: Daniel Latypov Date: Wed, 2 Dec 2020 11:08:23 -0800 Subject: [PATCH 03/12] kunit: tool: use `with open()` in unit test The use of manual open() and .close() calls seems to be an attempt to keep the contents in scope. But Python doesn't restrict variables like that, so we can introduce new variables inside of a `with` and use them outside. Do so to make the code more Pythonic. Signed-off-by: Daniel Latypov Reviewed-by: David Gow Tested-by: Brendan Higgins Acked-by: Brendan Higgins Signed-off-by: Shuah Khan --- tools/testing/kunit/kunit_tool_test.py | 33 +++++++++++--------------- 1 file changed, 14 insertions(+), 19 deletions(-) diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py index 72f6b4bb1b5a7..9a36936749f0d 100755 --- a/tools/testing/kunit/kunit_tool_test.py +++ b/tools/testing/kunit/kunit_tool_test.py @@ -100,15 +100,14 @@ def assertContains(self, needle, haystack): def test_output_isolated_correctly(self): log_path = get_absolute_path( 'test_data/test_output_isolated_correctly.log') - file = open(log_path) - result = kunit_parser.isolate_kunit_output(file.readlines()) + with open(log_path) as file: + result = kunit_parser.isolate_kunit_output(file.readlines()) self.assertContains('TAP version 14', result) self.assertContains(' # Subtest: example', result) self.assertContains(' 1..2', result) self.assertContains(' ok 1 - example_simple_test', result) self.assertContains(' ok 2 - example_mock_test', result) self.assertContains('ok 1 - example', result) - file.close() def test_output_with_prefix_isolated_correctly(self): log_path = get_absolute_path( @@ -143,42 +142,39 @@ def test_output_with_prefix_isolated_correctly(self): def test_parse_successful_test_log(self): all_passed_log = get_absolute_path( 'test_data/test_is_test_passed-all_passed.log') - file = open(all_passed_log) - result = kunit_parser.parse_run_tests(file.readlines()) + with open(all_passed_log) as file: + result = kunit_parser.parse_run_tests(file.readlines()) self.assertEqual( kunit_parser.TestStatus.SUCCESS, result.status) - file.close() def test_parse_failed_test_log(self): failed_log = get_absolute_path( 'test_data/test_is_test_passed-failure.log') - file = open(failed_log) - result = kunit_parser.parse_run_tests(file.readlines()) + with open(failed_log) as file: + result = kunit_parser.parse_run_tests(file.readlines()) self.assertEqual( kunit_parser.TestStatus.FAILURE, result.status) - file.close() def test_no_tests(self): empty_log = get_absolute_path( 'test_data/test_is_test_passed-no_tests_run.log') - file = open(empty_log) - result = kunit_parser.parse_run_tests( - kunit_parser.isolate_kunit_output(file.readlines())) + with open(empty_log) as file: + result = kunit_parser.parse_run_tests( + kunit_parser.isolate_kunit_output(file.readlines())) self.assertEqual(0, len(result.suites)) self.assertEqual( kunit_parser.TestStatus.NO_TESTS, result.status) - file.close() def test_no_kunit_output(self): crash_log = get_absolute_path( 'test_data/test_insufficient_memory.log') - file = open(crash_log) print_mock = mock.patch('builtins.print').start() - result = kunit_parser.parse_run_tests( - kunit_parser.isolate_kunit_output(file.readlines())) + with open(crash_log) as file: + result = kunit_parser.parse_run_tests( + kunit_parser.isolate_kunit_output(file.readlines())) print_mock.assert_any_call(StrContains('no tests run!')) print_mock.stop() file.close() @@ -186,12 +182,11 @@ def test_no_kunit_output(self): def test_crashed_test(self): crashed_log = get_absolute_path( 'test_data/test_is_test_passed-crash.log') - file = open(crashed_log) - result = kunit_parser.parse_run_tests(file.readlines()) + with open(crashed_log) as file: + result = kunit_parser.parse_run_tests(file.readlines()) self.assertEqual( kunit_parser.TestStatus.TEST_CRASHED, result.status) - file.close() def test_ignores_prefix_printk_time(self): prefix_log = get_absolute_path( From cd4a9bc8e0472da94f60f980d325c4825eacd918 Mon Sep 17 00:00:00 2001 From: Daniel Latypov Date: Wed, 2 Dec 2020 11:08:24 -0800 Subject: [PATCH 04/12] minor: kunit: tool: fix unit test so it can run from non-root dir MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Also take this time to rename get_absolute_path() to test_data_path(). 1. the name is currently a lie. It gives relative paths, e.g. if I run from the same dir as the test file, it gives './test_data/' See https://docs.python.org/3/reference/import.html#__file__, which doesn't stipulate that implementations provide absolute paths. 2. it's only used for generating paths to tools/testing/kunit/test_data/ So we can tersen things by making it less general. Cache the absolute path to the test data files per suggestion from [1]. Using relative paths, the tests break because of this code in kunit.py if get_kernel_root_path():         os.chdir(get_kernel_root_path()) [1] https://lore.kernel.org/linux-kselftest/CABVgOSnH0gz7z5JhRCGyG1wg0zDDBTLoSUCoB-gWMeXLgVTo2w@mail.gmail.com/ Fixes: 5578d008d9e0 ("kunit: tool: fix running kunit_tool from outside kernel tree") Signed-off-by: Daniel Latypov Reviewed-by: David Gow Tested-by: Brendan Higgins Acked-by: Brendan Higgins Signed-off-by: Shuah Khan --- tools/testing/kunit/kunit_tool_test.py | 60 +++++++++++--------------- 1 file changed, 24 insertions(+), 36 deletions(-) diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py index 9a36936749f0d..888f42129f20a 100755 --- a/tools/testing/kunit/kunit_tool_test.py +++ b/tools/testing/kunit/kunit_tool_test.py @@ -21,16 +21,18 @@ import kunit test_tmpdir = '' +abs_test_data_dir = '' def setUpModule(): - global test_tmpdir + global test_tmpdir, abs_test_data_dir test_tmpdir = tempfile.mkdtemp() + abs_test_data_dir = os.path.abspath(os.path.join(os.path.dirname(__file__), 'test_data')) def tearDownModule(): shutil.rmtree(test_tmpdir) -def get_absolute_path(path): - return os.path.join(os.path.dirname(__file__), path) +def test_data_path(path): + return os.path.join(abs_test_data_dir, path) class KconfigTest(unittest.TestCase): @@ -46,8 +48,7 @@ def test_is_subset_of(self): def test_read_from_file(self): kconfig = kunit_config.Kconfig() - kconfig_path = get_absolute_path( - 'test_data/test_read_from_file.kconfig') + kconfig_path = test_data_path('test_read_from_file.kconfig') kconfig.read_from_file(kconfig_path) @@ -98,8 +99,7 @@ def assertContains(self, needle, haystack): str(needle) + '" not found in "' + str(haystack) + '"!') def test_output_isolated_correctly(self): - log_path = get_absolute_path( - 'test_data/test_output_isolated_correctly.log') + log_path = test_data_path('test_output_isolated_correctly.log') with open(log_path) as file: result = kunit_parser.isolate_kunit_output(file.readlines()) self.assertContains('TAP version 14', result) @@ -110,8 +110,7 @@ def test_output_isolated_correctly(self): self.assertContains('ok 1 - example', result) def test_output_with_prefix_isolated_correctly(self): - log_path = get_absolute_path( - 'test_data/test_pound_sign.log') + log_path = test_data_path('test_pound_sign.log') with open(log_path) as file: result = kunit_parser.isolate_kunit_output(file.readlines()) self.assertContains('TAP version 14', result) @@ -140,8 +139,7 @@ def test_output_with_prefix_isolated_correctly(self): self.assertContains('ok 3 - string-stream-test', result) def test_parse_successful_test_log(self): - all_passed_log = get_absolute_path( - 'test_data/test_is_test_passed-all_passed.log') + all_passed_log = test_data_path('test_is_test_passed-all_passed.log') with open(all_passed_log) as file: result = kunit_parser.parse_run_tests(file.readlines()) self.assertEqual( @@ -149,8 +147,7 @@ def test_parse_successful_test_log(self): result.status) def test_parse_failed_test_log(self): - failed_log = get_absolute_path( - 'test_data/test_is_test_passed-failure.log') + failed_log = test_data_path('test_is_test_passed-failure.log') with open(failed_log) as file: result = kunit_parser.parse_run_tests(file.readlines()) self.assertEqual( @@ -158,8 +155,7 @@ def test_parse_failed_test_log(self): result.status) def test_no_tests(self): - empty_log = get_absolute_path( - 'test_data/test_is_test_passed-no_tests_run.log') + empty_log = test_data_path('test_is_test_passed-no_tests_run.log') with open(empty_log) as file: result = kunit_parser.parse_run_tests( kunit_parser.isolate_kunit_output(file.readlines())) @@ -169,8 +165,7 @@ def test_no_tests(self): result.status) def test_no_kunit_output(self): - crash_log = get_absolute_path( - 'test_data/test_insufficient_memory.log') + crash_log = test_data_path('test_insufficient_memory.log') print_mock = mock.patch('builtins.print').start() with open(crash_log) as file: result = kunit_parser.parse_run_tests( @@ -180,8 +175,7 @@ def test_no_kunit_output(self): file.close() def test_crashed_test(self): - crashed_log = get_absolute_path( - 'test_data/test_is_test_passed-crash.log') + crashed_log = test_data_path('test_is_test_passed-crash.log') with open(crashed_log) as file: result = kunit_parser.parse_run_tests(file.readlines()) self.assertEqual( @@ -189,8 +183,7 @@ def test_crashed_test(self): result.status) def test_ignores_prefix_printk_time(self): - prefix_log = get_absolute_path( - 'test_data/test_config_printk_time.log') + prefix_log = test_data_path('test_config_printk_time.log') with open(prefix_log) as file: result = kunit_parser.parse_run_tests(file.readlines()) self.assertEqual( @@ -199,8 +192,7 @@ def test_ignores_prefix_printk_time(self): self.assertEqual('kunit-resource-test', result.suites[0].name) def test_ignores_multiple_prefixes(self): - prefix_log = get_absolute_path( - 'test_data/test_multiple_prefixes.log') + prefix_log = test_data_path('test_multiple_prefixes.log') with open(prefix_log) as file: result = kunit_parser.parse_run_tests(file.readlines()) self.assertEqual( @@ -209,8 +201,7 @@ def test_ignores_multiple_prefixes(self): self.assertEqual('kunit-resource-test', result.suites[0].name) def test_prefix_mixed_kernel_output(self): - mixed_prefix_log = get_absolute_path( - 'test_data/test_interrupted_tap_output.log') + mixed_prefix_log = test_data_path('test_interrupted_tap_output.log') with open(mixed_prefix_log) as file: result = kunit_parser.parse_run_tests(file.readlines()) self.assertEqual( @@ -219,7 +210,7 @@ def test_prefix_mixed_kernel_output(self): self.assertEqual('kunit-resource-test', result.suites[0].name) def test_prefix_poundsign(self): - pound_log = get_absolute_path('test_data/test_pound_sign.log') + pound_log = test_data_path('test_pound_sign.log') with open(pound_log) as file: result = kunit_parser.parse_run_tests(file.readlines()) self.assertEqual( @@ -228,7 +219,7 @@ def test_prefix_poundsign(self): self.assertEqual('kunit-resource-test', result.suites[0].name) def test_kernel_panic_end(self): - panic_log = get_absolute_path('test_data/test_kernel_panic_interrupt.log') + panic_log = test_data_path('test_kernel_panic_interrupt.log') with open(panic_log) as file: result = kunit_parser.parse_run_tests(file.readlines()) self.assertEqual( @@ -237,7 +228,7 @@ def test_kernel_panic_end(self): self.assertEqual('kunit-resource-test', result.suites[0].name) def test_pound_no_prefix(self): - pound_log = get_absolute_path('test_data/test_pound_no_prefix.log') + pound_log = test_data_path('test_pound_no_prefix.log') with open(pound_log) as file: result = kunit_parser.parse_run_tests(file.readlines()) self.assertEqual( @@ -248,7 +239,7 @@ def test_pound_no_prefix(self): class KUnitJsonTest(unittest.TestCase): def _json_for(self, log_file): - with(open(get_absolute_path(log_file))) as file: + with open(test_data_path(log_file)) as file: test_result = kunit_parser.parse_run_tests(file) json_obj = kunit_json.get_json_result( test_result=test_result, @@ -258,22 +249,19 @@ def _json_for(self, log_file): return json.loads(json_obj) def test_failed_test_json(self): - result = self._json_for( - 'test_data/test_is_test_passed-failure.log') + result = self._json_for('test_is_test_passed-failure.log') self.assertEqual( {'name': 'example_simple_test', 'status': 'FAIL'}, result["sub_groups"][1]["test_cases"][0]) def test_crashed_test_json(self): - result = self._json_for( - 'test_data/test_is_test_passed-crash.log') + result = self._json_for('test_is_test_passed-crash.log') self.assertEqual( {'name': 'example_simple_test', 'status': 'ERROR'}, result["sub_groups"][1]["test_cases"][0]) def test_no_tests_json(self): - result = self._json_for( - 'test_data/test_is_test_passed-no_tests_run.log') + result = self._json_for('test_is_test_passed-no_tests_run.log') self.assertEqual(0, len(result['sub_groups'])) class StrContains(str): @@ -282,7 +270,7 @@ def __eq__(self, other): class KUnitMainTest(unittest.TestCase): def setUp(self): - path = get_absolute_path('test_data/test_is_test_passed-all_passed.log') + path = test_data_path('test_is_test_passed-all_passed.log') with open(path) as file: all_passed_log = file.readlines() From d3bae4a0b6e1bfbfcff3dbc2a6d96a505e31677e Mon Sep 17 00:00:00 2001 From: Daniel Latypov Date: Tue, 8 Dec 2020 15:21:02 -0800 Subject: [PATCH 05/12] kunit: tool: simplify kconfig is_subset_of() logic Don't use an O(nm) algorithm* and make it more readable by using a dict. *Most obviously, it does a nested for-loop over the entire other config. A bit more subtle, it calls .entries(), which constructs a set from the list for _every_ outer iteration. Signed-off-by: Daniel Latypov Reviewed-by: David Gow Tested-by: Brendan Higgins Acked-by: Brendan Higgins Signed-off-by: Shuah Khan --- tools/testing/kunit/kunit_config.py | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/tools/testing/kunit/kunit_config.py b/tools/testing/kunit/kunit_config.py index bdd60230764b0..0b550cbd667d1 100644 --- a/tools/testing/kunit/kunit_config.py +++ b/tools/testing/kunit/kunit_config.py @@ -41,15 +41,14 @@ def add_entry(self, entry: KconfigEntry) -> None: self._entries.append(entry) def is_subset_of(self, other: 'Kconfig') -> bool: + other_dict = {e.name: e.value for e in other.entries()} for a in self.entries(): - found = False - for b in other.entries(): - if a.name != b.name: + b = other_dict.get(a.name) + if b is None: + if a.value == 'n': continue - if a.value != b.value: - return False - found = True - if a.value != 'n' and found == False: + return False + elif a.value != b: return False return True From c9ef2d3e3f3b3e56429f56bbea2d16882b054dbe Mon Sep 17 00:00:00 2001 From: Daniel Latypov Date: Tue, 19 Jan 2021 15:52:26 -0800 Subject: [PATCH 06/12] KUnit: Docs: make start.rst example Kconfig follow style.rst The primary change is that we want to encourage people to respect KUNIT_ALL_TESTS to make it easy to run all the relevant tests for a given config. Signed-off-by: Daniel Latypov Reviewed-by: Brendan Higgins Signed-off-by: Shuah Khan --- Documentation/dev-tools/kunit/start.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Documentation/dev-tools/kunit/start.rst b/Documentation/dev-tools/kunit/start.rst index 454f307813ea6..560f27af46197 100644 --- a/Documentation/dev-tools/kunit/start.rst +++ b/Documentation/dev-tools/kunit/start.rst @@ -196,8 +196,9 @@ Now add the following to ``drivers/misc/Kconfig``: .. code-block:: kconfig config MISC_EXAMPLE_TEST - bool "Test for my example" + tristate "Test for my example" if !KUNIT_ALL_TESTS depends on MISC_EXAMPLE && KUNIT=y + default KUNIT_ALL_TESTS and the following to ``drivers/misc/Makefile``: From 7c2b108cbe75f993d5e69d5205a01211fa33417d Mon Sep 17 00:00:00 2001 From: Daniel Latypov Date: Mon, 25 Jan 2021 10:53:33 -0800 Subject: [PATCH 07/12] Documentation: kunit: add tips.rst for small examples ./usage.rst contains fairly long examples and explanations of things like how to fake a class and how to use parameterized tests (and how you could do table-driven tests yourself). It's not exactly necessary information, so we add a new page with more digestible tips like "use kunit_kzalloc() instead of kzalloc() so you don't have to worry about calling kfree() yourself" and the like. Change start.rst to point users to this new page first and let them know that usage.rst is more of optional further reading. Signed-off-by: Daniel Latypov Reviewed-by: Brendan Higgins Signed-off-by: Shuah Khan --- Documentation/dev-tools/kunit/index.rst | 2 + Documentation/dev-tools/kunit/start.rst | 4 +- Documentation/dev-tools/kunit/tips.rst | 115 ++++++++++++++++++++++++ 3 files changed, 120 insertions(+), 1 deletion(-) create mode 100644 Documentation/dev-tools/kunit/tips.rst diff --git a/Documentation/dev-tools/kunit/index.rst b/Documentation/dev-tools/kunit/index.rst index c234a3ab3c34f..8484788383475 100644 --- a/Documentation/dev-tools/kunit/index.rst +++ b/Documentation/dev-tools/kunit/index.rst @@ -13,6 +13,7 @@ KUnit - Unit Testing for the Linux Kernel api/index style faq + tips What is KUnit? ============== @@ -88,6 +89,7 @@ How do I use it? ================ * :doc:`start` - for new users of KUnit +* :doc:`tips` - for short examples of best practices * :doc:`usage` - for a more detailed explanation of KUnit features * :doc:`api/index` - for the list of KUnit APIs used for testing * :doc:`kunit-tool` - for more information on the kunit_tool helper script diff --git a/Documentation/dev-tools/kunit/start.rst b/Documentation/dev-tools/kunit/start.rst index 560f27af46197..0e65cabe08eb9 100644 --- a/Documentation/dev-tools/kunit/start.rst +++ b/Documentation/dev-tools/kunit/start.rst @@ -234,5 +234,7 @@ Congrats! You just wrote your first KUnit test! Next Steps ========== -* Check out the :doc:`usage` page for a more +* Check out the :doc:`tips` page for tips on + writing idiomatic KUnit tests. +* Optional: see the :doc:`usage` page for a more in-depth explanation of KUnit. diff --git a/Documentation/dev-tools/kunit/tips.rst b/Documentation/dev-tools/kunit/tips.rst new file mode 100644 index 0000000000000..a6ca0af140985 --- /dev/null +++ b/Documentation/dev-tools/kunit/tips.rst @@ -0,0 +1,115 @@ +.. SPDX-License-Identifier: GPL-2.0 + +============================ +Tips For Writing KUnit Tests +============================ + +Exiting early on failed expectations +------------------------------------ + +``KUNIT_EXPECT_EQ`` and friends will mark the test as failed and continue +execution. In some cases, it's unsafe to continue and you can use the +``KUNIT_ASSERT`` variant to exit on failure. + +.. code-block:: c + + void example_test_user_alloc_function(struct kunit *test) + { + void *object = alloc_some_object_for_me(); + + /* Make sure we got a valid pointer back. */ + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, object); + do_something_with_object(object); + } + +Allocating memory +----------------- + +Where you would use ``kzalloc``, you should prefer ``kunit_kzalloc`` instead. +KUnit will ensure the memory is freed once the test completes. + +This is particularly useful since it lets you use the ``KUNIT_ASSERT_EQ`` +macros to exit early from a test without having to worry about remembering to +call ``kfree``. + +Example: + +.. code-block:: c + + void example_test_allocation(struct kunit *test) + { + char *buffer = kunit_kzalloc(test, 16, GFP_KERNEL); + /* Ensure allocation succeeded. */ + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, buffer); + + KUNIT_ASSERT_STREQ(test, buffer, ""); + } + + +Testing static functions +------------------------ + +If you don't want to expose functions or variables just for testing, one option +is to conditionally ``#include`` the test file at the end of your .c file, e.g. + +.. code-block:: c + + /* In my_file.c */ + + static int do_interesting_thing(); + + #ifdef CONFIG_MY_KUNIT_TEST + #include "my_kunit_test.c" + #endif + +Injecting test-only code +------------------------ + +Similarly to the above, it can be useful to add test-specific logic. + +.. code-block:: c + + /* In my_file.h */ + + #ifdef CONFIG_MY_KUNIT_TEST + /* Defined in my_kunit_test.c */ + void test_only_hook(void); + #else + void test_only_hook(void) { } + #endif + +TODO(dlatypov@google.com): add an example of using ``current->kunit_test`` in +such a hook when it's not only updated for ``CONFIG_KASAN=y``. + +Customizing error messages +-------------------------- + +Each of the ``KUNIT_EXPECT`` and ``KUNIT_ASSERT`` macros have a ``_MSG`` variant. +These take a format string and arguments to provide additional context to the automatically generated error messages. + +.. code-block:: c + + char some_str[41]; + generate_sha1_hex_string(some_str); + + /* Before. Not easy to tell why the test failed. */ + KUNIT_EXPECT_EQ(test, strlen(some_str), 40); + + /* After. Now we see the offending string. */ + KUNIT_EXPECT_EQ_MSG(test, strlen(some_str), 40, "some_str='%s'", some_str); + +Alternatively, one can take full control over the error message by using ``KUNIT_FAIL()``, e.g. + +.. code-block:: c + + /* Before */ + KUNIT_EXPECT_EQ(test, some_setup_function(), 0); + + /* After: full control over the failure message. */ + if (some_setup_function()) + KUNIT_FAIL(test, "Failed to setup thing for testing"); + +Next Steps +========== +* Optional: see the :doc:`usage` page for a more + in-depth explanation of KUnit. From 243180f5924ed27ea417db39feb7f9691777688e Mon Sep 17 00:00:00 2001 From: Daniel Latypov Date: Mon, 1 Feb 2021 12:55:14 -0800 Subject: [PATCH 08/12] kunit: make kunit_tool accept optional path to .kunitconfig fragment Currently running tests via KUnit tool means tweaking a .kunitconfig file, which you'd keep around locally and never commit. This changes makes it so users can pass in a path to a kunitconfig. One of the imagined use cases is having kunitconfig fragments in-tree to formalize interesting sets of tests for features/subsystems, e.g. $ ./tools/testing/kunit/kunit.py run --kunticonfig=fs/ext4/kunitconfig For now, this hypothetical fs/ext4/kunitconfig would contain CONFIG_KUNIT=y CONFIG_EXT4_FS=y CONFIG_EXT4_KUNIT_TESTS=y At the moment, it's not hard to manually whip up this file, but as more and more tests get added, this will get tedious. It also opens the door to documenting how to run all the tests relevant to a specific subsystem or feature as a simple one-liner. This can be seen as an analogue to tools/testing/selftests/*/config But in the case of KUnit, the tests live in the same directory as the code-under-test, so it feels more natural to allow the kunitconfig fragments to live anywhere. (Though, people could create a separate directory if wanted; this patch imposes no restrictions on the path). Signed-off-by: Daniel Latypov Reviewed-by: Brendan Higgins Tested-by: Brendan Higgins Signed-off-by: Shuah Khan --- tools/testing/kunit/kunit.py | 9 +++++--- tools/testing/kunit/kunit_kernel.py | 12 ++++++---- tools/testing/kunit/kunit_tool_test.py | 32 ++++++++++++++++++++++++++ 3 files changed, 46 insertions(+), 7 deletions(-) diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py index e808a47c839bf..02871a363f765 100755 --- a/tools/testing/kunit/kunit.py +++ b/tools/testing/kunit/kunit.py @@ -182,6 +182,9 @@ def add_common_opts(parser) -> None: parser.add_argument('--alltests', help='Run all KUnit tests through allyesconfig', action='store_true') + parser.add_argument('--kunitconfig', + help='Path to Kconfig fragment that enables KUnit tests', + metavar='kunitconfig') def add_build_opts(parser) -> None: parser.add_argument('--jobs', @@ -256,7 +259,7 @@ def main(argv, linux=None): os.mkdir(cli_args.build_dir) if not linux: - linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir) + linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir, kunitconfig_path=cli_args.kunitconfig) request = KunitRequest(cli_args.raw_output, cli_args.timeout, @@ -274,7 +277,7 @@ def main(argv, linux=None): os.mkdir(cli_args.build_dir) if not linux: - linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir) + linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir, kunitconfig_path=cli_args.kunitconfig) request = KunitConfigRequest(cli_args.build_dir, cli_args.make_options) @@ -286,7 +289,7 @@ def main(argv, linux=None): sys.exit(1) elif cli_args.subcommand == 'build': if not linux: - linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir) + linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir, kunitconfig_path=cli_args.kunitconfig) request = KunitBuildRequest(cli_args.jobs, cli_args.build_dir, diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py index 2076a5a2d060a..0b461663e7d98 100644 --- a/tools/testing/kunit/kunit_kernel.py +++ b/tools/testing/kunit/kunit_kernel.py @@ -123,7 +123,7 @@ def get_outfile_path(build_dir) -> str: class LinuxSourceTree(object): """Represents a Linux kernel source tree with KUnit tests.""" - def __init__(self, build_dir: str, load_config=True, defconfig=DEFAULT_KUNITCONFIG_PATH) -> None: + def __init__(self, build_dir: str, load_config=True, kunitconfig_path='') -> None: signal.signal(signal.SIGINT, self.signal_handler) self._ops = LinuxSourceTreeOperations() @@ -131,9 +131,13 @@ def __init__(self, build_dir: str, load_config=True, defconfig=DEFAULT_KUNITCONF if not load_config: return - kunitconfig_path = get_kunitconfig_path(build_dir) - if not os.path.exists(kunitconfig_path): - shutil.copyfile(defconfig, kunitconfig_path) + if kunitconfig_path: + if not os.path.exists(kunitconfig_path): + raise ConfigError(f'Specified kunitconfig ({kunitconfig_path}) does not exist') + else: + kunitconfig_path = get_kunitconfig_path(build_dir) + if not os.path.exists(kunitconfig_path): + shutil.copyfile(DEFAULT_KUNITCONFIG_PATH, kunitconfig_path) self._kconfig = kunit_config.Kconfig() self._kconfig.read_from_file(kunitconfig_path) diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py index 888f42129f20a..251bb0b4bc2ee 100755 --- a/tools/testing/kunit/kunit_tool_test.py +++ b/tools/testing/kunit/kunit_tool_test.py @@ -12,6 +12,7 @@ import tempfile, shutil # Handling test_tmpdir import json +import signal import os import kunit_config @@ -236,6 +237,23 @@ def test_pound_no_prefix(self): result.status) self.assertEqual('kunit-resource-test', result.suites[0].name) +class LinuxSourceTreeTest(unittest.TestCase): + + def setUp(self): + mock.patch.object(signal, 'signal').start() + self.addCleanup(mock.patch.stopall) + + def test_invalid_kunitconfig(self): + with self.assertRaisesRegex(kunit_kernel.ConfigError, 'nonexistent.* does not exist'): + kunit_kernel.LinuxSourceTree('', kunitconfig_path='/nonexistent_file') + + def test_valid_kunitconfig(self): + with tempfile.NamedTemporaryFile('wt') as kunitconfig: + tree = kunit_kernel.LinuxSourceTree('', kunitconfig_path=kunitconfig.name) + + # TODO: add more test cases. + + class KUnitJsonTest(unittest.TestCase): def _json_for(self, log_file): @@ -378,5 +396,19 @@ def test_exec_builddir(self): self.linux_source_mock.run_kernel.assert_called_once_with(build_dir=build_dir, timeout=300) self.print_mock.assert_any_call(StrContains('Testing complete.')) + @mock.patch.object(kunit_kernel, 'LinuxSourceTree') + def test_run_kunitconfig(self, mock_linux_init): + mock_linux_init.return_value = self.linux_source_mock + kunit.main(['run', '--kunitconfig=mykunitconfig']) + # Just verify that we parsed and initialized it correctly here. + mock_linux_init.assert_called_once_with('.kunit', kunitconfig_path='mykunitconfig') + + @mock.patch.object(kunit_kernel, 'LinuxSourceTree') + def test_config_kunitconfig(self, mock_linux_init): + mock_linux_init.return_value = self.linux_source_mock + kunit.main(['config', '--kunitconfig=mykunitconfig']) + # Just verify that we parsed and initialized it correctly here. + mock_linux_init.assert_called_once_with('.kunit', kunitconfig_path='mykunitconfig') + if __name__ == '__main__': unittest.main() From 65af9b964d72d8d8e88f4f673d4d0e9467197373 Mon Sep 17 00:00:00 2001 From: Daniel Latypov Date: Fri, 5 Feb 2021 14:14:09 -0800 Subject: [PATCH 09/12] kunit: don't show `1 == 1` in failed assertion messages Currently, given something (fairly dystopian) like > KUNIT_EXPECT_EQ(test, 2 + 2, 5) KUnit will prints a failure message like this. > Expected 2 + 2 == 5, but > 2 + 2 == 4 > 5 == 5 With this patch, the output just becomes > Expected 2 + 2 == 5, but > 2 + 2 == 4 This patch is slightly hacky, but it's quite common* to compare an expression to a literal integer value, so this can make KUnit less chatty in many cases. (This patch also fixes variants like KUNIT_EXPECT_GT, LE, et al.). It also allocates an additional string briefly, but given this only happens on test failures, it doesn't seem too bad a tradeoff. Also, in most cases it'll realize the lengths are unequal and bail out before the allocation. We could save the result of the formatted string to avoid wasting this extra work, but it felt cleaner to leave it as-is. Edge case: for something silly and unrealistic like > KUNIT_EXPECT_EQ(test, 4, 5); It'll generate this message with a trailing "but" > Expected 4 == 5, but > It didn't feel worth adding a check up-front to see if both sides are literals to handle this better. *A quick grep suggests 100+ comparisons to an integer literal as the right hand side. Signed-off-by: Daniel Latypov Tested-by: David Gow Reviewed-by: Brendan Higgins Signed-off-by: Shuah Khan --- lib/kunit/assert.c | 39 +++++++++++++++++++++++++++++++++------ 1 file changed, 33 insertions(+), 6 deletions(-) diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c index 33acdaa28a7d9..e0ec7d6fed6fe 100644 --- a/lib/kunit/assert.c +++ b/lib/kunit/assert.c @@ -85,6 +85,29 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert, } EXPORT_SYMBOL_GPL(kunit_ptr_not_err_assert_format); +/* Checks if `text` is a literal representing `value`, e.g. "5" and 5 */ +static bool is_literal(struct kunit *test, const char *text, long long value, + gfp_t gfp) +{ + char *buffer; + int len; + bool ret; + + len = snprintf(NULL, 0, "%lld", value); + if (strlen(text) != len) + return false; + + buffer = kunit_kmalloc(test, len+1, gfp); + if (!buffer) + return false; + + snprintf(buffer, len+1, "%lld", value); + ret = strncmp(buffer, text, len) == 0; + + kunit_kfree(test, buffer); + return ret; +} + void kunit_binary_assert_format(const struct kunit_assert *assert, struct string_stream *stream) { @@ -97,12 +120,16 @@ void kunit_binary_assert_format(const struct kunit_assert *assert, binary_assert->left_text, binary_assert->operation, binary_assert->right_text); - string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %lld\n", - binary_assert->left_text, - binary_assert->left_value); - string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %lld", - binary_assert->right_text, - binary_assert->right_value); + if (!is_literal(stream->test, binary_assert->left_text, + binary_assert->left_value, stream->gfp)) + string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %lld\n", + binary_assert->left_text, + binary_assert->left_value); + if (!is_literal(stream->test, binary_assert->right_text, + binary_assert->right_value, stream->gfp)) + string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %lld", + binary_assert->right_text, + binary_assert->right_value); kunit_assert_print_msg(assert, stream); } EXPORT_SYMBOL_GPL(kunit_binary_assert_format); From 5d31f71efcb6bce56ca3ab92eed0c8f2dbcc6f9a Mon Sep 17 00:00:00 2001 From: Daniel Latypov Date: Fri, 5 Feb 2021 16:08:52 -0800 Subject: [PATCH 10/12] kunit: add kunit.filter_glob cmdline option to filter suites E.g. specifying this would run suites with "list" in their name. kunit.filter_glob=list* Note: the executor prints out a TAP header that includes the number of suites we intend to run. So unless we want to report empty results for filtered-out suites, we need to do the filtering here in the executor. It's also probably better in the executor since we most likely don't want any filtering to apply to tests built as modules. This code does add a CONFIG_GLOB=y dependency for CONFIG_KUNIT=y. But the code seems light enough that it shouldn't be an issue. For now, we only filter on suite names so we don't have to create copies of the suites themselves, just the array (of arrays) holding them. The name is rather generic since in the future, we could consider extending it to a syntax like: kunit.filter_glob=. E.g. to run all the del list tests kunit.filter_glob=list-kunit-test.*del* But at the moment, it's far easier to manually comment out test cases in test files as opposed to messing with sets of Kconfig entries to select specific suites. So even just doing this makes using kunit far less annoying. Signed-off-by: Daniel Latypov Reviewed-by: Brendan Higgins Signed-off-by: Shuah Khan --- lib/kunit/Kconfig | 1 + lib/kunit/executor.c | 93 +++++++++++++++++++++++++++++++++++++++----- 2 files changed, 85 insertions(+), 9 deletions(-) diff --git a/lib/kunit/Kconfig b/lib/kunit/Kconfig index 00909e6a24438..0b5dfb001bacc 100644 --- a/lib/kunit/Kconfig +++ b/lib/kunit/Kconfig @@ -4,6 +4,7 @@ menuconfig KUNIT tristate "KUnit - Enable support for unit tests" + select GLOB if KUNIT=y help Enables support for kernel unit tests (KUnit), a lightweight unit testing and mocking framework for the Linux kernel. These tests are diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c index a95742a4ece73..15832ed446685 100644 --- a/lib/kunit/executor.c +++ b/lib/kunit/executor.c @@ -1,6 +1,8 @@ // SPDX-License-Identifier: GPL-2.0 #include +#include +#include /* * These symbols point to the .kunit_test_suites section and are defined in @@ -11,14 +13,81 @@ extern struct kunit_suite * const * const __kunit_suites_end[]; #if IS_BUILTIN(CONFIG_KUNIT) -static void kunit_print_tap_header(void) +static char *filter_glob; +module_param(filter_glob, charp, 0); +MODULE_PARM_DESC(filter_glob, + "Filter which KUnit test suites run at boot-time, e.g. list*"); + +static struct kunit_suite * const * +kunit_filter_subsuite(struct kunit_suite * const * const subsuite) +{ + int i, n = 0; + struct kunit_suite **filtered; + + n = 0; + for (i = 0; subsuite[i] != NULL; ++i) { + if (glob_match(filter_glob, subsuite[i]->name)) + ++n; + } + + if (n == 0) + return NULL; + + filtered = kmalloc_array(n + 1, sizeof(*filtered), GFP_KERNEL); + if (!filtered) + return NULL; + + n = 0; + for (i = 0; subsuite[i] != NULL; ++i) { + if (glob_match(filter_glob, subsuite[i]->name)) + filtered[n++] = subsuite[i]; + } + filtered[n] = NULL; + + return filtered; +} + +struct suite_set { + struct kunit_suite * const * const *start; + struct kunit_suite * const * const *end; +}; + +static struct suite_set kunit_filter_suites(void) +{ + int i; + struct kunit_suite * const **copy, * const *filtered_subsuite; + struct suite_set filtered; + + const size_t max = __kunit_suites_end - __kunit_suites_start; + + if (!filter_glob) { + filtered.start = __kunit_suites_start; + filtered.end = __kunit_suites_end; + return filtered; + } + + copy = kmalloc_array(max, sizeof(*filtered.start), GFP_KERNEL); + filtered.start = copy; + if (!copy) { /* won't be able to run anything, return an empty set */ + filtered.end = copy; + return filtered; + } + + for (i = 0; i < max; ++i) { + filtered_subsuite = kunit_filter_subsuite(__kunit_suites_start[i]); + if (filtered_subsuite) + *copy++ = filtered_subsuite; + } + filtered.end = copy; + return filtered; +} + +static void kunit_print_tap_header(struct suite_set *suite_set) { struct kunit_suite * const * const *suites, * const *subsuite; int num_of_suites = 0; - for (suites = __kunit_suites_start; - suites < __kunit_suites_end; - suites++) + for (suites = suite_set->start; suites < suite_set->end; suites++) for (subsuite = *suites; *subsuite != NULL; subsuite++) num_of_suites++; @@ -30,12 +99,18 @@ int kunit_run_all_tests(void) { struct kunit_suite * const * const *suites; - kunit_print_tap_header(); + struct suite_set suite_set = kunit_filter_suites(); + + kunit_print_tap_header(&suite_set); + + for (suites = suite_set.start; suites < suite_set.end; suites++) + __kunit_test_suites_init(*suites); - for (suites = __kunit_suites_start; - suites < __kunit_suites_end; - suites++) - __kunit_test_suites_init(*suites); + if (filter_glob) { /* a copy was made of each array */ + for (suites = suite_set.start; suites < suite_set.end; suites++) + kfree(*suites); + kfree(suite_set.start); + } return 0; } From d992880b3d265597c5a16af3775257999492e957 Mon Sep 17 00:00:00 2001 From: Daniel Latypov Date: Fri, 5 Feb 2021 16:08:53 -0800 Subject: [PATCH 11/12] kunit: tool: add support for filtering suites by glob This allows running different subsets of tests, e.g. $ ./tools/testing/kunit/kunit.py build $ ./tools/testing/kunit/kunit.py exec 'list*' $ ./tools/testing/kunit/kunit.py exec 'kunit*' This passes the "kunit_filter.glob" commandline option to the UML kernel, which currently only supports filtering by suite name. Signed-off-by: Daniel Latypov Reviewed-by: Brendan Higgins Signed-off-by: Shuah Khan --- tools/testing/kunit/kunit.py | 21 ++++++++++++++++----- tools/testing/kunit/kunit_kernel.py | 4 +++- tools/testing/kunit/kunit_tool_test.py | 15 +++++++++------ 3 files changed, 28 insertions(+), 12 deletions(-) diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py index 02871a363f765..d5144fcb03acd 100755 --- a/tools/testing/kunit/kunit.py +++ b/tools/testing/kunit/kunit.py @@ -28,12 +28,12 @@ ['jobs', 'build_dir', 'alltests', 'make_options']) KunitExecRequest = namedtuple('KunitExecRequest', - ['timeout', 'build_dir', 'alltests']) + ['timeout', 'build_dir', 'alltests', 'filter_glob']) KunitParseRequest = namedtuple('KunitParseRequest', ['raw_output', 'input_data', 'build_dir', 'json']) KunitRequest = namedtuple('KunitRequest', ['raw_output','timeout', 'jobs', - 'build_dir', 'alltests', 'json', - 'make_options']) + 'build_dir', 'alltests', 'filter_glob', + 'json', 'make_options']) KernelDirectoryPath = sys.argv[0].split('tools/testing/kunit/')[0] @@ -93,6 +93,7 @@ def exec_tests(linux: kunit_kernel.LinuxSourceTree, test_start = time.time() result = linux.run_kernel( timeout=None if request.alltests else request.timeout, + filter_glob=request.filter_glob, build_dir=request.build_dir) test_end = time.time() @@ -149,7 +150,7 @@ def run_tests(linux: kunit_kernel.LinuxSourceTree, return build_result exec_request = KunitExecRequest(request.timeout, request.build_dir, - request.alltests) + request.alltests, request.filter_glob) exec_result = exec_tests(linux, exec_request) if exec_result.status != KunitStatus.SUCCESS: return exec_result @@ -200,6 +201,14 @@ def add_exec_opts(parser) -> None: type=int, default=300, metavar='timeout') + parser.add_argument('filter_glob', + help='maximum number of seconds to allow for all tests ' + 'to run. This does not include time taken to build the ' + 'tests.', + type=str, + nargs='?', + default='', + metavar='filter_glob') def add_parse_opts(parser) -> None: parser.add_argument('--raw_output', help='don\'t format output from kernel', @@ -266,6 +275,7 @@ def main(argv, linux=None): cli_args.jobs, cli_args.build_dir, cli_args.alltests, + cli_args.filter_glob, cli_args.json, cli_args.make_options) result = run_tests(linux, request) @@ -307,7 +317,8 @@ def main(argv, linux=None): exec_request = KunitExecRequest(cli_args.timeout, cli_args.build_dir, - cli_args.alltests) + cli_args.alltests, + cli_args.filter_glob) exec_result = exec_tests(linux, exec_request) parse_request = KunitParseRequest(cli_args.raw_output, exec_result.result, diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py index 0b461663e7d98..71a5f5c1750b3 100644 --- a/tools/testing/kunit/kunit_kernel.py +++ b/tools/testing/kunit/kunit_kernel.py @@ -203,8 +203,10 @@ def build_um_kernel(self, alltests, jobs, build_dir, make_options) -> bool: return False return self.validate_config(build_dir) - def run_kernel(self, args=[], build_dir='', timeout=None) -> Iterator[str]: + def run_kernel(self, args=[], build_dir='', filter_glob='', timeout=None) -> Iterator[str]: args.extend(['mem=1G', 'console=tty']) + if filter_glob: + args.append('kunit.filter_glob='+filter_glob) self._ops.linux_bin(args, timeout, build_dir) outfile = get_outfile_path(build_dir) subprocess.call(['stty', 'sane']) diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py index 251bb0b4bc2ee..1ad3049e90698 100755 --- a/tools/testing/kunit/kunit_tool_test.py +++ b/tools/testing/kunit/kunit_tool_test.py @@ -315,7 +315,8 @@ def test_exec_passes_args_pass(self): kunit.main(['exec'], self.linux_source_mock) self.assertEqual(self.linux_source_mock.build_reconfig.call_count, 0) self.assertEqual(self.linux_source_mock.run_kernel.call_count, 1) - self.linux_source_mock.run_kernel.assert_called_once_with(build_dir='.kunit', timeout=300) + self.linux_source_mock.run_kernel.assert_called_once_with( + build_dir='.kunit', filter_glob='', timeout=300) self.print_mock.assert_any_call(StrContains('Testing complete.')) def test_run_passes_args_pass(self): @@ -323,7 +324,7 @@ def test_run_passes_args_pass(self): self.assertEqual(self.linux_source_mock.build_reconfig.call_count, 1) self.assertEqual(self.linux_source_mock.run_kernel.call_count, 1) self.linux_source_mock.run_kernel.assert_called_once_with( - build_dir='.kunit', timeout=300) + build_dir='.kunit', filter_glob='', timeout=300) self.print_mock.assert_any_call(StrContains('Testing complete.')) def test_exec_passes_args_fail(self): @@ -361,7 +362,8 @@ def test_run_raw_output(self): def test_exec_timeout(self): timeout = 3453 kunit.main(['exec', '--timeout', str(timeout)], self.linux_source_mock) - self.linux_source_mock.run_kernel.assert_called_once_with(build_dir='.kunit', timeout=timeout) + self.linux_source_mock.run_kernel.assert_called_once_with( + build_dir='.kunit', filter_glob='', timeout=timeout) self.print_mock.assert_any_call(StrContains('Testing complete.')) def test_run_timeout(self): @@ -369,7 +371,7 @@ def test_run_timeout(self): kunit.main(['run', '--timeout', str(timeout)], self.linux_source_mock) self.assertEqual(self.linux_source_mock.build_reconfig.call_count, 1) self.linux_source_mock.run_kernel.assert_called_once_with( - build_dir='.kunit', timeout=timeout) + build_dir='.kunit', filter_glob='', timeout=timeout) self.print_mock.assert_any_call(StrContains('Testing complete.')) def test_run_builddir(self): @@ -377,7 +379,7 @@ def test_run_builddir(self): kunit.main(['run', '--build_dir=.kunit'], self.linux_source_mock) self.assertEqual(self.linux_source_mock.build_reconfig.call_count, 1) self.linux_source_mock.run_kernel.assert_called_once_with( - build_dir=build_dir, timeout=300) + build_dir=build_dir, filter_glob='', timeout=300) self.print_mock.assert_any_call(StrContains('Testing complete.')) def test_config_builddir(self): @@ -393,7 +395,8 @@ def test_build_builddir(self): def test_exec_builddir(self): build_dir = '.kunit' kunit.main(['exec', '--build_dir', build_dir], self.linux_source_mock) - self.linux_source_mock.run_kernel.assert_called_once_with(build_dir=build_dir, timeout=300) + self.linux_source_mock.run_kernel.assert_called_once_with( + build_dir=build_dir, filter_glob='', timeout=300) self.print_mock.assert_any_call(StrContains('Testing complete.')) @mock.patch.object(kunit_kernel, 'LinuxSourceTree') From 7af29141a31a2a2350589471c8979ff5f22fb9b7 Mon Sep 17 00:00:00 2001 From: Daniel Latypov Date: Fri, 5 Feb 2021 16:08:54 -0800 Subject: [PATCH 12/12] kunit: tool: fix unintentional statefulness in run_kernel() This is a bug that has been present since the first version of this code. Using [] as a default parameter is dangerous, since it's mutable. Example using the REPL: >>> def bad(param = []): ... param.append(len(param)) ... print(param) ... >>> bad() [0] >>> bad() [0, 1] This wasn't a concern in the past since it would just keep appending the same values to it. E.g. before, `args` would just grow in size like: [mem=1G', 'console=tty'] [mem=1G', 'console=tty', mem=1G', 'console=tty'] But with now filter_glob, this is more dangerous, e.g. run_kernel(filter_glob='my-test*') # default modified here run_kernel() # filter_glob still applies here! That earlier `filter_glob` will affect all subsequent calls that don't specify `args`. Note: currently the kunit tool only calls run_kernel() at most once, so it's not possible to trigger any negative side-effects right now. Fixes: 6ebf5866f2e8 ("kunit: tool: add Python wrappers for running KUnit tests") Signed-off-by: Daniel Latypov Reviewed-by: Brendan Higgins Signed-off-by: Shuah Khan --- tools/testing/kunit/kunit_kernel.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py index 71a5f5c1750b3..f309a33256cd3 100644 --- a/tools/testing/kunit/kunit_kernel.py +++ b/tools/testing/kunit/kunit_kernel.py @@ -203,7 +203,9 @@ def build_um_kernel(self, alltests, jobs, build_dir, make_options) -> bool: return False return self.validate_config(build_dir) - def run_kernel(self, args=[], build_dir='', filter_glob='', timeout=None) -> Iterator[str]: + def run_kernel(self, args=None, build_dir='', filter_glob='', timeout=None) -> Iterator[str]: + if not args: + args = [] args.extend(['mem=1G', 'console=tty']) if filter_glob: args.append('kunit.filter_glob='+filter_glob)