diff mbox series

kunit: tool: make --raw_output only support showing kunit output

Message ID 20210916223903.1592541-1-dlatypov@google.com
State New
Headers show
Series kunit: tool: make --raw_output only support showing kunit output | expand

Commit Message

Daniel Latypov Sept. 16, 2021, 10:39 p.m. UTC 
Commit 6a499c9c42d0 ("kunit: tool: make --raw_output support only
showing kunit output") made --raw_output a string-typed argument.
Passing --raw_output=kunit would make it only show KUnit-related output
and not everything.

However, converting it to a string-typed argument had side effects.

These calls used to work:
$ kunit.py run --raw_output
$ kunit.py run --raw_output suite_filter
$ kunit.py run suite_filter --raw_output

But now the second is actually parsed as
$ kunit.py run --raw_output=suite_filter

So the order you add in --raw_output now matters and command lines that
used to work might not anymore.

Change --raw_output back to a boolean flag, but change its behavior to
match that of the former --raw_output=kunit.
The assumption is that this is what most people wanted to see anyways.

To get the old behavior, users can simply do:
$ kunit.py run >/dev/null; cat .kunit/test.log
They don't have any easy way of getting the --raw_output=kunit behavior.

Signed-off-by: Daniel Latypov <dlatypov@google.com>
---

Meta: this is an alternative to
https://lore.kernel.org/linux-kselftest/20210903161405.1861312-1-dlatypov@google.com/

I'd slightly prefer that approach, but if we're fine with giving up the
old --raw_output semantics entirely, this would be cleaner.
I'd also assume that most people would prefer the new semantics, but I'm
not sure of that.

---
 Documentation/dev-tools/kunit/kunit-tool.rst |  7 -------
 tools/testing/kunit/kunit.py                 | 12 +++---------
 tools/testing/kunit/kunit_tool_test.py       | 13 ++++++-------
 3 files changed, 9 insertions(+), 23 deletions(-)


base-commit: 316346243be6df12799c0b64b788e06bad97c30b

Comments

David Gow Sept. 21, 2021, 4:19 p.m. UTC | #1 
On Fri, Sep 17, 2021 at 6:39 AM Daniel Latypov <dlatypov@google.com> wrote:
>

> Commit 6a499c9c42d0 ("kunit: tool: make --raw_output support only

> showing kunit output") made --raw_output a string-typed argument.

> Passing --raw_output=kunit would make it only show KUnit-related output

> and not everything.

>

> However, converting it to a string-typed argument had side effects.

>

> These calls used to work:

> $ kunit.py run --raw_output

> $ kunit.py run --raw_output suite_filter

> $ kunit.py run suite_filter --raw_output

>

> But now the second is actually parsed as

> $ kunit.py run --raw_output=suite_filter

>

> So the order you add in --raw_output now matters and command lines that

> used to work might not anymore.

>

> Change --raw_output back to a boolean flag, but change its behavior to

> match that of the former --raw_output=kunit.

> The assumption is that this is what most people wanted to see anyways.

>

> To get the old behavior, users can simply do:

> $ kunit.py run >/dev/null; cat .kunit/test.log

> They don't have any easy way of getting the --raw_output=kunit behavior.

>

> Signed-off-by: Daniel Latypov <dlatypov@google.com>

> ---

>

> Meta: this is an alternative to

> https://lore.kernel.org/linux-kselftest/20210903161405.1861312-1-dlatypov@google.com/

>

> I'd slightly prefer that approach, but if we're fine with giving up the

> old --raw_output semantics entirely, this would be cleaner.

> I'd also assume that most people would prefer the new semantics, but I'm

> not sure of that.

>

> ---


Thanks. I'm happy with either approach, but this is the one I properly
understand. If you'd rather push the other one, I agree that it's
better from a user perspective, so I'm okay with that: it's just a bit
beyond my comfort zone Python-hacks wise.

If we do go with this one, and I need the whole output, just running
the UML 'linux' binary is another option, which I've used in the past.
That's a bit trickier for qemu though: maybe there's some benefit in
having a --dry-run option for kunit.py run which just prints the
command used to execute the kernel. That's obviously beyond the scope
of this, though.

Regardless, this is
Reviewed-by: David Gow <davidgow@google.com>


Cheers,
-- David


>  Documentation/dev-tools/kunit/kunit-tool.rst |  7 -------

>  tools/testing/kunit/kunit.py                 | 12 +++---------

>  tools/testing/kunit/kunit_tool_test.py       | 13 ++++++-------

>  3 files changed, 9 insertions(+), 23 deletions(-)

>

> diff --git a/Documentation/dev-tools/kunit/kunit-tool.rst b/Documentation/dev-tools/kunit/kunit-tool.rst

> index ae52e0f489f9..03404746f1f6 100644

> --- a/Documentation/dev-tools/kunit/kunit-tool.rst

> +++ b/Documentation/dev-tools/kunit/kunit-tool.rst

> @@ -114,13 +114,6 @@ results in TAP format, you can pass the ``--raw_output`` argument.

>

>         ./tools/testing/kunit/kunit.py run --raw_output

>

> -The raw output from test runs may contain other, non-KUnit kernel log

> -lines. You can see just KUnit output with ``--raw_output=kunit``:

> -

> -.. code-block:: bash

> -

> -       ./tools/testing/kunit/kunit.py run --raw_output=kunit

> -

>  If you have KUnit results in their raw TAP format, you can parse them and print

>  the human-readable summary with the ``parse`` command for kunit_tool. This

>  accepts a filename for an argument, or will read from standard input.

> diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py

> index 5a931456e718..3626a56472b5 100755

> --- a/tools/testing/kunit/kunit.py

> +++ b/tools/testing/kunit/kunit.py

> @@ -115,13 +115,7 @@ def parse_tests(request: KunitParseRequest) -> KunitResult:

>                                               'Tests not Parsed.')

>

>         if request.raw_output:

> -               output: Iterable[str] = request.input_data

> -               if request.raw_output == 'all':

> -                       pass

> -               elif request.raw_output == 'kunit':

> -                       output = kunit_parser.extract_tap_lines(output)

> -               else:

> -                       print(f'Unknown --raw_output option "{request.raw_output}"', file=sys.stderr)

> +               output = kunit_parser.extract_tap_lines(request.input_data)

>                 for line in output:

>                         print(line.rstrip())

>

> @@ -256,8 +250,8 @@ def add_exec_opts(parser) -> None:

>

>  def add_parse_opts(parser) -> None:

>         parser.add_argument('--raw_output', help='If set don\'t format output from kernel. '

> -                           'If set to --raw_output=kunit, filters to just KUnit output.',

> -                           type=str, nargs='?', const='all', default=None)

> +                           'It will only show output from KUnit.',

> +                           action='store_true')

>         parser.add_argument('--json',

>                             nargs='?',

>                             help='Stores test results in a JSON, and either '

> diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py

> index 619c4554cbff..55ed3dac31ee 100755

> --- a/tools/testing/kunit/kunit_tool_test.py

> +++ b/tools/testing/kunit/kunit_tool_test.py

> @@ -399,14 +399,13 @@ class KUnitMainTest(unittest.TestCase):

>                         self.assertNotEqual(call, mock.call(StrContains('Testing complete.')))

>                         self.assertNotEqual(call, mock.call(StrContains(' 0 tests run')))

>

> -       def test_run_raw_output_kunit(self):

> +       def test_run_raw_output_does_not_take_positional_args(self):

> +               # --raw_output might eventually support an argument, but we don't want it

> +               # to consume any positional arguments, only ones after an '='.

>                 self.linux_source_mock.run_kernel = mock.Mock(return_value=[])

> -               kunit.main(['run', '--raw_output=kunit'], self.linux_source_mock)

> -               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')))

> +               kunit.main(['run', '--raw_output', 'filter_glob'], self.linux_source_mock)

> +               self.linux_source_mock.run_kernel.assert_called_once_with(

> +                       args=None, build_dir='.kunit', filter_glob='filter_glob', timeout=300)

>

>         def test_exec_timeout(self):

>                 timeout = 3453

>

> base-commit: 316346243be6df12799c0b64b788e06bad97c30b

> --

> 2.33.0.464.g1972c5931b-goog

>
diff mbox series

Patch

diff --git a/Documentation/dev-tools/kunit/kunit-tool.rst b/Documentation/dev-tools/kunit/kunit-tool.rst
index ae52e0f489f9..03404746f1f6 100644
--- a/Documentation/dev-tools/kunit/kunit-tool.rst
+++ b/Documentation/dev-tools/kunit/kunit-tool.rst
@@ -114,13 +114,6 @@  results in TAP format, you can pass the ``--raw_output`` argument.
 
 	./tools/testing/kunit/kunit.py run --raw_output
 
-The raw output from test runs may contain other, non-KUnit kernel log
-lines. You can see just KUnit output with ``--raw_output=kunit``:
-
-.. code-block:: bash
-
-	./tools/testing/kunit/kunit.py run --raw_output=kunit
-
 If you have KUnit results in their raw TAP format, you can parse them and print
 the human-readable summary with the ``parse`` command for kunit_tool. This
 accepts a filename for an argument, or will read from standard input.
diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py
index 5a931456e718..3626a56472b5 100755
--- a/tools/testing/kunit/kunit.py
+++ b/tools/testing/kunit/kunit.py
@@ -115,13 +115,7 @@  def parse_tests(request: KunitParseRequest) -> KunitResult:
 					      'Tests not Parsed.')
 
 	if request.raw_output:
-		output: Iterable[str] = request.input_data
-		if request.raw_output == 'all':
-			pass
-		elif request.raw_output == 'kunit':
-			output = kunit_parser.extract_tap_lines(output)
-		else:
-			print(f'Unknown --raw_output option "{request.raw_output}"', file=sys.stderr)
+		output = kunit_parser.extract_tap_lines(request.input_data)
 		for line in output:
 			print(line.rstrip())
 
@@ -256,8 +250,8 @@  def add_exec_opts(parser) -> None:
 
 def add_parse_opts(parser) -> None:
 	parser.add_argument('--raw_output', help='If set don\'t format output from kernel. '
-			    'If set to --raw_output=kunit, filters to just KUnit output.',
-			    type=str, nargs='?', const='all', default=None)
+			    'It will only show output from KUnit.',
+			    action='store_true')
 	parser.add_argument('--json',
 			    nargs='?',
 			    help='Stores test results in a JSON, and either '
diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py
index 619c4554cbff..55ed3dac31ee 100755
--- a/tools/testing/kunit/kunit_tool_test.py
+++ b/tools/testing/kunit/kunit_tool_test.py
@@ -399,14 +399,13 @@  class KUnitMainTest(unittest.TestCase):
 			self.assertNotEqual(call, mock.call(StrContains('Testing complete.')))
 			self.assertNotEqual(call, mock.call(StrContains(' 0 tests run')))
 
-	def test_run_raw_output_kunit(self):
+	def test_run_raw_output_does_not_take_positional_args(self):
+		# --raw_output might eventually support an argument, but we don't want it
+		# to consume any positional arguments, only ones after an '='.
 		self.linux_source_mock.run_kernel = mock.Mock(return_value=[])
-		kunit.main(['run', '--raw_output=kunit'], self.linux_source_mock)
-		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')))
+		kunit.main(['run', '--raw_output', 'filter_glob'], self.linux_source_mock)
+		self.linux_source_mock.run_kernel.assert_called_once_with(
+			args=None, build_dir='.kunit', filter_glob='filter_glob', timeout=300)
 
 	def test_exec_timeout(self):
 		timeout = 3453