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

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