| Message ID | 20240307222932.584164-1-rmoar@google.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] kunit: tool: add ability to parse multiple files | expand |
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. 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. [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) for f 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: ignore > - kunit_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.SUCCESS > + if 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) > + 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}"') > + else: > + print("Could not find ", file) > + > + if len(parsed_files) > 1: # if more than one file was parsed output total summary > + print('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. Sorry again for the delayed reply, Daniel
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) for > f 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: ignore > > - kunit_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.SUCCESS > > + if 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 summary > > + print('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
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: ignore - kunit_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.SUCCESS + if 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): + 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) + else: + print("Could not find ", file) + + if len(parsed_files) > 1: # if more than one file was parsed output total summary + print('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.', + type=str, nargs='*', metavar='input_files') cli_args = parser.parse_args(massage_argv(argv))
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. - Add more specific warning messages tools/testing/kunit/kunit.py | 56 +++++++++++++++++++++++++----------- 1 file changed, 40 insertions(+), 16 deletions(-) base-commit: 806cb2270237ce2ec672a407d66cee17a07d3aa2