Message-ID: <CAGS_qxpXLqrJWnb5N+2R5MRk2ZPxau+6NNB4BjVpViXoeQThgg@mail.gmail.com>
Date:   Tue, 2 Nov 2021 21:22:45 -0700
From:   Daniel Latypov <dlatypov@...gle.com>
To:     David Gow <davidgow@...gle.com>
Cc:     Brendan Higgins <brendanhiggins@...gle.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        KUnit Development <kunit-dev@...glegroups.com>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>,
        Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH] kunit: add run_checks.py script to validate kunit changes

On Tue, Nov 2, 2021 at 7:38 PM David Gow <davidgow@...gle.com> wrote:
>
> On Wed, Nov 3, 2021 at 7:57 AM 'Daniel Latypov' via KUnit Development
> <kunit-dev@...glegroups.com> wrote:
> >
> > This formalizes the checks KUnit maintainers have been running (or in
> > other cases: forgetting to run).
>
> Guilty as charged. :-)

I was largely referring to myself :)

> >
> > This script also runs them all in parallel to minimize friction (pytype
> > can be fairly slow, but not slower than running kunit.py).
> >
> > Example output:
> > $ ./tools/testing/kunit/run_checks.py
> > Waiting on 4 checks (kunit_tool_test.py, kunit smoke test, pytype, mypy)...
> > kunit_tool_test.py: PASSED
> > mypy: PASSED
> > pytype: PASSED
> > kunit smoke test: PASSED
> >
> > On failure or timeout (5 minutes), it'll dump out the stdout/stderr.
> > E.g. adding in a type-checking error:
> >   mypy: FAILED
> >   > kunit.py:54: error: Name 'nonexistent_function' is not defined
> >   > Found 1 error in 1 file (checked 8 source files)
> >
> > mypy and pytype are two Python type-checkers and must be installed.
> > This file treats them as optional and will mark them as SKIPPED if not
> > installed.
> >
> > This tool also runs `kunit.py run --kunitconfig=lib/kunit` to run
> > KUnit's own KUnit tests and to verify KUnit kernel code and kunit.py
> > play nicely together.
> >
> > It uses --build_dir=kunit_run_checks so as not to clobber the default
> > build_dir, which helps make it faster by reducing the need to rebuild,
> > esp. if you're been passing in --arch instead of using UML.
> >
> > Signed-off-by: Daniel Latypov <dlatypov@...gle.com>
> > ---
>
> Thanks -- this is working well here. A couple of minor suggestions
> below, but even without them, this is very useful.
>
> Reviewed-by: David Gow <davidgow@...gle.com>
>
> >  tools/testing/kunit/run_checks.py | 76 +++++++++++++++++++++++++++++++
> >  1 file changed, 76 insertions(+)
> >  create mode 100755 tools/testing/kunit/run_checks.py
> >
> > diff --git a/tools/testing/kunit/run_checks.py b/tools/testing/kunit/run_checks.py
> > new file mode 100755
> > index 000000000000..d03ca3f84b91
> > --- /dev/null
> > +++ b/tools/testing/kunit/run_checks.py
> > @@ -0,0 +1,76 @@
> > +#!/usr/bin/env python3
> > +# SPDX-License-Identifier: GPL-2.0
> > +#
> > +# This file runs some basic checks to verify kunit works.
> > +# It is only of interest if you're making changes to KUnit itself.
> > +#
> > +# Copyright (C) 2021, Google LLC.
> > +# Author: Daniel Latypov <dlatypov@...gle.com.com>
> > +
> > +from concurrent import futures
> > +import datetime
> > +import os
> > +import shutil
> > +import subprocess
> > +import sys
> > +import textwrap
> > +from typing import Dict, List, Sequence, Tuple
> > +
> > +ABS_TOOL_PATH = os.path.abspath(os.path.dirname(__file__))
> > +_TIMEOUT = datetime.timedelta(minutes=5).total_seconds()
> > +
> > +commands: Dict[str, Sequence[str]] = {
> > +       'kunit_tool_test.py': ['./kunit_tool_test.py'],
> > +       'kunit smoke test': ['./kunit.py', 'run', '--kunitconfig=lib/kunit', '--build_dir=kunit_run_checks'],
> > +       'pytype': ['/bin/sh', '-c', 'pytype *.py'],
> > +       'mypy': ['/bin/sh', '-c', 'mypy *.py'],
> > +}
> > +
> > +# The user might not have mypy or pytype installed, skip them if so.
> > +# Note: you can install both via `$ pip install mypy pytype`
> > +necessary_deps : Dict[str, str] = {
> > +       'pytype': 'pytype',
> > +       'mypy': 'mypy',
> > +}
> > +
> > +def main(argv: Sequence[str]) -> None:
> > +       if len(argv) > 1:
> > +               raise RuntimeError('Too many command-line arguments.')
>
> What does the command-line argument here actually do? It looks like
> nothing, because the argv variable is shadowed below?
>
> Or was this supposed to check that there are no arguments, which
> doesn't work because argv[] is stripped of its first element in the
> 'if __name__=='__main__'?

It was supposed to check for no args.
I forgot I already stripped it off (this bit of code here was
autogenerated for me by a snippet).

>
> > +
> > +       future_to_name: Dict[futures.Future, str] = {}
> > +       executor = futures.ThreadPoolExecutor(max_workers=len(commands))
> > +       for name, argv in commands.items():
> > +               if name in necessary_deps and shutil.which(necessary_deps[name]) is None:
> > +                       print(f'{name}: SKIPPED, {necessary_deps[name]} not in $PATH')
> > +                       continue
> > +               f = executor.submit(run_cmd, argv)
> > +               future_to_name[f] = name
> > +
> > +       print(f'Waiting on {len(future_to_name)} checks ({", ".join(future_to_name.values())})...')
> > +       for f in  futures.as_completed(future_to_name.keys()):
> > +               name = future_to_name[f]
> > +               ex = f.exception()
> > +               if not ex:
> > +                       print(f'{name}: PASSED')
> > +                       continue
> > +
> > +               if isinstance(ex, subprocess.TimeoutExpired):
> > +                       print(f'{name}: TIMED OUT')
> > +               elif isinstance(ex, subprocess.CalledProcessError):
> > +                       print(f'{name}: FAILED')
> > +               else:
> > +                       print('{name}: unexpected exception: {ex}')
> > +                       continue
> > +
> > +               output = ex.output
> > +               if output:
> > +                       print(textwrap.indent(output.decode(), '> '))
> > +       executor.shutdown()
> > +
> > +
> > +def run_cmd(argv: Sequence[str]):
> > +       subprocess.check_output(argv, stderr=subprocess.STDOUT, cwd=ABS_TOOL_PATH, timeout=_TIMEOUT)
> > +
> > +
> > +if __name__ == '__main__':
> > +       main(sys.argv[1:])
>
> Any chance we could get this to return a non-zero exit code if one of
> these checks fails?

Oh, I thought I did this before sending it out.
Will add this in for v2.

>
> >
> > base-commit: 52a5d80a2225e2d0b2a8f4656b76aead2a443b2a
> > --
> > 2.33.1.1089.g2158813163f-goog
> >
> > --
> > You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@...glegroups.com.
> > To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20211102235734.497713-1-dlatypov%40google.com.

Powered by blists - more mailing lists