Re: [PATCH v2] iotests: Write test output to TEST_DIR

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

22.02.2022 17:44, Hanna Reitz wrote:
> On 22.02.22 15:39, Vladimir Sementsov-Ogievskiy wrote:
>> 21.02.2022 20:29, Hanna Reitz wrote:
>>> Drop the use of OUTPUT_DIR (test/qemu-iotests under the build
>>> directory), and instead write test output files (.out.bad, .notrun, and
>>> .casenotrun) to TEST_DIR.
>>>
>>> With this, the same test can be run concurrently without the separate
>>> instances interfering, because they will need separate TEST_DIRs anyway.
>>> Running the same test separately is useful when running the iotests with
>>> various format/protocol combinations in parallel, or when you just want
>>> to aggressively exercise a single test (e.g. when it fails only
>>> sporadically).
>>>
>>> Putting this output into TEST_DIR means that it will stick around for
>>> inspection after the test run is done (though running the same test in
>>> the same TEST_DIR will overwrite it, just as it used to be); but given
>>> that TEST_DIR is a scratch directory, it should be clear that users can
>>> delete all of its content at any point.  (And if TEST_DIR is on tmpfs,
>>> it will just disappear on shutdown.)  Contrarily, alternative approaches
>>> that would put these output files into OUTPUT_DIR with some prefix to
>>> differentiate between separate test runs might easily lead to cluttering
>>> OUTPUT_DIR.
>>>
>>> (This change means OUTPUT_DIR is no longer written to by the iotests, so
>>> we can drop its usage altogether.)
>>>
>>> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
>>> ---
>>> v1: https://lists.nongnu.org/archive/html/qemu-block/2022-02/msg00675.html
>>>
>>> v2:
>>> - Delete .casenotrun before running a test: Writes to this file only
>>>    append data, so if we do not delete it before a test run, it may still
>>>    contain stale data from a previous run
>>> - While at it, we might as well delete .notrun, because before this
>>>    patch, all of .out.bad, .notrun, and .casenotrun are deleted. (Really
>>>    no need to delete .out.bad, though, given it is immediately
>>>    overwritten after where we delete .notrun and .casenotrun.)
>>> ---
>>>   tests/qemu-iotests/common.rc     |  6 +++---
>>>   tests/qemu-iotests/iotests.py    |  5 ++---
>>>   tests/qemu-iotests/testenv.py    |  5 +----
>>>   tests/qemu-iotests/testrunner.py | 15 +++++++++------
>>>   4 files changed, 15 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
>>> index 9885030b43..5bde2415dc 100644
>>> --- a/tests/qemu-iotests/common.rc
>>> +++ b/tests/qemu-iotests/common.rc
>>> @@ -726,7 +726,7 @@ _img_info()
>>>   #
>>>   _notrun()
>>>   {
>>> -    echo "$*" >"$OUTPUT_DIR/$seq.notrun"
>>> +    echo "$*" >"$TEST_DIR/$seq.notrun"
>>>       echo "$seq not run: $*"
>>>       status=0
>>>       exit
>>> @@ -738,14 +738,14 @@ _notrun()
>>>   #
>>>   _casenotrun()
>>>   {
>>> -    echo "    [case not run] $*" >>"$OUTPUT_DIR/$seq.casenotrun"
>>> +    echo "    [case not run] $*" >>"$TEST_DIR/$seq.casenotrun"
>>>   }
>>>     # just plain bail out
>>>   #
>>>   _fail()
>>>   {
>>> -    echo "$*" | tee -a "$OUTPUT_DIR/$seq.full"
>>> +    echo "$*" | tee -a "$TEST_DIR/$seq.full"
>>>       echo "(see $seq.full for details)"
>>>       status=1
>>>       exit 1
>>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>>> index 6ba65eb1ff..1d157d1325 100644
>>> --- a/tests/qemu-iotests/iotests.py
>>> +++ b/tests/qemu-iotests/iotests.py
>>> @@ -84,7 +84,6 @@
>>>     imgfmt = os.environ.get('IMGFMT', 'raw')
>>>   imgproto = os.environ.get('IMGPROTO', 'file')
>>> -output_dir = os.environ.get('OUTPUT_DIR', '.')
>>>     try:
>>>       test_dir = os.environ['TEST_DIR']
>>> @@ -1209,7 +1208,7 @@ def notrun(reason):
>>>       # Each test in qemu-iotests has a number ("seq")
>>>       seq = os.path.basename(sys.argv[0])
>>>   -    with open('%s/%s.notrun' % (output_dir, seq), 'w', encoding='utf-8') \
>>> +    with open('%s/%s.notrun' % (test_dir, seq), 'w', encoding='utf-8') \
>>>               as outfile:
>>>           outfile.write(reason + '\n')
>>>       logger.warning("%s not run: %s", seq, reason)
>>> @@ -1224,7 +1223,7 @@ def case_notrun(reason):
>>>       # Each test in qemu-iotests has a number ("seq")
>>>       seq = os.path.basename(sys.argv[0])
>>>   -    with open('%s/%s.casenotrun' % (output_dir, seq), 'a', encoding='utf-8') \
>>> +    with open('%s/%s.casenotrun' % (test_dir, seq), 'a', encoding='utf-8') \
>>>               as outfile:
>>>           outfile.write('    [case not run] ' + reason + '\n')
>>>   diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
>>> index 0f32897fe8..b11e943c8a 100644
>>> --- a/tests/qemu-iotests/testenv.py
>>> +++ b/tests/qemu-iotests/testenv.py
>>> @@ -66,7 +66,7 @@ class TestEnv(ContextManager['TestEnv']):
>>>       # pylint: disable=too-many-instance-attributes
>>>         env_variables = ['PYTHONPATH', 'TEST_DIR', 'SOCK_DIR', 'SAMPLE_IMG_DIR',
>>> -                     'OUTPUT_DIR', 'PYTHON', 'QEMU_PROG', 'QEMU_IMG_PROG',
>>> +                     'PYTHON', 'QEMU_PROG', 'QEMU_IMG_PROG',
>>>                        'QEMU_IO_PROG', 'QEMU_NBD_PROG', 'QSD_PROG',
>>>                        'QEMU_OPTIONS', 'QEMU_IMG_OPTIONS',
>>>                        'QEMU_IO_OPTIONS', 'QEMU_IO_OPTIONS_NO_FMT',
>>> @@ -106,7 +106,6 @@ def init_directories(self) -> None:
>>>                TEST_DIR
>>>                SOCK_DIR
>>>                SAMPLE_IMG_DIR
>>> -             OUTPUT_DIR
>>>           """
>>>             # Path where qemu goodies live in this source tree.
>>> @@ -134,8 +133,6 @@ def init_directories(self) -> None:
>>> os.path.join(self.source_iotests,
>>> 'sample_images'))
>>>   -        self.output_dir = os.getcwd()  # OUTPUT_DIR
>>> -
>>>       def init_binaries(self) -> None:
>>>           """Init binary path variables:
>>>                PYTHON (for bash tests)
>>> diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
>>> index 0eace147b8..262b13004d 100644
>>> --- a/tests/qemu-iotests/testrunner.py
>>> +++ b/tests/qemu-iotests/testrunner.py
>>> @@ -259,9 +259,6 @@ def do_run_test(self, test: str, mp: bool) -> TestResult:
>>>           """
>>>             f_test = Path(test)
>>> -        f_bad = Path(f_test.name + '.out.bad')
>>> -        f_notrun = Path(f_test.name + '.notrun')
>>> -        f_casenotrun = Path(f_test.name + '.casenotrun')
>>>           f_reference = Path(self.find_reference(test))
>>>             if not f_test.exists():
>>> @@ -276,9 +273,6 @@ def do_run_test(self, test: str, mp: bool) -> TestResult:
>>>                                 description='No qualified output '
>>>                                             f'(expected {f_reference})')
>>>   -        for p in (f_bad, f_notrun, f_casenotrun):
>>> -            silent_unlink(p)
>>> -
>>>           args = [str(f_test.resolve())]
>>>           env = self.env.prepare_subprocess(args)
>>>           if mp:
>>> @@ -288,6 +282,15 @@ def do_run_test(self, test: str, mp: bool) -> TestResult:
>>>                   env[d] = os.path.join(env[d], f_test.name)
>>>                   Path(env[d]).mkdir(parents=True, exist_ok=True)
>>>   +        test_dir = env['TEST_DIR']
>>> +        f_bad = Path(os.path.join(test_dir, f_test.name + '.out.bad'))
>>> +        f_notrun = Path(os.path.join(test_dir, f_test.name + '.notrun'))
>>> +        f_casenotrun = Path(os.path.join(test_dir,
>>> +                                         f_test.name + '.casenotrun'))
>>
>> You don't need os.path.join inside Path(), simple
>>
>>  Path(test_dir, f_test.name + '...')
>>
>> should work.
> 
> Oh, good!
> 
>>> +
>>> +        for p in (f_notrun, f_casenotrun):
>>> +            silent_unlink(p)
>>
>> Why don't you want to remove old f_bad too, like pre-patch?
> 
> Mainly because...
> 
>>> +
>>>           t0 = time.time()
>>>           with f_bad.open('w', encoding="utf-8") as f:
> 
> I found it’d look just a bit silly when we immediately overwrite it here anyway.  But if you prefer to keep the pre-patch list for completeness’ sake, I won’t mind.

Ah, right, we rewrite it immediately, OK for me.

With simplified Path(..) constructors:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> 
>>>               with subprocess.Popen(args, cwd=str(f_test.parent), env=env,
>>
>>
> 


-- 
Best regards,
Vladimir