diff mbox series

[1/3] kunit: tool: remove dead parse_crash_in_log() logic

Message ID 20220426173334.3871399-1-dlatypov@google.com (mailing list archive)
State Accepted
Commit 33d4a933e9273bb9b33db8dcd0e564881319443c
Delegated to: Brendan Higgins
Headers show
Series [1/3] kunit: tool: remove dead parse_crash_in_log() logic | expand

Commit Message

Daniel Latypov April 26, 2022, 5:33 p.m. UTC 
This logic depends on the kernel logging a message containing
'kunit test case crashed', but there is no corresponding logic to do so.

This is likely a relic of the revision process KUnit initially went
through when being upstreamed.

Delete it given
1) it's been missing for years and likely won't get implemented
2) the parser has been moving to be a more general KTAP parser,
   kunit-only magic like this isn't how we'd want to implement it.

Signed-off-by: Daniel Latypov <dlatypov@google.com>
---
 tools/testing/kunit/kunit_parser.py           | 21 ------
 tools/testing/kunit/kunit_tool_test.py        | 17 ++---
 .../test_data/test_is_test_passed-crash.log   | 70 -------------------
 3 files changed, 4 insertions(+), 104 deletions(-)
 delete mode 100644 tools/testing/kunit/test_data/test_is_test_passed-crash.log


base-commit: 59729170afcd4900e08997a482467ffda8d88c7f

Comments

David Gow April 28, 2022, 7:11 a.m. UTC | #1 
On Wed, Apr 27, 2022 at 1:33 AM Daniel Latypov <dlatypov@google.com> wrote:
>
> This logic depends on the kernel logging a message containing
> 'kunit test case crashed', but there is no corresponding logic to do so.
>
> This is likely a relic of the revision process KUnit initially went
> through when being upstreamed.
>
> Delete it given
> 1) it's been missing for years and likely won't get implemented
> 2) the parser has been moving to be a more general KTAP parser,
>    kunit-only magic like this isn't how we'd want to implement it.
>
> Signed-off-by: Daniel Latypov <dlatypov@google.com>
> ---

Makes sense, thanks!

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

-- David

>  tools/testing/kunit/kunit_parser.py           | 21 ------
>  tools/testing/kunit/kunit_tool_test.py        | 17 ++---
>  .../test_data/test_is_test_passed-crash.log   | 70 -------------------
>  3 files changed, 4 insertions(+), 104 deletions(-)
>  delete mode 100644 tools/testing/kunit/test_data/test_is_test_passed-crash.log
>
> diff --git a/tools/testing/kunit/kunit_parser.py b/tools/testing/kunit/kunit_parser.py
> index 807ed2bd6832..7a0faf527a98 100644
> --- a/tools/testing/kunit/kunit_parser.py
> +++ b/tools/testing/kunit/kunit_parser.py
> @@ -475,26 +475,6 @@ def parse_diagnostic(lines: LineStream) -> List[str]:
>                 log.append(lines.pop())
>         return log
>
> -DIAGNOSTIC_CRASH_MESSAGE = re.compile(r'^# .*?: kunit test case crashed!$')
> -
> -def parse_crash_in_log(test: Test) -> bool:
> -       """
> -       Iterate through the lines of the log to parse for crash message.
> -       If crash message found, set status to crashed and return True.
> -       Otherwise return False.
> -
> -       Parameters:
> -       test - Test object for current test being parsed
> -
> -       Return:
> -       True if crash message found in log
> -       """
> -       for line in test.log:
> -               if DIAGNOSTIC_CRASH_MESSAGE.match(line):
> -                       test.status = TestStatus.TEST_CRASHED
> -                       return True
> -       return False
> -
>
>  # Printing helper methods:
>
> @@ -682,7 +662,6 @@ def bubble_up_test_results(test: Test) -> None:
>         Parameters:
>         test - Test object for current test being parsed
>         """
> -       parse_crash_in_log(test)
>         subtests = test.subtests
>         counts = test.counts
>         status = test.status
> diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py
> index 210df0f443e6..1200e451c418 100755
> --- a/tools/testing/kunit/kunit_tool_test.py
> +++ b/tools/testing/kunit/kunit_tool_test.py
> @@ -230,15 +230,6 @@ class KUnitParserTest(unittest.TestCase):
>                 print_mock.stop()
>                 self.assertEqual(0, len(result.subtests))
>
> -       def test_crashed_test(self):
> -               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(
> -                       kunit_parser.TestStatus.TEST_CRASHED,
> -                       result.status)
> -
>         def test_skipped_test(self):
>                 skipped_log = test_data_path('test_skip_tests.log')
>                 with open(skipped_log) as file:
> @@ -478,10 +469,10 @@ class KUnitJsonTest(unittest.TestCase):
>                         result["sub_groups"][1]["test_cases"][0])
>
>         def test_crashed_test_json(self):
> -               result = self._json_for('test_is_test_passed-crash.log')
> +               result = self._json_for('test_kernel_panic_interrupt.log')
>                 self.assertEqual(
> -                       {'name': 'example_simple_test', 'status': 'ERROR'},
> -                       result["sub_groups"][1]["test_cases"][0])
> +                       {'name': '', 'status': 'ERROR'},
> +                       result["sub_groups"][2]["test_cases"][1])
>
>         def test_skipped_test_json(self):
>                 result = self._json_for('test_skip_tests.log')
> @@ -562,7 +553,7 @@ class KUnitMainTest(unittest.TestCase):
>         def test_exec_no_tests(self):
>                 self.linux_source_mock.run_kernel = mock.Mock(return_value=['TAP version 14', '1..0'])
>                 with self.assertRaises(SystemExit) as e:
> -                  kunit.main(['run'], self.linux_source_mock)
> +                 kunit.main(['run'], self.linux_source_mock)

Noting that this is just an indentation fix.


>                 self.linux_source_mock.run_kernel.assert_called_once_with(
>                         args=None, build_dir='.kunit', filter_glob='', timeout=300)
>                 self.print_mock.assert_any_call(StrContains(' 0 tests run!'))
> diff --git a/tools/testing/kunit/test_data/test_is_test_passed-crash.log b/tools/testing/kunit/test_data/test_is_test_passed-crash.log
> deleted file mode 100644
> index 4d97f6708c4a..000000000000
> --- a/tools/testing/kunit/test_data/test_is_test_passed-crash.log
> +++ /dev/null
> @@ -1,70 +0,0 @@
> -printk: console [tty0] enabled
> -printk: console [mc-1] enabled
> -TAP version 14
> -1..2
> -       # Subtest: sysctl_test
> -       1..8
> -       # sysctl_test_dointvec_null_tbl_data: sysctl_test_dointvec_null_tbl_data passed
> -       ok 1 - sysctl_test_dointvec_null_tbl_data
> -       # sysctl_test_dointvec_table_maxlen_unset: sysctl_test_dointvec_table_maxlen_unset passed
> -       ok 2 - sysctl_test_dointvec_table_maxlen_unset
> -       # sysctl_test_dointvec_table_len_is_zero: sysctl_test_dointvec_table_len_is_zero passed
> -       ok 3 - sysctl_test_dointvec_table_len_is_zero
> -       # sysctl_test_dointvec_table_read_but_position_set: sysctl_test_dointvec_table_read_but_position_set passed
> -       ok 4 - sysctl_test_dointvec_table_read_but_position_set
> -       # sysctl_test_dointvec_happy_single_positive: sysctl_test_dointvec_happy_single_positive passed
> -       ok 5 - sysctl_test_dointvec_happy_single_positive
> -       # sysctl_test_dointvec_happy_single_negative: sysctl_test_dointvec_happy_single_negative passed
> -       ok 6 - sysctl_test_dointvec_happy_single_negative
> -       # sysctl_test_dointvec_single_less_int_min: sysctl_test_dointvec_single_less_int_min passed
> -       ok 7 - sysctl_test_dointvec_single_less_int_min
> -       # sysctl_test_dointvec_single_greater_int_max: sysctl_test_dointvec_single_greater_int_max passed
> -       ok 8 - sysctl_test_dointvec_single_greater_int_max
> -kunit sysctl_test: all tests passed
> -ok 1 - sysctl_test
> -       # Subtest: example
> -       1..2
> -init_suite
> -       # example_simple_test: initializing
> -Stack:
> - 6016f7db 6f81bd30 6f81bdd0 60021450
> - 6024b0e8 60021440 60018bbe 16f81bdc0
> - 00000001 6f81bd30 6f81bd20 6f81bdd0
> -Call Trace:
> - [<6016f7db>] ? kunit_try_run_case+0xab/0xf0
> - [<60021450>] ? set_signals+0x0/0x60
> - [<60021440>] ? get_signals+0x0/0x10
> - [<60018bbe>] ? kunit_um_run_try_catch+0x5e/0xc0
> - [<60021450>] ? set_signals+0x0/0x60
> - [<60021440>] ? get_signals+0x0/0x10
> - [<60018bb3>] ? kunit_um_run_try_catch+0x53/0xc0
> - [<6016f321>] ? kunit_run_case_catch_errors+0x121/0x1a0
> - [<60018b60>] ? kunit_um_run_try_catch+0x0/0xc0
> - [<600189e0>] ? kunit_um_throw+0x0/0x180
> - [<6016f730>] ? kunit_try_run_case+0x0/0xf0
> - [<6016f600>] ? kunit_catch_run_case+0x0/0x130
> - [<6016edd0>] ? kunit_vprintk+0x0/0x30
> - [<6016ece0>] ? kunit_fail+0x0/0x40
> - [<6016eca0>] ? kunit_abort+0x0/0x40
> - [<6016ed20>] ? kunit_printk_emit+0x0/0xb0
> - [<6016f200>] ? kunit_run_case_catch_errors+0x0/0x1a0
> - [<6016f46e>] ? kunit_run_tests+0xce/0x260
> - [<6005b390>] ? unregister_console+0x0/0x190
> - [<60175b70>] ? suite_kunit_initexample_test_suite+0x0/0x20
> - [<60001cbb>] ? do_one_initcall+0x0/0x197
> - [<60001d47>] ? do_one_initcall+0x8c/0x197
> - [<6005cd20>] ? irq_to_desc+0x0/0x30
> - [<60002005>] ? kernel_init_freeable+0x1b3/0x272
> - [<6005c5ec>] ? printk+0x0/0x9b
> - [<601c0086>] ? kernel_init+0x26/0x160
> - [<60014442>] ? new_thread_handler+0x82/0xc0
> -
> -       # example_simple_test: kunit test case crashed!
> -       # example_simple_test: example_simple_test failed
> -       not ok 1 - example_simple_test
> -       # example_mock_test: initializing
> -       # example_mock_test: example_mock_test passed
> -       ok 2 - example_mock_test
> -kunit example: one or more tests failed
> -not ok 2 - example
> -List of all partitions:
>
> base-commit: 59729170afcd4900e08997a482467ffda8d88c7f
> --
> 2.36.0.rc2.479.g8af0fa9b8e-goog
>
Brendan Higgins May 12, 2022, 5:49 p.m. UTC | #2 
On Tue, Apr 26, 2022 at 1:33 PM Daniel Latypov <dlatypov@google.com> wrote:
>
> This logic depends on the kernel logging a message containing
> 'kunit test case crashed', but there is no corresponding logic to do so.
>
> This is likely a relic of the revision process KUnit initially went
> through when being upstreamed.
>
> Delete it given
> 1) it's been missing for years and likely won't get implemented
> 2) the parser has been moving to be a more general KTAP parser,
>    kunit-only magic like this isn't how we'd want to implement it.
>
> Signed-off-by: Daniel Latypov <dlatypov@google.com>

Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
diff mbox series

Patch

diff --git a/tools/testing/kunit/kunit_parser.py b/tools/testing/kunit/kunit_parser.py
index 807ed2bd6832..7a0faf527a98 100644
--- a/tools/testing/kunit/kunit_parser.py
+++ b/tools/testing/kunit/kunit_parser.py
@@ -475,26 +475,6 @@  def parse_diagnostic(lines: LineStream) -> List[str]:
 		log.append(lines.pop())
 	return log
 
-DIAGNOSTIC_CRASH_MESSAGE = re.compile(r'^# .*?: kunit test case crashed!$')
-
-def parse_crash_in_log(test: Test) -> bool:
-	"""
-	Iterate through the lines of the log to parse for crash message.
-	If crash message found, set status to crashed and return True.
-	Otherwise return False.
-
-	Parameters:
-	test - Test object for current test being parsed
-
-	Return:
-	True if crash message found in log
-	"""
-	for line in test.log:
-		if DIAGNOSTIC_CRASH_MESSAGE.match(line):
-			test.status = TestStatus.TEST_CRASHED
-			return True
-	return False
-
 
 # Printing helper methods:
 
@@ -682,7 +662,6 @@  def bubble_up_test_results(test: Test) -> None:
 	Parameters:
 	test - Test object for current test being parsed
 	"""
-	parse_crash_in_log(test)
 	subtests = test.subtests
 	counts = test.counts
 	status = test.status
diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py
index 210df0f443e6..1200e451c418 100755
--- a/tools/testing/kunit/kunit_tool_test.py
+++ b/tools/testing/kunit/kunit_tool_test.py
@@ -230,15 +230,6 @@  class KUnitParserTest(unittest.TestCase):
 		print_mock.stop()
 		self.assertEqual(0, len(result.subtests))
 
-	def test_crashed_test(self):
-		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(
-			kunit_parser.TestStatus.TEST_CRASHED,
-			result.status)
-
 	def test_skipped_test(self):
 		skipped_log = test_data_path('test_skip_tests.log')
 		with open(skipped_log) as file:
@@ -478,10 +469,10 @@  class KUnitJsonTest(unittest.TestCase):
 			result["sub_groups"][1]["test_cases"][0])
 
 	def test_crashed_test_json(self):
-		result = self._json_for('test_is_test_passed-crash.log')
+		result = self._json_for('test_kernel_panic_interrupt.log')
 		self.assertEqual(
-			{'name': 'example_simple_test', 'status': 'ERROR'},
-			result["sub_groups"][1]["test_cases"][0])
+			{'name': '', 'status': 'ERROR'},
+			result["sub_groups"][2]["test_cases"][1])
 
 	def test_skipped_test_json(self):
 		result = self._json_for('test_skip_tests.log')
@@ -562,7 +553,7 @@  class KUnitMainTest(unittest.TestCase):
 	def test_exec_no_tests(self):
 		self.linux_source_mock.run_kernel = mock.Mock(return_value=['TAP version 14', '1..0'])
 		with self.assertRaises(SystemExit) as e:
-                  kunit.main(['run'], self.linux_source_mock)
+		  kunit.main(['run'], self.linux_source_mock)
 		self.linux_source_mock.run_kernel.assert_called_once_with(
 			args=None, build_dir='.kunit', filter_glob='', timeout=300)
 		self.print_mock.assert_any_call(StrContains(' 0 tests run!'))
diff --git a/tools/testing/kunit/test_data/test_is_test_passed-crash.log b/tools/testing/kunit/test_data/test_is_test_passed-crash.log
deleted file mode 100644
index 4d97f6708c4a..000000000000
--- a/tools/testing/kunit/test_data/test_is_test_passed-crash.log
+++ /dev/null
@@ -1,70 +0,0 @@ 
-printk: console [tty0] enabled
-printk: console [mc-1] enabled
-TAP version 14
-1..2
-	# Subtest: sysctl_test
-	1..8
-	# sysctl_test_dointvec_null_tbl_data: sysctl_test_dointvec_null_tbl_data passed
-	ok 1 - sysctl_test_dointvec_null_tbl_data
-	# sysctl_test_dointvec_table_maxlen_unset: sysctl_test_dointvec_table_maxlen_unset passed
-	ok 2 - sysctl_test_dointvec_table_maxlen_unset
-	# sysctl_test_dointvec_table_len_is_zero: sysctl_test_dointvec_table_len_is_zero passed
-	ok 3 - sysctl_test_dointvec_table_len_is_zero
-	# sysctl_test_dointvec_table_read_but_position_set: sysctl_test_dointvec_table_read_but_position_set passed
-	ok 4 - sysctl_test_dointvec_table_read_but_position_set
-	# sysctl_test_dointvec_happy_single_positive: sysctl_test_dointvec_happy_single_positive passed
-	ok 5 - sysctl_test_dointvec_happy_single_positive
-	# sysctl_test_dointvec_happy_single_negative: sysctl_test_dointvec_happy_single_negative passed
-	ok 6 - sysctl_test_dointvec_happy_single_negative
-	# sysctl_test_dointvec_single_less_int_min: sysctl_test_dointvec_single_less_int_min passed
-	ok 7 - sysctl_test_dointvec_single_less_int_min
-	# sysctl_test_dointvec_single_greater_int_max: sysctl_test_dointvec_single_greater_int_max passed
-	ok 8 - sysctl_test_dointvec_single_greater_int_max
-kunit sysctl_test: all tests passed
-ok 1 - sysctl_test
-	# Subtest: example
-	1..2
-init_suite
-	# example_simple_test: initializing
-Stack:
- 6016f7db 6f81bd30 6f81bdd0 60021450
- 6024b0e8 60021440 60018bbe 16f81bdc0
- 00000001 6f81bd30 6f81bd20 6f81bdd0
-Call Trace:
- [<6016f7db>] ? kunit_try_run_case+0xab/0xf0
- [<60021450>] ? set_signals+0x0/0x60
- [<60021440>] ? get_signals+0x0/0x10
- [<60018bbe>] ? kunit_um_run_try_catch+0x5e/0xc0
- [<60021450>] ? set_signals+0x0/0x60
- [<60021440>] ? get_signals+0x0/0x10
- [<60018bb3>] ? kunit_um_run_try_catch+0x53/0xc0
- [<6016f321>] ? kunit_run_case_catch_errors+0x121/0x1a0
- [<60018b60>] ? kunit_um_run_try_catch+0x0/0xc0
- [<600189e0>] ? kunit_um_throw+0x0/0x180
- [<6016f730>] ? kunit_try_run_case+0x0/0xf0
- [<6016f600>] ? kunit_catch_run_case+0x0/0x130
- [<6016edd0>] ? kunit_vprintk+0x0/0x30
- [<6016ece0>] ? kunit_fail+0x0/0x40
- [<6016eca0>] ? kunit_abort+0x0/0x40
- [<6016ed20>] ? kunit_printk_emit+0x0/0xb0
- [<6016f200>] ? kunit_run_case_catch_errors+0x0/0x1a0
- [<6016f46e>] ? kunit_run_tests+0xce/0x260
- [<6005b390>] ? unregister_console+0x0/0x190
- [<60175b70>] ? suite_kunit_initexample_test_suite+0x0/0x20
- [<60001cbb>] ? do_one_initcall+0x0/0x197
- [<60001d47>] ? do_one_initcall+0x8c/0x197
- [<6005cd20>] ? irq_to_desc+0x0/0x30
- [<60002005>] ? kernel_init_freeable+0x1b3/0x272
- [<6005c5ec>] ? printk+0x0/0x9b
- [<601c0086>] ? kernel_init+0x26/0x160
- [<60014442>] ? new_thread_handler+0x82/0xc0
-
-	# example_simple_test: kunit test case crashed!
-	# example_simple_test: example_simple_test failed
-	not ok 1 - example_simple_test
-	# example_mock_test: initializing
-	# example_mock_test: example_mock_test passed
-	ok 2 - example_mock_test
-kunit example: one or more tests failed
-not ok 2 - example
-List of all partitions: