Re: [Qemu-devel] [PATCH v3] tests/qemu-iotests: re-format output to for make check-block

[Thomas Huth <thuth@redhat.com>]

On 10/05/2019 13.10, Alex Bennée wrote:
> 
> Thomas Huth <thuth@redhat.com> writes:
> 
>> On 10/05/2019 12.29, Alex Bennée wrote:
>>> This attempts to clean-up the output to better match the output of the
>>> rest of the QEMU check system when called with -pretty. This includes:
>>>
>>>   - formatting as "  TEST    iotest: nnn"
>>>   - calculating time diff at the end
>>>   - only dumping config on failure (when -pretty enabled)
>>>
>>> The existing output is mostly preserved although the dumping of the
>>> old time at the start "Ns ..." was removed to keep the logic simple.
>>> The timestamp mode can still be used to see which tests are "hanging".
>>>
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> Message-Id: <20190503143904.31211-1-alex.bennee@linaro.org>
>>>
>>> ---
>>> v3
>>>   - revert echo to printf
>>>   - add _report_test_start
>>> ---
>>>  tests/qemu-iotests/check | 101 ++++++++++++++++++++++++++-------------
>>>  1 file changed, 68 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
>>> index 922c5d1d3d3..ac481f905bf 100755
>>> --- a/tests/qemu-iotests/check
>>> +++ b/tests/qemu-iotests/check
>>> @@ -27,6 +27,7 @@ bad=""
>>>  notrun=""
>>>  casenotrun=""
>>>  interrupt=true
>>> +pretty=false
>>>
>>>  # by default don't output timestamps
>>>  timestamp=${TIMESTAMP:=false}
>>> @@ -88,6 +89,22 @@ _full_platform_details()
>>>      echo "$os/$platform $host $kernel"
>>>  }
>>>
>>> +_full_env_details()
>>> +{
>>> +    cat <<EOF
>>> +QEMU          -- "$QEMU_PROG" $QEMU_OPTIONS
>>> +QEMU_IMG      -- "$QEMU_IMG_PROG" $QEMU_IMG_OPTIONS
>>> +QEMU_IO       -- "$QEMU_IO_PROG" $QEMU_IO_OPTIONS
>>> +QEMU_NBD      -- "$QEMU_NBD_PROG" $QEMU_NBD_OPTIONS
>>> +IMGFMT        -- $FULL_IMGFMT_DETAILS
>>> +IMGPROTO      -- $IMGPROTO
>>> +PLATFORM      -- $FULL_HOST_DETAILS
>>> +TEST_DIR      -- $TEST_DIR
>>> +SOCKET_SCM_HELPER -- $SOCKET_SCM_HELPER
>>> +
>>> +EOF
>>> +}
>>> +
>>>  # $1 = prog to look for
>>>  set_prog_path()
>>>  {
>>> @@ -256,6 +273,7 @@ other options
>>>      -o options          -o options to pass to qemu-img create/convert
>>>      -T                  output timestamps
>>>      -c mode             cache mode
>>> +    -pretty             pretty print output for make check
>>
>> "pretty" is likely just a matter of taste ... so maybe this should be
>> named differently instead? "--makecheck" ? Or "--one-shot" ?
>>
>>>  testlist options
>>>      -g group[,group...]        include tests from these groups
>>> @@ -403,7 +421,10 @@ testlist options
>>>                  command -v xxdiff >/dev/null 2>&1 && diff=xxdiff
>>>              fi
>>>              ;;
>>> -
>>> +        -pretty)   # pretty print output
>>> +            pretty=true
>>> +            xpand=false
>>> +            ;;
>>>          -n)        # show me, don't do it
>>>              showme=true
>>>              xpand=false
>>> @@ -704,23 +725,30 @@ END        { if (NR > 0) {
>>>
>>>  trap "_wrapup; exit \$status" 0 1 2 3 15
>>>
>>> +# Report the test start and results, optionally pretty printing for make
>>> +# args: $seq
>>> +_report_test_start()
>>> +{
>>> +    if $pretty; then
>>> +        printf "  TEST    iotest: %s" "$1"
>>
>> Could you maybe change the "iotest:" into "iotest-$IMGFMT:" ? ... so
>> that when you run "make check SPEED=slow" you also see which kind of
>> format is currently under test?
> 
> Sure I can do that.
> 
>>
>> And this currently also does not play very nicely when running "make -j8
>> check" in parallel:
>>
>>   [...]
>>   TEST    iotest: 001  TEST    check-qtest-alpha: tests/qmp-test
>>   TEST    check-qtest-alpha: tests/qmp-cmd-test
>>   TEST    check-qtest-aarch64: tests/boot-serial-test
>>   TEST    check-qtest-aarch64: tests/migration-test
>>   TEST    check-qtest-arm: tests/tmp105-test
>>   TEST    check-unit: tests/check-qnum
>>   TEST    check-unit: tests/check-qstring
>>   TEST    check-unit: tests/check-qlist
>>   TEST    check-unit: tests/check-qnull
>>  2s (last 2s)
>>   TEST    iotest: 002  TEST    check-qtest-arm: tests/pca9552-test
>>   TEST    check-unit: tests/check-qobject
>>   TEST    check-qtest-cris: tests/qmp-test
>>   [...]
>>
>> I think the "make check" mode should only print out one time for each
>> test, preferable at the end, like the other tests (like qtests) are
>> doing it...?
> 
> *sigh* and this is of course why deferred everything to the end in the
> last revision. Should we just assume the -pretty/-make whatever is
> incompatible with -T for the timestamp mode?

Fine for me. ... and maybe that's one more reason to call the parameter
rather "-makecheck" or so instead, so that it is clear that this mode is
not to be mixed with other parameters that influence the output.

 Thomas