| Message ID | 20220217205227.4098452-3-dlatypov@google.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Brendan Higgins |
| Headers | show |
| Series | [1/3] kunit: tool: readability tweaks in KernelCI json generation logic | expand |
On Fri, Feb 18, 2022 at 4:52 AM Daniel Latypov <dlatypov@google.com> wrote: > > Before, kunit.py always printed "arch": "UM" in its json output, but... > 1. With `kunit.py parse`, we could be parsing output from anywhere, so > we can't say that. > 2. Capitalizing it is probably wrong, as it's `ARCH=um` > 3. Commit 87c9c1631788 ("kunit: tool: add support for QEMU") made it so > kunit.py could knowingly run a different arch, yet we'd still always > claim "UM". > Agreed on all counts! > This patch addresses all of those. E.g. > > 1. > $ ./tools/testing/kunit/kunit.py parse .kunit/test.log --json | grep -o '"arch.*' | sort -u > "arch": "", > > 2. > $ ./tools/testing/kunit/kunit.py run --json | ... > "arch": "um", > > 3. > $ ./tools/testing/kunit/kunit.py run --json --arch=x86_64 | ... > "arch": "x86_64", > > Signed-off-by: Daniel Latypov <dlatypov@google.com> > --- Looks good, and works well here. One question/comment below, but in general: Reviewed-by: David Gow <davidgow@google.com> Cheers, -- David > tools/testing/kunit/kunit.py | 4 ++-- > tools/testing/kunit/kunit_kernel.py | 2 ++ > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py > index 7dd6ed42141f..5ccdafd4d5aa 100755 > --- a/tools/testing/kunit/kunit.py > +++ b/tools/testing/kunit/kunit.py > @@ -153,7 +153,7 @@ def exec_tests(linux: kunit_kernel.LinuxSourceTree, request: KunitExecRequest) - > test_glob = request.filter_glob.split('.', maxsplit=2)[1] > filter_globs = [g + '.'+ test_glob for g in filter_globs] > > - metadata = kunit_json.Metadata(build_dir=request.build_dir) > + metadata = kunit_json.Metadata(arch=linux.arch(), build_dir=request.build_dir) > > test_counts = kunit_parser.TestCounts() > exec_time = 0.0 > @@ -506,7 +506,7 @@ def main(argv, linux=None): > 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() > + metadata = kunit_json.Metadata(arch='', build_dir='', def_config='') Why do we explicitly pass empty strings in here, rather than making the defaults correct for this case? > request = KunitParseRequest(raw_output=cli_args.raw_output, > json=cli_args.json) > result, _ = parse_tests(request, metadata, kunit_output) > diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py > index fe159e7ff697..bbbe2ffe30b7 100644 > --- a/tools/testing/kunit/kunit_kernel.py > +++ b/tools/testing/kunit/kunit_kernel.py > @@ -248,6 +248,8 @@ class LinuxSourceTree(object): > kconfig = kunit_config.parse_from_string('\n'.join(kconfig_add)) > self._kconfig.merge_in_entries(kconfig) > > + def arch(self) -> str: > + return self._arch > > def clean(self) -> bool: > try: > -- > 2.35.1.473.g83b2b277ed-goog >
On Wed, Feb 23, 2022 at 10:26 PM David Gow <davidgow@google.com> wrote: > > On Fri, Feb 18, 2022 at 4:52 AM Daniel Latypov <dlatypov@google.com> wrote: > > > > Before, kunit.py always printed "arch": "UM" in its json output, but... > > 1. With `kunit.py parse`, we could be parsing output from anywhere, so > > we can't say that. > > 2. Capitalizing it is probably wrong, as it's `ARCH=um` > > 3. Commit 87c9c1631788 ("kunit: tool: add support for QEMU") made it so > > kunit.py could knowingly run a different arch, yet we'd still always > > claim "UM". > > > Agreed on all counts! > > > This patch addresses all of those. E.g. > > > > 1. > > $ ./tools/testing/kunit/kunit.py parse .kunit/test.log --json | grep -o '"arch.*' | sort -u > > "arch": "", > > > > 2. > > $ ./tools/testing/kunit/kunit.py run --json | ... > > "arch": "um", > > > > 3. > > $ ./tools/testing/kunit/kunit.py run --json --arch=x86_64 | ... > > "arch": "x86_64", > > > > Signed-off-by: Daniel Latypov <dlatypov@google.com> > > --- > > Looks good, and works well here. One question/comment below, but in general: > > Reviewed-by: David Gow <davidgow@google.com> > > Cheers, > -- David > > > tools/testing/kunit/kunit.py | 4 ++-- > > tools/testing/kunit/kunit_kernel.py | 2 ++ > > 2 files changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py > > index 7dd6ed42141f..5ccdafd4d5aa 100755 > > --- a/tools/testing/kunit/kunit.py > > +++ b/tools/testing/kunit/kunit.py > > @@ -153,7 +153,7 @@ def exec_tests(linux: kunit_kernel.LinuxSourceTree, request: KunitExecRequest) - > > test_glob = request.filter_glob.split('.', maxsplit=2)[1] > > filter_globs = [g + '.'+ test_glob for g in filter_globs] > > > > - metadata = kunit_json.Metadata(build_dir=request.build_dir) > > + metadata = kunit_json.Metadata(arch=linux.arch(), build_dir=request.build_dir) > > > > test_counts = kunit_parser.TestCounts() > > exec_time = 0.0 > > @@ -506,7 +506,7 @@ def main(argv, linux=None): > > 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() > > + metadata = kunit_json.Metadata(arch='', build_dir='', def_config='') > > Why do we explicitly pass empty strings in here, rather than making > the defaults correct for this case? Good point, we should just make '' the defaults now. I'll move the hard-coding of "kunit_defconfig" out of the defaults and into exec_tests() then. > > > > request = KunitParseRequest(raw_output=cli_args.raw_output, > > json=cli_args.json) > > result, _ = parse_tests(request, metadata, kunit_output) > > diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py > > index fe159e7ff697..bbbe2ffe30b7 100644 > > --- a/tools/testing/kunit/kunit_kernel.py > > +++ b/tools/testing/kunit/kunit_kernel.py > > @@ -248,6 +248,8 @@ class LinuxSourceTree(object): > > kconfig = kunit_config.parse_from_string('\n'.join(kconfig_add)) > > self._kconfig.merge_in_entries(kconfig) > > > > + def arch(self) -> str: > > + return self._arch > > > > def clean(self) -> bool: > > try: > > -- > > 2.35.1.473.g83b2b277ed-goog > >
diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py index 7dd6ed42141f..5ccdafd4d5aa 100755 --- a/tools/testing/kunit/kunit.py +++ b/tools/testing/kunit/kunit.py @@ -153,7 +153,7 @@ def exec_tests(linux: kunit_kernel.LinuxSourceTree, request: KunitExecRequest) - test_glob = request.filter_glob.split('.', maxsplit=2)[1] filter_globs = [g + '.'+ test_glob for g in filter_globs] - metadata = kunit_json.Metadata(build_dir=request.build_dir) + metadata = kunit_json.Metadata(arch=linux.arch(), build_dir=request.build_dir) test_counts = kunit_parser.TestCounts() exec_time = 0.0 @@ -506,7 +506,7 @@ def main(argv, linux=None): 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() + metadata = kunit_json.Metadata(arch='', build_dir='', def_config='') request = KunitParseRequest(raw_output=cli_args.raw_output, json=cli_args.json) result, _ = parse_tests(request, metadata, kunit_output) diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py index fe159e7ff697..bbbe2ffe30b7 100644 --- a/tools/testing/kunit/kunit_kernel.py +++ b/tools/testing/kunit/kunit_kernel.py @@ -248,6 +248,8 @@ class LinuxSourceTree(object): kconfig = kunit_config.parse_from_string('\n'.join(kconfig_add)) self._kconfig.merge_in_entries(kconfig) + def arch(self) -> str: + return self._arch def clean(self) -> bool: try:
Before, kunit.py always printed "arch": "UM" in its json output, but... 1. With `kunit.py parse`, we could be parsing output from anywhere, so we can't say that. 2. Capitalizing it is probably wrong, as it's `ARCH=um` 3. Commit 87c9c1631788 ("kunit: tool: add support for QEMU") made it so kunit.py could knowingly run a different arch, yet we'd still always claim "UM". This patch addresses all of those. E.g. 1. $ ./tools/testing/kunit/kunit.py parse .kunit/test.log --json | grep -o '"arch.*' | sort -u "arch": "", 2. $ ./tools/testing/kunit/kunit.py run --json | ... "arch": "um", 3. $ ./tools/testing/kunit/kunit.py run --json --arch=x86_64 | ... "arch": "x86_64", Signed-off-by: Daniel Latypov <dlatypov@google.com> --- tools/testing/kunit/kunit.py | 4 ++-- tools/testing/kunit/kunit_kernel.py | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-)