On Fri, Mar 15, 2024 at 7:16 PM Daniel Latypov dlatypov@google.com wrote:
On Thu, Mar 7, 2024 at 2:29 PM Rae Moar rmoar@google.com wrote:
Add ability to parse multiple files. Additionally add the ability to parse all results in the KUnit debugfs repository.
How to parse multiple files:
./tools/testing/kunit/kunit.py parse results.log results2.log
How to parse all files in directory:
./tools/testing/kunit/kunit.py parse directory_path/*
How to parse KUnit debugfs repository:
./tools/testing/kunit/kunit.py parse debugfs
For each file, the parser outputs the file name, results, and test summary. At the end of all parsing, the parser outputs a total summary line.
This feature can be easily tested on the tools/testing/kunit/test_data/ directory.
Signed-off-by: Rae Moar rmoar@google.com
Changes since v2:
- Fixed bug with input from command line. I changed this to use input(). Daniel, let me know if this works for you.
Oops, sorry for the delay.
Hi!
No worries at all. Thanks for the review!
Hmm, it seems to be treating the stdin lines like file names
$ ./tools/testing/kunit/kunit.py parse < ./tools/testing/kunit/test_data/test_config_printk_time.log File path: Could not find [ 0.060000] printk: console [mc-1] enabled
Oh, I see, we're prompting the user via input("File path: ") ?
I'm not necessarily against such a change, but I would personally prefer the old behavior of being able to read ktap from stdin directly. As a user, I'd also prefer to only type out filenames as arguments where I can get autocomplete, so `input()` here wouldn't help me personally.
Applying a hackish patch like this [1] on top gets the behavior I'd personally expect: $ ./tools/testing/kunit/kunit.py parse < ./tools/testing/kunit/test_data/test_config_printk_time.log /dev/stdin ... [16:01:50] Testing complete. Ran 10 tests: passed: 10
I'd mentioned in the previous version that we could have parsed files contain a `Union[str, TextIO]` and then read from the `sys.stdin` file object directly. But having it blindly open `/dev/stdin` seems to work just the same, if we want to keep our list simpler and just hold strings.
I definitely see why the change to stdin would be better. My original change to input() was to keep it simple. But I really like the change listed below. I will go ahead and implement that.
[1] this just also re-orders the `os.path.isdir()` check as mentioned below, which simplifies things diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py index 1aa3d736d80c..311d107bd684 100755 --- a/tools/testing/kunit/kunit.py +++ b/tools/testing/kunit/kunit.py @@ -515,18 +515,18 @@ def parse_handler(cli_args: argparse.Namespace) -> None: total_test = kunit_parser.Test() total_test.status = kunit_parser.TestStatus.SUCCESS if not parsed_files:
parsed_files.append(input("File path: "))if parsed_files[0] == "debugfs" and len(parsed_files) == 1:
parsed_files.append('/dev/stdin')elif len(parsed_files) == 1 and parsed_files[0] == "debugfs": parsed_files.pop() for (root, _, files) in os.walk("/sys/kernel/debug/kunit"): parsed_files.extend(os.path.join(root, f) forf in files if f == "results")
if not parsed_files:print("No files found.")
if not parsed_files:print("No files found.") for file in parsed_files:
if os.path.isfile(file):
if os.path.isdir(file):print("Ignoring directory ", file)elif os.path.exists(file): print(file) with open(file, 'r', errors='backslashreplace') as f: kunit_output = f.read().splitlines()@@ -536,8 +536,6 @@ def parse_handler(cli_args: argparse.Namespace) -> None: json=cli_args.json) _, test = parse_tests(request, metadata, kunit_output) total_test.subtests.append(test)
elif os.path.isdir(file):print("Ignoring directory ", file) else: print("Could not find ", file)
- Add more specific warning messages
tools/testing/kunit/kunit.py | 56 +++++++++++++++++++++++++----------- 1 file changed, 40 insertions(+), 16 deletions(-)
diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py index bc74088c458a..1aa3d736d80c 100755 --- a/tools/testing/kunit/kunit.py +++ b/tools/testing/kunit/kunit.py @@ -511,19 +511,42 @@ def exec_handler(cli_args: argparse.Namespace) -> None:
def parse_handler(cli_args: argparse.Namespace) -> None:
if cli_args.file is None:sys.stdin.reconfigure(errors='backslashreplace') # type: ignorekunit_output = sys.stdin # type: Iterable[str]else:with open(cli_args.file, 'r', errors='backslashreplace') as f:kunit_output = f.read().splitlines()# We know nothing about how the result was created!metadata = kunit_json.Metadata()request = KunitParseRequest(raw_output=cli_args.raw_output,json=cli_args.json)result, _ = parse_tests(request, metadata, kunit_output)if result.status != KunitStatus.SUCCESS:sys.exit(1)
parsed_files = cli_args.files # type: List[str]total_test = kunit_parser.Test()total_test.status = kunit_parser.TestStatus.SUCCESSif not parsed_files:parsed_files.append(input("File path: "))if parsed_files[0] == "debugfs" and len(parsed_files) == 1:parsed_files.pop()for (root, _, files) in os.walk("/sys/kernel/debug/kunit"):parsed_files.extend(os.path.join(root, f) for f in files if f == "results")if not parsed_files:print("No files found.")for file in parsed_files:if os.path.isfile(file):Note: perhaps we should reorder this to
if os.path.isdir(file): ... elif os.path.exists(file): ...
That way this code will then start handling non-regular, yet readable files, like links, etc. That would also help out if we started passing in the magic "/dev/stdin" (since it's a symlink)
Oh I see. Yes I will try to implement this! Thanks!
print(file)with open(file, 'r', errors='backslashreplace') as f:kunit_output = f.read().splitlines()# We know nothing about how the result was created!metadata = kunit_json.Metadata()request = KunitParseRequest(raw_output=cli_args.raw_output,json=cli_args.json)_, test = parse_tests(request, metadata, kunit_output)total_test.subtests.append(test)elif os.path.isdir(file):print("Ignoring directory ", file)minor nit: `print()` will automatically put a space between arguments, e.g.
Ignoring directory .
is what it'll print if I run `kunit.py parse .`
It might be better to use a f-string so put quotes around it, like so print(f'Ignoring directory "{file}"')} and below, print(f'Could not find "{file}"')
Yep! Happy to change this.
else:print("Could not find ", file)if len(parsed_files) > 1: # if more than one file was parsed output total summaryprint('All files parsed.')if not request.raw_output:stdout.print_with_timestamp(kunit_parser.DIVIDER)kunit_parser.bubble_up_test_results(total_test)kunit_parser.print_summary_line(total_test)subcommand_handlers_map = { @@ -569,9 +592,10 @@ def main(argv: Sequence[str]) -> None: help='Parses KUnit results from a file, ' 'and parses formatted results.') add_parse_opts(parse_parser)
parse_parser.add_argument('file',help='Specifies the file to read results from.',type=str, nargs='?', metavar='input_file')
parse_parser.add_argument('files',help='List of file paths to read results from or keyword''"debugfs" to read all results from the debugfs directory.',minor spacing note: there are two ' 's here in the series of tabs, i.e. ^I^I^I^I ^I^I'"debugfs" to read all results from the debugfs directory.',$ (using vim's :list formatting)
This was copy-pasted from the lines above and below which look like ^I^I^I^I help='List of file paths to read results from or keyword'$ i.e. they use the 2 spaces to align after the tabs.
We can just drop those 2 spaces since they won't visually affect the outcome with a tabwidth of 8 spaces.
Thanks for pointing this out! I will change the spacing here. I am thinking of just changing it to match the other lines. So something like this: ^I^I^I^I '"debugfs" to read all results from the debugfs directory.',$
Sorry again for the delayed reply, Daniel