* [PATCH] tools/testing/kunit/kunit.py: remove redundant double check
@ 2023-01-15 21:05 Alexander Pantyukhin
2023-01-17 9:19 ` David Gow
2023-01-17 21:55 ` Daniel Latypov
0 siblings, 2 replies; 6+ messages in thread
From: Alexander Pantyukhin @ 2023-01-15 21:05 UTC (permalink / raw)
To: brendan.higgins
Cc: davidgow, linux-kselftest, kunit-dev, linux-kernel, akpm,
Alexander Pantyukhin
The build_tests function contained the doulbe checking for not success
result. It is fixed in the current patch. Additional small
simplifications of code like useing ternary if were applied (avoid use
the same operation by calculation times differ in two places).
Signed-off-by: Alexander Pantyukhin <apantykhin@gmail.com>
---
tools/testing/kunit/kunit.py | 17 +++++------------
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py
index 43fbe96318fe..481c213818bd 100755
--- a/tools/testing/kunit/kunit.py
+++ b/tools/testing/kunit/kunit.py
@@ -77,10 +77,8 @@ def config_tests(linux: kunit_kernel.LinuxSourceTree,
config_start = time.time()
success = linux.build_reconfig(request.build_dir, request.make_options)
config_end = time.time()
- if not success:
- return KunitResult(KunitStatus.CONFIG_FAILURE,
- config_end - config_start)
- return KunitResult(KunitStatus.SUCCESS,
+ status = KunitStatus.SUCCESS if success else KunitStatus.CONFIG_FAILURE
+ return KunitResult(status,
config_end - config_start)
def build_tests(linux: kunit_kernel.LinuxSourceTree,
@@ -92,13 +90,8 @@ def build_tests(linux: kunit_kernel.LinuxSourceTree,
request.build_dir,
request.make_options)
build_end = time.time()
- if not success:
- return KunitResult(KunitStatus.BUILD_FAILURE,
- build_end - build_start)
- if not success:
- return KunitResult(KunitStatus.BUILD_FAILURE,
- build_end - build_start)
- return KunitResult(KunitStatus.SUCCESS,
+ status = KunitStatus.SUCCESS if success else KunitStatus.BUILD_FAILURE
+ return KunitResult(status,
build_end - build_start)
def config_and_build_tests(linux: kunit_kernel.LinuxSourceTree,
@@ -145,7 +138,7 @@ def exec_tests(linux: kunit_kernel.LinuxSourceTree, request: KunitExecRequest) -
tests = _list_tests(linux, request)
if request.run_isolated == 'test':
filter_globs = tests
- if request.run_isolated == 'suite':
+ elif request.run_isolated == 'suite':
filter_globs = _suites_from_test_list(tests)
# Apply the test-part of the user's glob, if present.
if '.' in request.filter_glob:
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] tools/testing/kunit/kunit.py: remove redundant double check
2023-01-15 21:05 [PATCH] tools/testing/kunit/kunit.py: remove redundant double check Alexander Pantyukhin
@ 2023-01-17 9:19 ` David Gow
2023-01-17 9:35 ` Alexander Pantyukhin
2023-01-17 21:55 ` Daniel Latypov
1 sibling, 1 reply; 6+ messages in thread
From: David Gow @ 2023-01-17 9:19 UTC (permalink / raw)
To: Alexander Pantyukhin
Cc: brendan.higgins, linux-kselftest, kunit-dev, linux-kernel, akpm
[-- Attachment #1: Type: text/plain, Size: 3171 bytes --]
On Mon, 16 Jan 2023 at 05:05, Alexander Pantyukhin <apantykhin@gmail.com> wrote:
>
> The build_tests function contained the doulbe checking for not success
Nit: "double" -> "double"
> result. It is fixed in the current patch. Additional small
> simplifications of code like useing ternary if were applied (avoid use
Nit: "useing" -> "using"
> the same operation by calculation times differ in two places).
>
> Signed-off-by: Alexander Pantyukhin <apantykhin@gmail.com>
> ---
Thanks for catching these!
This looks good to me, save for a few typos (which you should fix if
there's a v2, but it isn't important enough to do another version...)
Unless someone with more Python knowledge than me jumps in and
complains, this is:
Reviewed-by: David Gow <davidgow@google.com>
Cheers,
-- David
> tools/testing/kunit/kunit.py | 17 +++++------------
> 1 file changed, 5 insertions(+), 12 deletions(-)
>
> diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py
> index 43fbe96318fe..481c213818bd 100755
> --- a/tools/testing/kunit/kunit.py
> +++ b/tools/testing/kunit/kunit.py
> @@ -77,10 +77,8 @@ def config_tests(linux: kunit_kernel.LinuxSourceTree,
> config_start = time.time()
> success = linux.build_reconfig(request.build_dir, request.make_options)
> config_end = time.time()
> - if not success:
> - return KunitResult(KunitStatus.CONFIG_FAILURE,
> - config_end - config_start)
> - return KunitResult(KunitStatus.SUCCESS,
> + status = KunitStatus.SUCCESS if success else KunitStatus.CONFIG_FAILURE
> + return KunitResult(status,
> config_end - config_start)
>
> def build_tests(linux: kunit_kernel.LinuxSourceTree,
> @@ -92,13 +90,8 @@ def build_tests(linux: kunit_kernel.LinuxSourceTree,
> request.build_dir,
> request.make_options)
> build_end = time.time()
> - if not success:
> - return KunitResult(KunitStatus.BUILD_FAILURE,
> - build_end - build_start)
> - if not success:
> - return KunitResult(KunitStatus.BUILD_FAILURE,
> - build_end - build_start)
> - return KunitResult(KunitStatus.SUCCESS,
> + status = KunitStatus.SUCCESS if success else KunitStatus.BUILD_FAILURE
> + return KunitResult(status,
> build_end - build_start)
>
> def config_and_build_tests(linux: kunit_kernel.LinuxSourceTree,
> @@ -145,7 +138,7 @@ def exec_tests(linux: kunit_kernel.LinuxSourceTree, request: KunitExecRequest) -
> tests = _list_tests(linux, request)
> if request.run_isolated == 'test':
> filter_globs = tests
> - if request.run_isolated == 'suite':
> + elif request.run_isolated == 'suite':
> filter_globs = _suites_from_test_list(tests)
> # Apply the test-part of the user's glob, if present.
> if '.' in request.filter_glob:
> --
> 2.25.1
>
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tools/testing/kunit/kunit.py: remove redundant double check
2023-01-17 9:19 ` David Gow
@ 2023-01-17 9:35 ` Alexander Pantyukhin
0 siblings, 0 replies; 6+ messages in thread
From: Alexander Pantyukhin @ 2023-01-17 9:35 UTC (permalink / raw)
To: David Gow; +Cc: brendan.higgins, linux-kselftest, kunit-dev, linux-kernel, akpm
Hello, David.
Thank you very much for your review!
There is no v2 yet.
>
> On Mon, 16 Jan 2023 at 05:05, Alexander Pantyukhin <apantykhin@gmail.com> wrote:
> >
> > The build_tests function contained the doulbe checking for not success
> Nit: "double" -> "double"
>
> > result. It is fixed in the current patch. Additional small
> > simplifications of code like useing ternary if were applied (avoid use
> Nit: "useing" -> "using"
> > the same operation by calculation times differ in two places).
> >
> > Signed-off-by: Alexander Pantyukhin <apantykhin@gmail.com>
> > ---
> Thanks for catching these!
> This looks good to me, save for a few typos (which you should fix if
> there's a v2, but it isn't important enough to do another version...)
>
> Unless someone with more Python knowledge than me jumps in and
> complains, this is:
>
> Reviewed-by: David Gow <davidgow@google.com>
>
> Cheers,
> -- David
>
> > tools/testing/kunit/kunit.py | 17 +++++------------
> > 1 file changed, 5 insertions(+), 12 deletions(-)
> >
> > diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py
> > index 43fbe96318fe..481c213818bd 100755
> > --- a/tools/testing/kunit/kunit.py
> > +++ b/tools/testing/kunit/kunit.py
> > @@ -77,10 +77,8 @@ def config_tests(linux: kunit_kernel.LinuxSourceTree,
> > config_start = time.time()
> > success = linux.build_reconfig(request.build_dir, request.make_options)
> > config_end = time.time()
> > - if not success:
> > - return KunitResult(KunitStatus.CONFIG_FAILURE,
> > - config_end - config_start)
> > - return KunitResult(KunitStatus.SUCCESS,
> > + status = KunitStatus.SUCCESS if success else KunitStatus.CONFIG_FAILURE
> > + return KunitResult(status,
> > config_end - config_start)
> >
> > def build_tests(linux: kunit_kernel.LinuxSourceTree,
> > @@ -92,13 +90,8 @@ def build_tests(linux: kunit_kernel.LinuxSourceTree,
> > request.build_dir,
> > request.make_options)
> > build_end = time.time()
> > - if not success:
> > - return KunitResult(KunitStatus.BUILD_FAILURE,
> > - build_end - build_start)
> > - if not success:
> > - return KunitResult(KunitStatus.BUILD_FAILURE,
> > - build_end - build_start)
> > - return KunitResult(KunitStatus.SUCCESS,
> > + status = KunitStatus.SUCCESS if success else KunitStatus.BUILD_FAILURE
> > + return KunitResult(status,
> > build_end - build_start)
> >
> > def config_and_build_tests(linux: kunit_kernel.LinuxSourceTree,
> > @@ -145,7 +138,7 @@ def exec_tests(linux: kunit_kernel.LinuxSourceTree, request: KunitExecRequest) -
> > tests = _list_tests(linux, request)
> > if request.run_isolated == 'test':
> > filter_globs = tests
> > - if request.run_isolated == 'suite':
> > + elif request.run_isolated == 'suite':
> > filter_globs = _suites_from_test_list(tests)
> > # Apply the test-part of the user's glob, if present.
> > if '.' in request.filter_glob:
> > --
> > 2.25.1
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tools/testing/kunit/kunit.py: remove redundant double check
2023-01-15 21:05 [PATCH] tools/testing/kunit/kunit.py: remove redundant double check Alexander Pantyukhin
2023-01-17 9:19 ` David Gow
@ 2023-01-17 21:55 ` Daniel Latypov
2023-01-18 5:33 ` Alexander Pantyukhin
1 sibling, 1 reply; 6+ messages in thread
From: Daniel Latypov @ 2023-01-17 21:55 UTC (permalink / raw)
To: Alexander Pantyukhin
Cc: brendan.higgins, davidgow, linux-kselftest, kunit-dev,
linux-kernel, akpm
On Sun, Jan 15, 2023 at 1:05 PM Alexander Pantyukhin
<apantykhin@gmail.com> wrote:
>
> The build_tests function contained the doulbe checking for not success
very nit: if we're fixing the "doulbe" typo, can we also do
"the doulbe" => "double" (drop the "the")
> result. It is fixed in the current patch. Additional small
> simplifications of code like useing ternary if were applied (avoid use
> the same operation by calculation times differ in two places).
>
> Signed-off-by: Alexander Pantyukhin <apantykhin@gmail.com>
Reviewed-by: Daniel Latypov <dlatypov@google.com>
Thanks!
I can't believe we never noticed the duplicate `if not success` blocks
before now.
Some minor suggestions below if we're already going to send a v2 out for typos.
> ---
> tools/testing/kunit/kunit.py | 17 +++++------------
> 1 file changed, 5 insertions(+), 12 deletions(-)
>
> diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py
> index 43fbe96318fe..481c213818bd 100755
> --- a/tools/testing/kunit/kunit.py
> +++ b/tools/testing/kunit/kunit.py
> @@ -77,10 +77,8 @@ def config_tests(linux: kunit_kernel.LinuxSourceTree,
> config_start = time.time()
> success = linux.build_reconfig(request.build_dir, request.make_options)
> config_end = time.time()
> - if not success:
> - return KunitResult(KunitStatus.CONFIG_FAILURE,
> - config_end - config_start)
> - return KunitResult(KunitStatus.SUCCESS,
> + status = KunitStatus.SUCCESS if success else KunitStatus.CONFIG_FAILURE
> + return KunitResult(status,
> config_end - config_start)
nit: perhaps we can shorten this to one line, i.e.
return KunitResult(status, config_end - config_start)
>
> def build_tests(linux: kunit_kernel.LinuxSourceTree,
> @@ -92,13 +90,8 @@ def build_tests(linux: kunit_kernel.LinuxSourceTree,
> request.build_dir,
> request.make_options)
> build_end = time.time()
> - if not success:
> - return KunitResult(KunitStatus.BUILD_FAILURE,
> - build_end - build_start)
> - if not success:
> - return KunitResult(KunitStatus.BUILD_FAILURE,
> - build_end - build_start)
Oh huh, I guess this duplication comes from commit 45ba7a893ad8
("kunit: kunit_tool: Separate out config/build/exec/parse")
@@ -64,78 +84,167 @@ def run_tests(linux: kunit_kernel.LinuxSourceTree,
build_end = time.time()
if not success:
return KunitResult(KunitStatus.BUILD_FAILURE, 'could
not build kernel')
+ if not success:
+ return KunitResult(KunitStatus.BUILD_FAILURE,
+ 'could not build kernel',
> - return KunitResult(KunitStatus.SUCCESS,
> + status = KunitStatus.SUCCESS if success else KunitStatus.BUILD_FAILURE
> + return KunitResult(status,
> build_end - build_start)
ditto here,
return KunitResult(status, build_end - build_start)
>
> def config_and_build_tests(linux: kunit_kernel.LinuxSourceTree,
> @@ -145,7 +138,7 @@ def exec_tests(linux: kunit_kernel.LinuxSourceTree, request: KunitExecRequest) -
> tests = _list_tests(linux, request)
> if request.run_isolated == 'test':
> filter_globs = tests
> - if request.run_isolated == 'suite':
> + elif request.run_isolated == 'suite':
This is better, thanks.
Daniel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tools/testing/kunit/kunit.py: remove redundant double check
2023-01-17 21:55 ` Daniel Latypov
@ 2023-01-18 5:33 ` Alexander Pantyukhin
2023-01-18 7:17 ` Daniel Latypov
0 siblings, 1 reply; 6+ messages in thread
From: Alexander Pantyukhin @ 2023-01-18 5:33 UTC (permalink / raw)
To: Daniel Latypov
Cc: brendan.higgins, davidgow, linux-kselftest, kunit-dev,
linux-kernel, akpm
Hello Daniel.
Thank you very much for your review!
Could you advise me whom I can address the V2 patch "to"?
Best, Alex.
ср, 18 янв. 2023 г. в 01:56, Daniel Latypov <dlatypov@google.com>:
>
> On Sun, Jan 15, 2023 at 1:05 PM Alexander Pantyukhin
> <apantykhin@gmail.com> wrote:
> >
> > The build_tests function contained the doulbe checking for not success
>
> very nit: if we're fixing the "doulbe" typo, can we also do
> "the doulbe" => "double" (drop the "the")
>
> > result. It is fixed in the current patch. Additional small
> > simplifications of code like useing ternary if were applied (avoid use
> > the same operation by calculation times differ in two places).
> >
> > Signed-off-by: Alexander Pantyukhin <apantykhin@gmail.com>
>
> Reviewed-by: Daniel Latypov <dlatypov@google.com>
>
> Thanks!
> I can't believe we never noticed the duplicate `if not success` blocks
> before now.
>
> Some minor suggestions below if we're already going to send a v2 out for typos.
>
> > ---
> > tools/testing/kunit/kunit.py | 17 +++++------------
> > 1 file changed, 5 insertions(+), 12 deletions(-)
> >
> > diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py
> > index 43fbe96318fe..481c213818bd 100755
> > --- a/tools/testing/kunit/kunit.py
> > +++ b/tools/testing/kunit/kunit.py
> > @@ -77,10 +77,8 @@ def config_tests(linux: kunit_kernel.LinuxSourceTree,
> > config_start = time.time()
> > success = linux.build_reconfig(request.build_dir, request.make_options)
> > config_end = time.time()
> > - if not success:
> > - return KunitResult(KunitStatus.CONFIG_FAILURE,
> > - config_end - config_start)
> > - return KunitResult(KunitStatus.SUCCESS,
> > + status = KunitStatus.SUCCESS if success else KunitStatus.CONFIG_FAILURE
> > + return KunitResult(status,
> > config_end - config_start)
>
> nit: perhaps we can shorten this to one line, i.e.
> return KunitResult(status, config_end - config_start)
>
> >
> > def build_tests(linux: kunit_kernel.LinuxSourceTree,
> > @@ -92,13 +90,8 @@ def build_tests(linux: kunit_kernel.LinuxSourceTree,
> > request.build_dir,
> > request.make_options)
> > build_end = time.time()
> > - if not success:
> > - return KunitResult(KunitStatus.BUILD_FAILURE,
> > - build_end - build_start)
> > - if not success:
> > - return KunitResult(KunitStatus.BUILD_FAILURE,
> > - build_end - build_start)
>
> Oh huh, I guess this duplication comes from commit 45ba7a893ad8
> ("kunit: kunit_tool: Separate out config/build/exec/parse")
>
> @@ -64,78 +84,167 @@ def run_tests(linux: kunit_kernel.LinuxSourceTree,
> build_end = time.time()
> if not success:
> return KunitResult(KunitStatus.BUILD_FAILURE, 'could
> not build kernel')
> + if not success:
> + return KunitResult(KunitStatus.BUILD_FAILURE,
> + 'could not build kernel',
>
> > - return KunitResult(KunitStatus.SUCCESS,
> > + status = KunitStatus.SUCCESS if success else KunitStatus.BUILD_FAILURE
> > + return KunitResult(status,
> > build_end - build_start)
>
> ditto here,
> return KunitResult(status, build_end - build_start)
>
> >
> > def config_and_build_tests(linux: kunit_kernel.LinuxSourceTree,
> > @@ -145,7 +138,7 @@ def exec_tests(linux: kunit_kernel.LinuxSourceTree, request: KunitExecRequest) -
> > tests = _list_tests(linux, request)
> > if request.run_isolated == 'test':
> > filter_globs = tests
> > - if request.run_isolated == 'suite':
> > + elif request.run_isolated == 'suite':
>
> This is better, thanks.
>
> Daniel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tools/testing/kunit/kunit.py: remove redundant double check
2023-01-18 5:33 ` Alexander Pantyukhin
@ 2023-01-18 7:17 ` Daniel Latypov
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Latypov @ 2023-01-18 7:17 UTC (permalink / raw)
To: Alexander Pantyukhin
Cc: brendan.higgins, davidgow, linux-kselftest, kunit-dev,
linux-kernel, akpm
On Tue, Jan 17, 2023 at 9:33 PM Alexander Pantyukhin
<apantykhin@gmail.com> wrote:
>
> Hello Daniel.
> Thank you very much for your review!
> Could you advise me whom I can address the V2 patch "to"?
davidgow@google.com is the more active maintainer right now.
But since you've got his Reviewed-by already, as long as you CC
kunit-dev@ and linux-kselftest@, whatever you want to put is fine. We
can make sure v2 gets picked up.
You'll ultimately see the patch go in through
https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/?h=kunit
and hopefully merged into 6.3.
Daniel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-01-18 7:50 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-01-15 21:05 [PATCH] tools/testing/kunit/kunit.py: remove redundant double check Alexander Pantyukhin
2023-01-17 9:19 ` David Gow
2023-01-17 9:35 ` Alexander Pantyukhin
2023-01-17 21:55 ` Daniel Latypov
2023-01-18 5:33 ` Alexander Pantyukhin
2023-01-18 7:17 ` Daniel Latypov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox