[PATCH 1/5] kunit: tool: fix unit test cleanup handling

[PATCH 1/5] kunit: tool: fix unit test cleanup handling

[Daniel Latypov]

* 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 <dlatypov@google.com>
---
 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 497ab51bc170..3fbe1acd531a 100755
--- a/tools/testing/kunit/kunit_tool_test.py
+++ b/tools/testing/kunit/kunit_tool_test.py
@@ -288,19 +288,17 @@ class StrContains(str):
 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

base-commit: b65054597872ce3aefbc6a666385eabdf9e288da
-- 
2.29.2.454.gaff20da3a2-goog

[PATCH 2/5] kunit: tool: fix unit test so it can run from non-root dir

[Daniel Latypov]

get_absolute_path() makes an attempt to allow for this.
But that doesn't work as soon as os.chdir() gets called.

So make it so that os.chdir() does nothing to avoid this.

Note: mock.patch.object() doesn't seem to work in setUpModule(), hence
the introduction of a new base class instead.

Fixes: 5578d008d9e0 ("kunit: tool: fix running kunit_tool from outside kernel tree")
Signed-off-by: Daniel Latypov <dlatypov@google.com>
---
 tools/testing/kunit/kunit_tool_test.py | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py
index 3fbe1acd531a..9f1f1e1b772a 100755
--- a/tools/testing/kunit/kunit_tool_test.py
+++ b/tools/testing/kunit/kunit_tool_test.py
@@ -32,7 +32,13 @@ def tearDownModule():
 def get_absolute_path(path):
 	return os.path.join(os.path.dirname(__file__), path)
 
-class KconfigTest(unittest.TestCase):
+class KUnitTest(unittest.TestCase):
+	"""Contains common setup, like stopping main() from calling chdir."""
+	def setUp(self):
+		mock.patch.object(os, 'chdir').start()
+		self.addCleanup(mock.patch.stopall)
+
+class KconfigTest(KUnitTest):
 
 	def test_is_subset_of(self):
 		kconfig0 = kunit_config.Kconfig()
@@ -88,7 +94,7 @@ class KconfigTest(unittest.TestCase):
 		self.assertEqual(actual_kconfig.entries(),
 				 expected_kconfig.entries())
 
-class KUnitParserTest(unittest.TestCase):
+class KUnitParserTest(KUnitTest):
 
 	def assertContains(self, needle, haystack):
 		for line in haystack:
@@ -250,7 +256,7 @@ class KUnitParserTest(unittest.TestCase):
 				result.status)
 			self.assertEqual('kunit-resource-test', result.suites[0].name)
 
-class KUnitJsonTest(unittest.TestCase):
+class KUnitJsonTest(KUnitTest):
 
 	def _json_for(self, log_file):
 		with(open(get_absolute_path(log_file))) as file:
@@ -285,8 +291,9 @@ class StrContains(str):
 	def __eq__(self, other):
 		return self in other
 
-class KUnitMainTest(unittest.TestCase):
+class KUnitMainTest(KUnitTest):
 	def setUp(self):
+		super().setUp()
 		path = get_absolute_path('test_data/test_is_test_passed-all_passed.log')
 		with open(path) as file:
 			all_passed_log = file.readlines()
-- 
2.29.2.454.gaff20da3a2-goog

[PATCH 3/5] kunit: tool: stop using bare asserts in unit test

[Daniel Latypov]

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 <dlatypov@google.com>
---
 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 9f1f1e1b772a..e527ce9d3295 100755
--- a/tools/testing/kunit/kunit_tool_test.py
+++ b/tools/testing/kunit/kunit_tool_test.py
@@ -308,26 +308,26 @@ class KUnitMainTest(KUnitTest):
 
 	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.'))
@@ -336,35 +336,33 @@ class KUnitMainTest(KUnitTest):
 		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
@@ -375,7 +373,7 @@ class KUnitMainTest(KUnitTest):
 	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.'))
@@ -383,7 +381,7 @@ class KUnitMainTest(KUnitTest):
 	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.'))
@@ -391,7 +389,7 @@ class KUnitMainTest(KUnitTest):
 	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'
-- 
2.29.2.454.gaff20da3a2-goog

[PATCH 4/5] kunit: tool: use `with open()` in unit test

[Daniel Latypov]

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 <dlatypov@google.com>
---
 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 e527ce9d3295..f2c12d96caa1 100755
--- a/tools/testing/kunit/kunit_tool_test.py
+++ b/tools/testing/kunit/kunit_tool_test.py
@@ -106,15 +106,14 @@ class KUnitParserTest(KUnitTest):
 	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(
@@ -149,42 +148,39 @@ class KUnitParserTest(KUnitTest):
 	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()
@@ -192,12 +188,11 @@ class KUnitParserTest(KUnitTest):
 	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(
-- 
2.29.2.454.gaff20da3a2-goog

[PATCH 5/5] minor: kunit: tool: s/get_absolute_path/test_data_path in unit test

[Daniel Latypov]

1. the name is a lie. It gives relative paths, e.g. if I run from the
same dir as the test file, it gives './test_data/<file>'

2. it's only used for generating paths to tools/testing/kunit/test_data/
So we can tersen things by making it less general.

Signed-off-by: Daniel Latypov <dlatypov@google.com>
---
 tools/testing/kunit/kunit_tool_test.py | 56 ++++++++++----------------
 1 file changed, 21 insertions(+), 35 deletions(-)

diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py
index f2c12d96caa1..fbd8cd974554 100755
--- a/tools/testing/kunit/kunit_tool_test.py
+++ b/tools/testing/kunit/kunit_tool_test.py
@@ -29,8 +29,8 @@ def setUpModule():
 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(os.path.dirname(__file__), 'test_data', path)
 
 class KUnitTest(unittest.TestCase):
 	"""Contains common setup, like stopping main() from calling chdir."""
@@ -52,8 +52,7 @@ class KconfigTest(KUnitTest):
 
 	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)
 
@@ -104,8 +103,7 @@ class KUnitParserTest(KUnitTest):
 			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)
@@ -116,8 +114,7 @@ class KUnitParserTest(KUnitTest):
 		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)
@@ -146,8 +143,7 @@ class KUnitParserTest(KUnitTest):
 		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(
@@ -155,8 +151,7 @@ class KUnitParserTest(KUnitTest):
 			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(
@@ -164,8 +159,7 @@ class KUnitParserTest(KUnitTest):
 			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()))
@@ -175,8 +169,7 @@ class KUnitParserTest(KUnitTest):
 			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(
@@ -186,8 +179,7 @@ class KUnitParserTest(KUnitTest):
 		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(
@@ -195,8 +187,7 @@ class KUnitParserTest(KUnitTest):
 			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(
@@ -205,8 +196,7 @@ class KUnitParserTest(KUnitTest):
 			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(
@@ -215,8 +205,7 @@ class KUnitParserTest(KUnitTest):
 			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(
@@ -225,7 +214,7 @@ class KUnitParserTest(KUnitTest):
 			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(
@@ -234,7 +223,7 @@ class KUnitParserTest(KUnitTest):
 			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(
@@ -243,7 +232,7 @@ class KUnitParserTest(KUnitTest):
 			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(
@@ -254,7 +243,7 @@ class KUnitParserTest(KUnitTest):
 class KUnitJsonTest(KUnitTest):
 
 	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,
@@ -264,22 +253,19 @@ class KUnitJsonTest(KUnitTest):
 		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):
@@ -289,7 +275,7 @@ class StrContains(str):
 class KUnitMainTest(KUnitTest):
 	def setUp(self):
 		super().setUp()
-		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()
 
-- 
2.29.2.454.gaff20da3a2-goog

Re: [PATCH 1/5] kunit: tool: fix unit test cleanup handling

[David Gow]

On Tue, Dec 1, 2020 at 7:32 AM Daniel Latypov <dlatypov@google.com> wrote:
>
> * 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 <dlatypov@google.com>
> ---

I won't pretend to be an expert on Python, but this seems good to me.
I tested it on my machine and it works fine.

So,
Reviewed-by: David Gow <davidgow@google.com>

-- Davkd

>  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 497ab51bc170..3fbe1acd531a 100755
> --- a/tools/testing/kunit/kunit_tool_test.py
> +++ b/tools/testing/kunit/kunit_tool_test.py
> @@ -288,19 +288,17 @@ class StrContains(str):
>  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
>
> base-commit: b65054597872ce3aefbc6a666385eabdf9e288da
> --
> 2.29.2.454.gaff20da3a2-goog
>

Re: [PATCH 2/5] kunit: tool: fix unit test so it can run from non-root dir

[David Gow]

On Tue, Dec 1, 2020 at 7:33 AM Daniel Latypov <dlatypov@google.com> wrote:
>
> get_absolute_path() makes an attempt to allow for this.
> But that doesn't work as soon as os.chdir() gets called.

Can we explain why this doesn't work? It's because the test_data/
files are accessed with relative paths, so chdir breaks access to
them, right?

>
> So make it so that os.chdir() does nothing to avoid this.
>
> Note: mock.patch.object() doesn't seem to work in setUpModule(), hence
> the introduction of a new base class instead.
>
> Fixes: 5578d008d9e0 ("kunit: tool: fix running kunit_tool from outside kernel tree")
> Signed-off-by: Daniel Latypov <dlatypov@google.com>
> ---

I don't like this: disabling a real system call seems like overkill to
work around a path issue like this.

Wouldn't it be better to either:
(a) stop kunit_tool from needing to chdir entirely; or
(b) have get_absolute_path() / test_data_path() produce an absolute path.

The latter really seems like the most sensible approach: have the
script read its own path and read files relative to that.

Cheers,
-- David


>  tools/testing/kunit/kunit_tool_test.py | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py
> index 3fbe1acd531a..9f1f1e1b772a 100755
> --- a/tools/testing/kunit/kunit_tool_test.py
> +++ b/tools/testing/kunit/kunit_tool_test.py
> @@ -32,7 +32,13 @@ def tearDownModule():
>  def get_absolute_path(path):
>         return os.path.join(os.path.dirname(__file__), path)
>
> -class KconfigTest(unittest.TestCase):
> +class KUnitTest(unittest.TestCase):
> +       """Contains common setup, like stopping main() from calling chdir."""
> +       def setUp(self):
> +               mock.patch.object(os, 'chdir').start()
> +               self.addCleanup(mock.patch.stopall)
> +
> +class KconfigTest(KUnitTest):
>
>         def test_is_subset_of(self):
>                 kconfig0 = kunit_config.Kconfig()
> @@ -88,7 +94,7 @@ class KconfigTest(unittest.TestCase):
>                 self.assertEqual(actual_kconfig.entries(),
>                                  expected_kconfig.entries())
>
> -class KUnitParserTest(unittest.TestCase):
> +class KUnitParserTest(KUnitTest):
>
>         def assertContains(self, needle, haystack):
>                 for line in haystack:
> @@ -250,7 +256,7 @@ class KUnitParserTest(unittest.TestCase):
>                                 result.status)
>                         self.assertEqual('kunit-resource-test', result.suites[0].name)
>
> -class KUnitJsonTest(unittest.TestCase):
> +class KUnitJsonTest(KUnitTest):
>
>         def _json_for(self, log_file):
>                 with(open(get_absolute_path(log_file))) as file:
> @@ -285,8 +291,9 @@ class StrContains(str):
>         def __eq__(self, other):
>                 return self in other
>
> -class KUnitMainTest(unittest.TestCase):
> +class KUnitMainTest(KUnitTest):
>         def setUp(self):
> +               super().setUp()
>                 path = get_absolute_path('test_data/test_is_test_passed-all_passed.log')
>                 with open(path) as file:
>                         all_passed_log = file.readlines()
> --
> 2.29.2.454.gaff20da3a2-goog
>

Re: [PATCH 3/5] kunit: tool: stop using bare asserts in unit test

[David Gow]

On Tue, Dec 1, 2020 at 7:33 AM Daniel Latypov <dlatypov@google.com> wrote:
>
> 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 <dlatypov@google.com>
> ---

This works for me, and seems pretty sensible from my rudimentary
python knowledge.

Reviewed-by: David Gow <davidgow@google.com>

Cheers,
-- David

Re: [PATCH 4/5] kunit: tool: use `with open()` in unit test

[David Gow]

On Tue, Dec 1, 2020 at 7:33 AM Daniel Latypov <dlatypov@google.com> wrote:
>
> 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 <dlatypov@google.com>
> ---
I'm fine with this, and it clearly works fine for me. Out of
curiosity, though, is there any difference here other than it being
more usual Python style?

We've struggled a bit in the past toeing a line between trying to
follow "normal" Python style versus adapting it a bit to be more
"kernel-y". Experience thus far has actually been that going out on
our own has caused more problems than it solves, so I'm all for this
change, but I do admit that my brain does understand the older code a
touch more easily.

In any case,
Reviewed-by: David Gow <davidgow@google.com>


-- David

Re: [PATCH 5/5] minor: kunit: tool: s/get_absolute_path/test_data_path in unit test

[David Gow]

On Tue, Dec 1, 2020 at 7:33 AM Daniel Latypov <dlatypov@google.com> wrote:
>
> 1. the name is a lie. It gives relative paths, e.g. if I run from the
> same dir as the test file, it gives './test_data/<file>'
>
> 2. it's only used for generating paths to tools/testing/kunit/test_data/
> So we can tersen things by making it less general.
>
> Signed-off-by: Daniel Latypov <dlatypov@google.com>
> ---
This is an excellent and overdue rename/replacement.

My only note is re: the concerns I have in patch 2, where I think we
probably ought to make this function actually return an absolute path.
It seems from the code (and the function name) that that was the
intent, so if we can fix it, that'd be ideal.

Personally, though, I'd still prefer the new test_data_path(), just
have it be an actually absolute path.


-- David

Re: [PATCH 4/5] kunit: tool: use `with open()` in unit test

[Daniel Latypov]

On Mon, Nov 30, 2020 at 11:33 PM David Gow <davidgow@google.com> wrote:
>
> On Tue, Dec 1, 2020 at 7:33 AM Daniel Latypov <dlatypov@google.com> wrote:
> >
> > 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 <dlatypov@google.com>
> > ---
> I'm fine with this, and it clearly works fine for me. Out of
> curiosity, though, is there any difference here other than it being
> more usual Python style?

Files aren't leaked if we error out due to some exception before we
call close().
And self.assertEqual() and friends raise exceptions on failure, so
we're toeing that line here.

But other than being more robust, it should be equivalent.

>
> We've struggled a bit in the past toeing a line between trying to
> follow "normal" Python style versus adapting it a bit to be more
> "kernel-y". Experience thus far has actually been that going out on
> our own has caused more problems than it solves, so I'm all for this
> change, but I do admit that my brain does understand the older code a
> touch more easily.

Ack. Python's lack of lexical scopes is a bit disconcerting.
But from a readability standpoint, it's a bit more self-evident than
having to write it would be, imo.
So I think following more standard Python style here outweighs the cost.

>
> In any case,
> Reviewed-by: David Gow <davidgow@google.com>
>
>
> -- David

Re: [PATCH 2/5] kunit: tool: fix unit test so it can run from non-root dir

[Daniel Latypov]

On Mon, Nov 30, 2020 at 11:33 PM David Gow <davidgow@google.com> wrote:
>
> On Tue, Dec 1, 2020 at 7:33 AM Daniel Latypov <dlatypov@google.com> wrote:
> >
> > get_absolute_path() makes an attempt to allow for this.
> > But that doesn't work as soon as os.chdir() gets called.
>
> Can we explain why this doesn't work? It's because the test_data/
> files are accessed with relative paths, so chdir breaks access to
> them, right?

Correct.
Because it actually returns a relative path.

(I forgot that I called out that get_absolute_path() gives relative
paths in the last patch, and not this one. I can update the commit
desc if we want a v2 of this)

>
> >
> > So make it so that os.chdir() does nothing to avoid this.
> >
> > Note: mock.patch.object() doesn't seem to work in setUpModule(), hence
> > the introduction of a new base class instead.
> >
> > Fixes: 5578d008d9e0 ("kunit: tool: fix running kunit_tool from outside kernel tree")
> > Signed-off-by: Daniel Latypov <dlatypov@google.com>
> > ---
>
> I don't like this: disabling a real system call seems like overkill to
> work around a path issue like this.
>
> Wouldn't it be better to either:
> (a) stop kunit_tool from needing to chdir entirely; or

a) is doable, but would require plumbing cwd=get_kernel_root_path() to
all the subprocess calls to make, etc.
I'm not sure fixing a internal test-only issue necessarily justifies
taking that path instead of the easier "just add a chdir" we opted for
in 5578d008d9e0 ("kunit: tool: fix running kunit_tool from outside
kernel tree").

> (b) have get_absolute_path() / test_data_path() produce an absolute path.
>
> The latter really seems like the most sensible approach: have the
> script read its own path and read files relative to that.

b) is not that simple, sadly.

Say I invoke
$ python3 kunit_tool_test.py
then __file__ = kunit_tool_test.py.

So __file__ is a relative path, but the code assumed it was an
absolute one and any change of directory breaks things.
Working around that would require something like caching the result of
os.path.abspath(__file__) somewhere.

I can see the point about not mocking out something like os.chdir().
But on the other hand, do we have any other legitimate reason to call
it besides that one place in kunit.py?
If we do have something that relies on doing an os.chdir(), it should
ideally notice that it didn't work and manifest in a unit test failure
somehow.

Alternatively, we could make get_kernel_root_path() return ""/None to
avoid the chdir call.
kunit.py:       if get_kernel_root_path():
kunit.py:               os.chdir(get_kernel_root_path())

There'd be no adverse affects atm for stubbing that out, and I don't forsee any.
But if we want to be even safer, then perhaps we have

def chdir_to_kernel_root():
   ...

and mock out just that func in the unit test?

>
> Cheers,
> -- David
>
>
> >  tools/testing/kunit/kunit_tool_test.py | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py
> > index 3fbe1acd531a..9f1f1e1b772a 100755
> > --- a/tools/testing/kunit/kunit_tool_test.py
> > +++ b/tools/testing/kunit/kunit_tool_test.py
> > @@ -32,7 +32,13 @@ def tearDownModule():
> >  def get_absolute_path(path):
> >         return os.path.join(os.path.dirname(__file__), path)
> >
> > -class KconfigTest(unittest.TestCase):
> > +class KUnitTest(unittest.TestCase):
> > +       """Contains common setup, like stopping main() from calling chdir."""
> > +       def setUp(self):
> > +               mock.patch.object(os, 'chdir').start()
> > +               self.addCleanup(mock.patch.stopall)
> > +
> > +class KconfigTest(KUnitTest):
> >
> >         def test_is_subset_of(self):
> >                 kconfig0 = kunit_config.Kconfig()
> > @@ -88,7 +94,7 @@ class KconfigTest(unittest.TestCase):
> >                 self.assertEqual(actual_kconfig.entries(),
> >                                  expected_kconfig.entries())
> >
> > -class KUnitParserTest(unittest.TestCase):
> > +class KUnitParserTest(KUnitTest):
> >
> >         def assertContains(self, needle, haystack):
> >                 for line in haystack:
> > @@ -250,7 +256,7 @@ class KUnitParserTest(unittest.TestCase):
> >                                 result.status)
> >                         self.assertEqual('kunit-resource-test', result.suites[0].name)
> >
> > -class KUnitJsonTest(unittest.TestCase):
> > +class KUnitJsonTest(KUnitTest):
> >
> >         def _json_for(self, log_file):
> >                 with(open(get_absolute_path(log_file))) as file:
> > @@ -285,8 +291,9 @@ class StrContains(str):
> >         def __eq__(self, other):
> >                 return self in other
> >
> > -class KUnitMainTest(unittest.TestCase):
> > +class KUnitMainTest(KUnitTest):
> >         def setUp(self):
> > +               super().setUp()
> >                 path = get_absolute_path('test_data/test_is_test_passed-all_passed.log')
> >                 with open(path) as file:
> >                         all_passed_log = file.readlines()
> > --
> > 2.29.2.454.gaff20da3a2-goog
> >

Re: [PATCH 2/5] kunit: tool: fix unit test so it can run from non-root dir

[David Gow]

On Wed, Dec 2, 2020 at 3:00 AM Daniel Latypov <dlatypov@google.com> wrote:
>
> On Mon, Nov 30, 2020 at 11:33 PM David Gow <davidgow@google.com> wrote:
> >
> > On Tue, Dec 1, 2020 at 7:33 AM Daniel Latypov <dlatypov@google.com> wrote:
> > >
> > > get_absolute_path() makes an attempt to allow for this.
> > > But that doesn't work as soon as os.chdir() gets called.
> >
> > Can we explain why this doesn't work? It's because the test_data/
> > files are accessed with relative paths, so chdir breaks access to
> > them, right?
>
> Correct.
> Because it actually returns a relative path.
>
> (I forgot that I called out that get_absolute_path() gives relative
> paths in the last patch, and not this one. I can update the commit
> desc if we want a v2 of this)
>
> >
> > >
> > > So make it so that os.chdir() does nothing to avoid this.
> > >
> > > Note: mock.patch.object() doesn't seem to work in setUpModule(), hence
> > > the introduction of a new base class instead.
> > >
> > > Fixes: 5578d008d9e0 ("kunit: tool: fix running kunit_tool from outside kernel tree")
> > > Signed-off-by: Daniel Latypov <dlatypov@google.com>
> > > ---
> >
> > I don't like this: disabling a real system call seems like overkill to
> > work around a path issue like this.
> >
> > Wouldn't it be better to either:
> > (a) stop kunit_tool from needing to chdir entirely; or
>
> a) is doable, but would require plumbing cwd=get_kernel_root_path() to
> all the subprocess calls to make, etc.
> I'm not sure fixing a internal test-only issue necessarily justifies
> taking that path instead of the easier "just add a chdir" we opted for
> in 5578d008d9e0 ("kunit: tool: fix running kunit_tool from outside
> kernel tree").
>
> > (b) have get_absolute_path() / test_data_path() produce an absolute path.
> >
> > The latter really seems like the most sensible approach: have the
> > script read its own path and read files relative to that.
>
> b) is not that simple, sadly.
>
> Say I invoke
> $ python3 kunit_tool_test.py
> then __file__ = kunit_tool_test.py.
>
> So __file__ is a relative path, but the code assumed it was an
> absolute one and any change of directory breaks things.
> Working around that would require something like caching the result of
> os.path.abspath(__file__) somewhere.

So, to clarify, __file__ is a relative path based on the cwd when the
script is initially run, right?

In any case, caching the result of os.path.abspath(__file__) seems
like the most sensible solution to me. There's global state anyway
(the current directory), we might as well have it in an explicit
variable, IMHO.
>
> I can see the point about not mocking out something like os.chdir().
> But on the other hand, do we have any other legitimate reason to call
> it besides that one place in kunit.py?
> If we do have something that relies on doing an os.chdir(), it should
> ideally notice that it didn't work and manifest in a unit test failure
> somehow.

Certainly there doesn't seem to be any other need to chdir() in
kunit_tool at the moment, but I could see us doing so when adding
other features. More to the point, if both kunit.py and
kunit_tool_test.py rely on or manipulate the current directory as part
of their state, that seems like it's asking for some trouble down the
line.

If we use an absolute path for the test data, that's something that
seems unlikely to ever need further changes or cause issues.
>
> Alternatively, we could make get_kernel_root_path() return ""/None to
> avoid the chdir call.
> kunit.py:       if get_kernel_root_path():
> kunit.py:               os.chdir(get_kernel_root_path())
>
> There'd be no adverse affects atm for stubbing that out, and I don't forsee any.
> But if we want to be even safer, then perhaps we have
>
> def chdir_to_kernel_root():
>    ...
>
> and mock out just that func in the unit test?

I'd be okay with this, even if I'd prefer us to use an absolute path
for the test_data as well. Having something like this might even give
us the opportunity to verify that we're actually trying to change to
the kernel directory in cases where we need to, but that seems like
it's out-of-scope for a simple fix.

-- David

Re: [PATCH 2/5] kunit: tool: fix unit test so it can run from non-root dir

[Daniel Latypov]

On Tue, Dec 1, 2020 at 8:41 PM David Gow <davidgow@google.com> wrote:
>
> On Wed, Dec 2, 2020 at 3:00 AM Daniel Latypov <dlatypov@google.com> wrote:
> >
> > On Mon, Nov 30, 2020 at 11:33 PM David Gow <davidgow@google.com> wrote:
> > >
> > > On Tue, Dec 1, 2020 at 7:33 AM Daniel Latypov <dlatypov@google.com> wrote:
> > > >
> > > > get_absolute_path() makes an attempt to allow for this.
> > > > But that doesn't work as soon as os.chdir() gets called.
> > >
> > > Can we explain why this doesn't work? It's because the test_data/
> > > files are accessed with relative paths, so chdir breaks access to
> > > them, right?
> >
> > Correct.
> > Because it actually returns a relative path.
> >
> > (I forgot that I called out that get_absolute_path() gives relative
> > paths in the last patch, and not this one. I can update the commit
> > desc if we want a v2 of this)
> >
> > >
> > > >
> > > > So make it so that os.chdir() does nothing to avoid this.
> > > >
> > > > Note: mock.patch.object() doesn't seem to work in setUpModule(), hence
> > > > the introduction of a new base class instead.
> > > >
> > > > Fixes: 5578d008d9e0 ("kunit: tool: fix running kunit_tool from outside kernel tree")
> > > > Signed-off-by: Daniel Latypov <dlatypov@google.com>
> > > > ---
> > >
> > > I don't like this: disabling a real system call seems like overkill to
> > > work around a path issue like this.
> > >
> > > Wouldn't it be better to either:
> > > (a) stop kunit_tool from needing to chdir entirely; or
> >
> > a) is doable, but would require plumbing cwd=get_kernel_root_path() to
> > all the subprocess calls to make, etc.
> > I'm not sure fixing a internal test-only issue necessarily justifies
> > taking that path instead of the easier "just add a chdir" we opted for
> > in 5578d008d9e0 ("kunit: tool: fix running kunit_tool from outside
> > kernel tree").
> >
> > > (b) have get_absolute_path() / test_data_path() produce an absolute path.
> > >
> > > The latter really seems like the most sensible approach: have the
> > > script read its own path and read files relative to that.
> >
> > b) is not that simple, sadly.
> >
> > Say I invoke
> > $ python3 kunit_tool_test.py
> > then __file__ = kunit_tool_test.py.
> >
> > So __file__ is a relative path, but the code assumed it was an
> > absolute one and any change of directory breaks things.
> > Working around that would require something like caching the result of
> > os.path.abspath(__file__) somewhere.
>
> So, to clarify, __file__ is a relative path based on the cwd when the
> script is initially run, right?

Yes, on my box at least.
https://docs.python.org/3/reference/import.html#__file__ doesn't not
seem to stipulate an absolute path, either.

>
> In any case, caching the result of os.path.abspath(__file__) seems
> like the most sensible solution to me. There's global state anyway
> (the current directory), we might as well have it in an explicit
> variable, IMHO.

Ok, sent out a v2 and squash this change with the test_data_path() patch.

Not really a fan of the adding the global state, but I see your point
about there maybe being need for more chdir calls and I don't see a
better way of keeping track of the absolute path.

> >
> > I can see the point about not mocking out something like os.chdir().
> > But on the other hand, do we have any other legitimate reason to call
> > it besides that one place in kunit.py?
> > If we do have something that relies on doing an os.chdir(), it should
> > ideally notice that it didn't work and manifest in a unit test failure
> > somehow.
>
> Certainly there doesn't seem to be any other need to chdir() in
> kunit_tool at the moment, but I could see us doing so when adding
> other features. More to the point, if both kunit.py and
> kunit_tool_test.py rely on or manipulate the current directory as part
> of their state, that seems like it's asking for some trouble down the
> line.
>
> If we use an absolute path for the test data, that's something that
> seems unlikely to ever need further changes or cause issues.
> >
> > Alternatively, we could make get_kernel_root_path() return ""/None to
> > avoid the chdir call.
> > kunit.py:       if get_kernel_root_path():
> > kunit.py:               os.chdir(get_kernel_root_path())
> >
> > There'd be no adverse affects atm for stubbing that out, and I don't forsee any.
> > But if we want to be even safer, then perhaps we have
> >
> > def chdir_to_kernel_root():
> >    ...
> >
> > and mock out just that func in the unit test?
>
> I'd be okay with this, even if I'd prefer us to use an absolute path
> for the test_data as well. Having something like this might even give
> us the opportunity to verify that we're actually trying to change to
> the kernel directory in cases where we need to, but that seems like
> it's out-of-scope for a simple fix.
>
> -- David