Re: [PATCH v3 4/5] qemu-iotests: let "check" spawn an arbitrary test command

[Max Reitz]

On 30.03.21 12:38, Max Reitz wrote:
> On 26.03.21 16:05, Max Reitz wrote:
> > On 26.03.21 15:23, Paolo Bonzini wrote:
> > > Right now there is no easy way for "check" to print a reproducer 
> > > command.
> > > Because such a reproducer command line would be huge, we can instead 
> > > teach
> > > check to start a command of our choice.  This can be for example a 
> > > Python
> > > unit test with arguments to only run a specific subtest.
> > >
> > > Move the trailing empty line to print_env(), since it always looks 
> > > better
> > > and one caller was not adding it.
> > >
> > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > > Tested-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> > > Message-Id: <20210323181928.311862-5-pbonzini@redhat.com>
> > > ---
> > >   tests/qemu-iotests/check         | 18 +++++++++++++++++-
> > >   tests/qemu-iotests/testenv.py    |  3 ++-
> > >   tests/qemu-iotests/testrunner.py |  1 -
> > >   3 files changed, 19 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
> > > index d1c87ceaf1..df9fd733ff 100755
> > > --- a/tests/qemu-iotests/check
> > > +++ b/tests/qemu-iotests/check
> > > @@ -19,6 +19,9 @@
> > >   import os
> > >   import sys
> > >   import argparse
> > > +import shutil
> > > +from pathlib import Path
> > > +
> > >   from findtests import TestFinder
> > >   from testenv import TestEnv
> > >   from testrunner import TestRunner
> > > @@ -101,7 +104,7 @@ def make_argparser() -> argparse.ArgumentParser:
> > >                          'rerun failed ./check command, starting from 
> > > the '
> > >                          'middle of the process.')
> > >       g_sel.add_argument('tests', metavar='TEST_FILES', nargs='*',
> > > -                       help='tests to run')
> > > +                       help='tests to run, or "--" followed by a 
> > > command')
> > >       return p
> > > @@ -114,6 +117,19 @@ if __name__ == '__main__':
> > >                     imgopts=args.imgopts, misalign=args.misalign,
> > >                     debug=args.debug, valgrind=args.valgrind)
> > > +    if len(sys.argv) > 1 and sys.argv[-len(args.tests)-1] == '--':
> > > +        if not args.tests:
> > > +            sys.exit("missing command after '--'")
> > > +        cmd = args.tests
> > > +        env.print_env()
> > > +        exec_path = Path(shutil.which(cmd[0]))
> >
> > 297 says:
> >
> > check:125: error: Argument 1 to "Path" has incompatible type 
> > "Optional[str]"; expected "Union[str, _PathLike[str]]"
> > Found 1 error in 1 file (checked 1 source file)
> >
> > Normally I’d assert this away, but actually I think the returned value 
> > should be checked and we should print an error if it’s None.  (Seems 
> > like shutil.which() doesn’t raise an exception if there is no such 
> > command, it just returns None.)
> >
> > Max
> >
> > > +        if exec_path is None:
> > > +            sys.exit('command not found: ' + cmd[0])
>
> Oh, I see, the intent to print an error is actually there.  The problem 
> is just that Path(None) throws an exception, so we must check 
> shutil.which()’s return value.
>
> I’ll squash this in if you don’t mind:
>
> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
> index df9fd733ff..e2230f5612 100755
> --- a/tests/qemu-iotests/check
> +++ b/tests/qemu-iotests/check
> @@ -122,9 +122,10 @@ if __name__ == '__main__':
>               sys.exit("missing command after '--'")
>           cmd = args.tests
>           env.print_env()
> -        exec_path = Path(shutil.which(cmd[0]))
> -        if exec_path is None:
> +        exec_pathstr = shutil.which(cmd[0])
> +        if exec_pathstr is None:
>               sys.exit('command not found: ' + cmd[0])
> +        exec_path = Path(exec_pathstr)
>           cmd[0] = exec_path.resolve()
>           full_env = env.prepare_subprocess(cmd)
>           os.chdir(Path(exec_path).parent)
>
> > > +        cmd[0] = exec_path.resolve()
> > > +        full_env = env.prepare_subprocess(cmd)
> > > +        os.chdir(Path(exec_path).parent)

Oh, and this Path() does nothing, I presume, so I’m going to replace it 
with just “exec_path”.

Max

> > > +        os.execve(cmd[0], cmd, full_env)
> > > +
> > >       testfinder = TestFinder(test_dir=env.source_iotests)
> > >       groups = args.groups.split(',') if args.groups else None
> > > diff --git a/tests/qemu-iotests/testenv.py 
> > > b/tests/qemu-iotests/testenv.py
> > > index fca3a609e0..cd0e39b789 100644
> > > --- a/tests/qemu-iotests/testenv.py
> > > +++ b/tests/qemu-iotests/testenv.py
> > > @@ -284,7 +284,8 @@ def print_env(self) -> None:
> > >   PLATFORM      -- {platform}
> > >   TEST_DIR      -- {TEST_DIR}
> > >   SOCK_DIR      -- {SOCK_DIR}
> > > -SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER}"""
> > > +SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER}
> > > +"""
> > >           args = collections.defaultdict(str, self.get_env())
> > > diff --git a/tests/qemu-iotests/testrunner.py 
> > > b/tests/qemu-iotests/testrunner.py
> > > index 519924dc81..2f56ac545d 100644
> > > --- a/tests/qemu-iotests/testrunner.py
> > > +++ b/tests/qemu-iotests/testrunner.py
> > > @@ -316,7 +316,6 @@ def run_tests(self, tests: List[str]) -> bool:
> > >           if not self.makecheck:
> > >               self.env.print_env()
> > > -            print()
> > >           test_field_width = max(len(os.path.basename(t)) for t in 
> > > tests) + 2