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

[Max Reitz <mreitz@redhat.com>]

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
>>>
>>
>