[PATCH 0/3] separated wanted and unwanted skips in QEMU iotests

[PATCH 0/3] separated wanted and unwanted skips in QEMU iotests

[Denis V. Lunev]

Some time ago there was a discussion in the QEMU mailing list about
the situation when some IO tests were not run and thus the patch
has added a regression, namely
    https://lists.gnu.org/archive/html/qemu-devel/2023-08/msg02381.html

This series contains a possible answer to the problem. Let us separate
intentional and unwanted skipping of the testcase. This patch adds new
state - 'skipped' and that state is considered as a normal skip. In the
other case we do not want that.

The series also contains some fixes which were revealed once the feature
was implemented.

Hope this would not be too bad :)

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Hanna Reitz <hreitz@redhat.com>
CC: Eric Blake <eblake@redhat.com>
CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

[PATCH 1/3] iotests: use TEST_IMG_FILE instead of TEST_IMG in _require_large_file

[Denis V. Lunev]

We need to check that we are able to create large enough file which is
used as an export base rather than connection URL. Unfortunately, there
are cases when the TEST_IMG_FILE is not defined. We should fallback to
TEST_IMG in that case.

This problem has been detected when running
    ./check -nbd 5
The test should be able to run while it does not.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Hanna Reitz <hreitz@redhat.com>
CC: Eric Blake <eblake@redhat.com>
CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 tests/qemu-iotests/common.rc | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index d145f08201..95c12577dd 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -979,10 +979,15 @@ _require_drivers()
 #
 _require_large_file()
 {
-    if ! truncate --size="$1" "$TEST_IMG"; then
+    if [ -z "$TEST_IMG_FILE" ]; then
+        FILENAME="$TEST_IMG"
+    else
+        FILENAME="$TEST_IMG_FILE"
+    fi
+    if ! truncate --size="$1" "$FILENAME"; then
         _notrun "file system on $TEST_DIR does not support large enough files"
     fi
-    rm "$TEST_IMG"
+    rm "$FILENAME"
 }
 
 # Check that a set of devices is available in the QEMU binary
-- 
2.34.1

Re: [PATCH 1/3] iotests: use TEST_IMG_FILE instead of TEST_IMG in _require_large_file

[Eric Blake]

On Wed, Sep 06, 2023 at 04:09:15PM +0200, Denis V. Lunev wrote:
> We need to check that we are able to create large enough file which is
> used as an export base rather than connection URL. Unfortunately, there
> are cases when the TEST_IMG_FILE is not defined. We should fallback to
> TEST_IMG in that case.
> 
> This problem has been detected when running
>     ./check -nbd 5
> The test should be able to run while it does not.

Pre-patch it skips, post-patch it passes.  Nice observation!
Tested-by: Eric Blake <eblake@redhat.com>

> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Hanna Reitz <hreitz@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  tests/qemu-iotests/common.rc | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

[PATCH 2/3] iotests: improve 'not run' message for nbd-multiconn test

[Denis V. Lunev]

The test actually requires Python bindings to libnbd rather than libnbd
itself. Clarify that inside the message.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Hanna Reitz <hreitz@redhat.com>
CC: Eric Blake <eblake@redhat.com>
CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 tests/qemu-iotests/tests/nbd-multiconn | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/tests/nbd-multiconn b/tests/qemu-iotests/tests/nbd-multiconn
index b121f2e363..478a1eaba2 100755
--- a/tests/qemu-iotests/tests/nbd-multiconn
+++ b/tests/qemu-iotests/tests/nbd-multiconn
@@ -142,4 +142,4 @@ if __name__ == '__main__':
 
         iotests.main(supported_fmts=['qcow2'])
     except ImportError:
-        iotests.notrun('libnbd not installed')
+        iotests.notrun('Python bindings to libnbd are not installed')
-- 
2.34.1

Re: [PATCH 2/3] iotests: improve 'not run' message for nbd-multiconn test

[Vladimir Sementsov-Ogievskiy]

On 06.09.23 17:09, Denis V. Lunev wrote:
> The test actually requires Python bindings to libnbd rather than libnbd
> itself. Clarify that inside the message.
> 
> Signed-off-by: Denis V. Lunev<den@openvz.org>
> CC: Kevin Wolf<kwolf@redhat.com>
> CC: Hanna Reitz<hreitz@redhat.com>
> CC: Eric Blake<eblake@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

-- 
Best regards,
Vladimir

Re: [PATCH 2/3] iotests: improve 'not run' message for nbd-multiconn test

[Eric Blake]

On Wed, Sep 06, 2023 at 04:09:16PM +0200, Denis V. Lunev wrote:
> The test actually requires Python bindings to libnbd rather than libnbd
> itself. Clarify that inside the message.

Makes sense; distros have the freedom to break the upstream monolithic
libnbd project into smaller distribution packages for minimizing
dependencies for different functionalities.

> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Hanna Reitz <hreitz@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  tests/qemu-iotests/tests/nbd-multiconn | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/tests/qemu-iotests/tests/nbd-multiconn b/tests/qemu-iotests/tests/nbd-multiconn
> index b121f2e363..478a1eaba2 100755
> --- a/tests/qemu-iotests/tests/nbd-multiconn
> +++ b/tests/qemu-iotests/tests/nbd-multiconn
> @@ -142,4 +142,4 @@ if __name__ == '__main__':
>  
>          iotests.main(supported_fmts=['qcow2'])
>      except ImportError:
> -        iotests.notrun('libnbd not installed')
> +        iotests.notrun('Python bindings to libnbd are not installed')
> -- 
> 2.34.1
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

[PATCH 3/3] iotests: distinguish 'skipped' and 'not run' states

[Denis V. Lunev]

Each particular testcase could skipped intentionally and accidentally.
For example the test is not designed for a particular image format or
is not run due to the missed library.

The latter case is unwanted in reality. Though the discussion has
revealed that failing the test in such a case would be bad. Thus the
patch tries to do different thing. It adds additional status for
the test case - 'skipped' and bound intentinal cases to that state.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Hanna Reitz <hreitz@redhat.com>
CC: Eric Blake <eblake@redhat.com>
CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 tests/qemu-iotests/common.rc     | 23 ++++++++++++++++-------
 tests/qemu-iotests/iotests.py    | 19 +++++++++++++++----
 tests/qemu-iotests/testrunner.py | 17 +++++++++++++++--
 3 files changed, 46 insertions(+), 13 deletions(-)

diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 95c12577dd..74f64e8af8 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -37,6 +37,15 @@ _notrun()
     exit
 }
 
+# bail out, setting up .skip file
+_skip()
+{
+    echo "$*" >"$TEST_DIR/$seq.skip"
+    echo "$seq skipped: $*"
+    status=0
+    exit
+}
+
 if ! command -v gsed >/dev/null 2>&1; then
     if sed --version 2>&1 | grep -v 'not GNU sed' | grep 'GNU sed' > /dev/null;
     then
@@ -782,7 +791,7 @@ _supported_fmt()
         fi
     done
 
-    _notrun "not suitable for this image format: $IMGFMT"
+    _skip "not suitable for this image format: $IMGFMT"
 }
 
 # tests whether $IMGFMT is one of the unsupported image format for a test
@@ -791,7 +800,7 @@ _unsupported_fmt()
 {
     for f; do
         if [ "$f" = "$IMGFMT" ]; then
-            _notrun "not suitable for this image format: $IMGFMT"
+            _skip "not suitable for this image format: $IMGFMT"
         fi
     done
 }
@@ -806,7 +815,7 @@ _supported_proto()
         fi
     done
 
-    _notrun "not suitable for this image protocol: $IMGPROTO"
+    _skip "not suitable for this image protocol: $IMGPROTO"
 }
 
 # tests whether $IMGPROTO is specified as an unsupported image protocol for a test
@@ -815,7 +824,7 @@ _unsupported_proto()
 {
     for f; do
         if [ "$f" = "$IMGPROTO" ]; then
-            _notrun "not suitable for this image protocol: $IMGPROTO"
+            _skip "not suitable for this image protocol: $IMGPROTO"
             return
         fi
     done
@@ -843,7 +852,7 @@ _supported_cache_modes()
             return
         fi
     done
-    _notrun "not suitable for cache mode: $CACHEMODE"
+    _skip "not suitable for cache mode: $CACHEMODE"
 }
 
 # Check whether the filesystem supports O_DIRECT
@@ -895,7 +904,7 @@ _supported_aio_modes()
             return
         fi
     done
-    _notrun "not suitable for aio mode: $AIOMODE"
+    _skip "not suitable for aio mode: $AIOMODE"
 }
 _default_aio_mode()
 {
@@ -911,7 +920,7 @@ _unsupported_imgopts()
         # end of an option (\b or \> are not portable)
         if echo "$IMGOPTS " | grep -q 2>/dev/null "$bad_opt"
         then
-            _notrun "not suitable for image option: $bad_opt"
+            _skip "not suitable for image option: $bad_opt"
         fi
     done
 }
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index ef66fbd62b..746772fab0 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -1353,6 +1353,17 @@ def notrun(reason):
     logger.warning("%s not run: %s", seq, reason)
     sys.exit(0)
 
+def skip(reason):
+    '''Skip this test suite for a purpose'''
+    # Each test in qemu-iotests has a number ("seq")
+    seq = os.path.basename(sys.argv[0])
+
+    with open('%s/%s.skip' % (test_dir, seq), 'w', encoding='utf-8') \
+            as outfile:
+        outfile.write(reason + '\n')
+    logger.warning("%s not run: %s", seq, reason)
+    sys.exit(0)
+
 def case_notrun(reason):
     '''Mark this test case as not having been run (without actually
     skipping it, that is left to the caller).  See
@@ -1377,7 +1388,7 @@ def _verify_image_format(supported_fmts: Sequence[str] = (),
 
     not_sup = supported_fmts and (imgfmt not in supported_fmts)
     if not_sup or (imgfmt in unsupported_fmts):
-        notrun('not suitable for this image format: %s' % imgfmt)
+        skip('not suitable for this image format: %s' % imgfmt)
 
     if imgfmt == 'luks':
         verify_working_luks()
@@ -1391,7 +1402,7 @@ def _verify_protocol(supported: Sequence[str] = (),
 
     not_sup = supported and (imgproto not in supported)
     if not_sup or (imgproto in unsupported):
-        notrun('not suitable for this protocol: %s' % imgproto)
+        skip('not suitable for this protocol: %s' % imgproto)
 
 def _verify_platform(supported: Sequence[str] = (),
                      unsupported: Sequence[str] = ()) -> None:
@@ -1404,11 +1415,11 @@ def _verify_platform(supported: Sequence[str] = (),
 
 def _verify_cache_mode(supported_cache_modes: Sequence[str] = ()) -> None:
     if supported_cache_modes and (cachemode not in supported_cache_modes):
-        notrun('not suitable for this cache mode: %s' % cachemode)
+        skip('not suitable for this cache mode: %s' % cachemode)
 
 def _verify_aio_mode(supported_aio_modes: Sequence[str] = ()) -> None:
     if supported_aio_modes and (aiomode not in supported_aio_modes):
-        notrun('not suitable for this aio mode: %s' % aiomode)
+        skip('not suitable for this aio mode: %s' % aiomode)
 
 def _verify_formats(required_formats: Sequence[str] = ()) -> None:
     usf_list = list(set(required_formats) - set(supported_formats()))
diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
index 7b322272e9..137dd6e06c 100644
--- a/tests/qemu-iotests/testrunner.py
+++ b/tests/qemu-iotests/testrunner.py
@@ -200,6 +200,8 @@ def test_print_one_line(self, test: str,
                 col = '\033[1m\033[31m'
             elif status == 'not run':
                 col = '\033[33m'
+            elif status == 'skipped':
+                col = '\033[34m'
             else:
                 col = ''
 
@@ -268,8 +270,9 @@ def do_run_test(self, test: str) -> TestResult:
         f_bad = Path(test_dir, f_test.name + '.out.bad')
         f_notrun = Path(test_dir, f_test.name + '.notrun')
         f_casenotrun = Path(test_dir, f_test.name + '.casenotrun')
+        f_skipped = Path(test_dir, f_test.name + '.skip')
 
-        for p in (f_notrun, f_casenotrun):
+        for p in (f_notrun, f_casenotrun, f_skipped):
             silent_unlink(p)
 
         t0 = time.time()
@@ -298,6 +301,10 @@ def do_run_test(self, test: str) -> TestResult:
             return TestResult(
                 status='not run',
                 description=f_notrun.read_text(encoding='utf-8').strip())
+        if f_skipped.exists():
+            return TestResult(
+                status='skipped',
+                description=f_skipped.read_text(encoding='utf-8').strip())
 
         casenotrun = ''
         if f_casenotrun.exists():
@@ -370,6 +377,7 @@ def run_tests(self, tests: List[str], jobs: int = 1) -> bool:
         n_run = 0
         failed = []
         notrun = []
+        skipped = []
         casenotrun = []
 
         if self.tap:
@@ -392,7 +400,7 @@ def run_tests(self, tests: List[str], jobs: int = 1) -> bool:
             else:
                 res = self.run_test(t, test_field_width)
 
-            assert res.status in ('pass', 'fail', 'not run')
+            assert res.status in ('pass', 'fail', 'not run', 'skipped')
 
             if res.casenotrun:
                 casenotrun.append(t)
@@ -409,6 +417,8 @@ def run_tests(self, tests: List[str], jobs: int = 1) -> bool:
                         print('\n'.join(res.diff))
             elif res.status == 'not run':
                 notrun.append(name)
+            elif res.status == 'skipped':
+                skipped.append(name)
             elif res.status == 'pass':
                 assert res.elapsed is not None
                 self.last_elapsed.update(t, res.elapsed)
@@ -418,6 +428,9 @@ def run_tests(self, tests: List[str], jobs: int = 1) -> bool:
                 break
 
         if not self.tap:
+            if skipped:
+                print('Skipped:', ' '.join(skipped))
+
             if notrun:
                 print('Not run:', ' '.join(notrun))
 
-- 
2.34.1

Re: [PATCH 3/3] iotests: distinguish 'skipped' and 'not run' states

[Vladimir Sementsov-Ogievskiy]

On 06.09.23 17:09, Denis V. Lunev wrote:
> Each particular testcase could skipped intentionally and accidentally.
> For example the test is not designed for a particular image format or
> is not run due to the missed library.
> 
> The latter case is unwanted in reality. Though the discussion has
> revealed that failing the test in such a case would be bad. Thus the
> patch tries to do different thing. It adds additional status for
> the test case - 'skipped' and bound intentinal cases to that state.

Hmm. Do I miss something, or in this patch you only split them, not making "not run" produce an error? So ./check still reports success when some tests are "not run"?

The split itself looks correct to me.

-- 
Best regards,
Vladimir

Re: [PATCH 3/3] iotests: distinguish 'skipped' and 'not run' states

[Denis V. Lunev]

On 9/12/23 22:03, Vladimir Sementsov-Ogievskiy wrote:
> On 06.09.23 17:09, Denis V. Lunev wrote:
>> Each particular testcase could skipped intentionally and accidentally.
>> For example the test is not designed for a particular image format or
>> is not run due to the missed library.
>>
>> The latter case is unwanted in reality. Though the discussion has
>> revealed that failing the test in such a case would be bad. Thus the
>> patch tries to do different thing. It adds additional status for
>> the test case - 'skipped' and bound intentinal cases to that state.
>
> Hmm. Do I miss something, or in this patch you only split them, not 
> making "not run" produce an error? So ./check still reports success 
> when some tests are "not run"?
>
> The split itself looks correct to me.
>
The original talk was to avoid failing of such tests.
If we would let them fail - that could be done much
faster and without dances.

Thus tests are still counted as "skipped other way".
But I would definitely like to concentrate my attention
on something abnormal, i.e. things that should be
run but don't run.

These tests count be missed due to error (see patch 1)
or due to the missed package. And the reason of skip
is barely visible within the list of 100+ tests. While
1-2-3 "should not be skipped" tests are clearly in the
focus.

Den

Re: [PATCH 3/3] iotests: distinguish 'skipped' and 'not run' states

[Eric Blake]

On Wed, Sep 06, 2023 at 04:09:17PM +0200, Denis V. Lunev wrote:
> Each particular testcase could skipped intentionally and accidentally.
> For example the test is not designed for a particular image format or
> is not run due to the missed library.
> 
> The latter case is unwanted in reality. Though the discussion has
> revealed that failing the test in such a case would be bad. Thus the
> patch tries to do different thing. It adds additional status for
> the test case - 'skipped' and bound intentinal cases to that state.

intentional

> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Hanna Reitz <hreitz@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  tests/qemu-iotests/common.rc     | 23 ++++++++++++++++-------
>  tests/qemu-iotests/iotests.py    | 19 +++++++++++++++----
>  tests/qemu-iotests/testrunner.py | 17 +++++++++++++++--
>  3 files changed, 46 insertions(+), 13 deletions(-)

I agree that this makes it easier to identify skipped tests that can
never be run (not designed for that combination) vs tests that can
plausibly be run by addressing missing dependencies (such as
installing additional libraries, a different file system, ...).

Reviewed-by: Eric Blake <eblake@redhat.com>

Because patch 1 impacts NBD testing, I can queue the series through my
NBD tree if no one else picks it up first.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH 3/3] iotests: distinguish 'skipped' and 'not run' states

[Eric Blake]

On Wed, Sep 13, 2023 at 10:36:35AM -0500, Eric Blake wrote:
> On Wed, Sep 06, 2023 at 04:09:17PM +0200, Denis V. Lunev wrote:
> > Each particular testcase could skipped intentionally and accidentally.
> > For example the test is not designed for a particular image format or
> > is not run due to the missed library.
> > 
> > The latter case is unwanted in reality. Though the discussion has
> > revealed that failing the test in such a case would be bad. Thus the
> > patch tries to do different thing. It adds additional status for
> > the test case - 'skipped' and bound intentinal cases to that state.
> 
> intentional
> 
> > 
> > Signed-off-by: Denis V. Lunev <den@openvz.org>
> > CC: Kevin Wolf <kwolf@redhat.com>
> > CC: Hanna Reitz <hreitz@redhat.com>
> > CC: Eric Blake <eblake@redhat.com>
> > CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> > ---
> >  tests/qemu-iotests/common.rc     | 23 ++++++++++++++++-------
> >  tests/qemu-iotests/iotests.py    | 19 +++++++++++++++----
> >  tests/qemu-iotests/testrunner.py | 17 +++++++++++++++--
> >  3 files changed, 46 insertions(+), 13 deletions(-)
> 
> I agree that this makes it easier to identify skipped tests that can
> never be run (not designed for that combination) vs tests that can
> plausibly be run by addressing missing dependencies (such as
> installing additional libraries, a different file system, ...).
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> Because patch 1 impacts NBD testing, I can queue the series through my
> NBD tree if no one else picks it up first.

Given further conversation, I've removed patch 3 from my queue while
conversation continues, but patch 1 and 2 are still queued (they are
independently useful, whether or not we add another result category)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH 3/3] iotests: distinguish 'skipped' and 'not run' states

[Denis V. Lunev]

On 9/14/23 16:10, Eric Blake wrote:
> On Wed, Sep 13, 2023 at 10:36:35AM -0500, Eric Blake wrote:
>> On Wed, Sep 06, 2023 at 04:09:17PM +0200, Denis V. Lunev wrote:
>>> Each particular testcase could skipped intentionally and accidentally.
>>> For example the test is not designed for a particular image format or
>>> is not run due to the missed library.
>>>
>>> The latter case is unwanted in reality. Though the discussion has
>>> revealed that failing the test in such a case would be bad. Thus the
>>> patch tries to do different thing. It adds additional status for
>>> the test case - 'skipped' and bound intentinal cases to that state.
>> intentional
>>
>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>> CC: Kevin Wolf <kwolf@redhat.com>
>>> CC: Hanna Reitz <hreitz@redhat.com>
>>> CC: Eric Blake <eblake@redhat.com>
>>> CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>> ---
>>>   tests/qemu-iotests/common.rc     | 23 ++++++++++++++++-------
>>>   tests/qemu-iotests/iotests.py    | 19 +++++++++++++++----
>>>   tests/qemu-iotests/testrunner.py | 17 +++++++++++++++--
>>>   3 files changed, 46 insertions(+), 13 deletions(-)
>> I agree that this makes it easier to identify skipped tests that can
>> never be run (not designed for that combination) vs tests that can
>> plausibly be run by addressing missing dependencies (such as
>> installing additional libraries, a different file system, ...).
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>
>> Because patch 1 impacts NBD testing, I can queue the series through my
>> NBD tree if no one else picks it up first.
> Given further conversation, I've removed patch 3 from my queue while
> conversation continues, but patch 1 and 2 are still queued (they are
> independently useful, whether or not we add another result category)
>
clear...

For me such a filter was very useful, as we
* have had problems in the past with the missed test cases due to 
non-satisfied reqs
* this separation gives positive feedback with test fixes (2 patches in 
the serie)
* there is no idea how this could be done in the ideal case and there is 
no idea
    how unclear ideal could be reached. Having something simple/useful looks
    preferred to me.

Den

Re: [PATCH 3/3] iotests: distinguish 'skipped' and 'not run' states

[Daniel P. Berrangé]

On Wed, Sep 06, 2023 at 04:09:17PM +0200, Denis V. Lunev wrote:
> Each particular testcase could skipped intentionally and accidentally.
> For example the test is not designed for a particular image format or
> is not run due to the missed library.
> 
> The latter case is unwanted in reality. Though the discussion has
> revealed that failing the test in such a case would be bad. Thus the
> patch tries to do different thing. It adds additional status for
> the test case - 'skipped' and bound intentinal cases to that state.

I'm not convinced this distinction makes sense and I fear it is
leading us down the same undesirable route as avocado with too
many distinct states leading to confusion:

  https://lore.kernel.org/qemu-devel/Yy1gB1KB3YSIUcoC@redhat.com/

If I looked at the output I can't tell you the difference between
"not run" and "skipped" - they both sound the same to me.

IMHO there's alot to be said for the simplicity of sticking with
nothing more than PASS/FAIL/SKIP as status names.  The descriptive
text associated with each SKIP would give more context as to the
reason in each case if needed.

> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Hanna Reitz <hreitz@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  tests/qemu-iotests/common.rc     | 23 ++++++++++++++++-------
>  tests/qemu-iotests/iotests.py    | 19 +++++++++++++++----
>  tests/qemu-iotests/testrunner.py | 17 +++++++++++++++--
>  3 files changed, 46 insertions(+), 13 deletions(-)
> 
> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
> index 95c12577dd..74f64e8af8 100644
> --- a/tests/qemu-iotests/common.rc
> +++ b/tests/qemu-iotests/common.rc
> @@ -37,6 +37,15 @@ _notrun()
>      exit
>  }
>  
> +# bail out, setting up .skip file
> +_skip()
> +{
> +    echo "$*" >"$TEST_DIR/$seq.skip"
> +    echo "$seq skipped: $*"
> +    status=0
> +    exit
> +}
> +
>  if ! command -v gsed >/dev/null 2>&1; then
>      if sed --version 2>&1 | grep -v 'not GNU sed' | grep 'GNU sed' > /dev/null;
>      then
> @@ -782,7 +791,7 @@ _supported_fmt()
>          fi
>      done
>  
> -    _notrun "not suitable for this image format: $IMGFMT"
> +    _skip "not suitable for this image format: $IMGFMT"
>  }
>  
>  # tests whether $IMGFMT is one of the unsupported image format for a test
> @@ -791,7 +800,7 @@ _unsupported_fmt()
>  {
>      for f; do
>          if [ "$f" = "$IMGFMT" ]; then
> -            _notrun "not suitable for this image format: $IMGFMT"
> +            _skip "not suitable for this image format: $IMGFMT"
>          fi
>      done
>  }
> @@ -806,7 +815,7 @@ _supported_proto()
>          fi
>      done
>  
> -    _notrun "not suitable for this image protocol: $IMGPROTO"
> +    _skip "not suitable for this image protocol: $IMGPROTO"
>  }
>  
>  # tests whether $IMGPROTO is specified as an unsupported image protocol for a test
> @@ -815,7 +824,7 @@ _unsupported_proto()
>  {
>      for f; do
>          if [ "$f" = "$IMGPROTO" ]; then
> -            _notrun "not suitable for this image protocol: $IMGPROTO"
> +            _skip "not suitable for this image protocol: $IMGPROTO"
>              return
>          fi
>      done
> @@ -843,7 +852,7 @@ _supported_cache_modes()
>              return
>          fi
>      done
> -    _notrun "not suitable for cache mode: $CACHEMODE"
> +    _skip "not suitable for cache mode: $CACHEMODE"
>  }
>  
>  # Check whether the filesystem supports O_DIRECT
> @@ -895,7 +904,7 @@ _supported_aio_modes()
>              return
>          fi
>      done
> -    _notrun "not suitable for aio mode: $AIOMODE"
> +    _skip "not suitable for aio mode: $AIOMODE"
>  }
>  _default_aio_mode()
>  {
> @@ -911,7 +920,7 @@ _unsupported_imgopts()
>          # end of an option (\b or \> are not portable)
>          if echo "$IMGOPTS " | grep -q 2>/dev/null "$bad_opt"
>          then
> -            _notrun "not suitable for image option: $bad_opt"
> +            _skip "not suitable for image option: $bad_opt"
>          fi
>      done
>  }
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index ef66fbd62b..746772fab0 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -1353,6 +1353,17 @@ def notrun(reason):
>      logger.warning("%s not run: %s", seq, reason)
>      sys.exit(0)
>  
> +def skip(reason):
> +    '''Skip this test suite for a purpose'''
> +    # Each test in qemu-iotests has a number ("seq")
> +    seq = os.path.basename(sys.argv[0])
> +
> +    with open('%s/%s.skip' % (test_dir, seq), 'w', encoding='utf-8') \
> +            as outfile:
> +        outfile.write(reason + '\n')
> +    logger.warning("%s not run: %s", seq, reason)
> +    sys.exit(0)
> +
>  def case_notrun(reason):
>      '''Mark this test case as not having been run (without actually
>      skipping it, that is left to the caller).  See
> @@ -1377,7 +1388,7 @@ def _verify_image_format(supported_fmts: Sequence[str] = (),
>  
>      not_sup = supported_fmts and (imgfmt not in supported_fmts)
>      if not_sup or (imgfmt in unsupported_fmts):
> -        notrun('not suitable for this image format: %s' % imgfmt)
> +        skip('not suitable for this image format: %s' % imgfmt)
>  
>      if imgfmt == 'luks':
>          verify_working_luks()
> @@ -1391,7 +1402,7 @@ def _verify_protocol(supported: Sequence[str] = (),
>  
>      not_sup = supported and (imgproto not in supported)
>      if not_sup or (imgproto in unsupported):
> -        notrun('not suitable for this protocol: %s' % imgproto)
> +        skip('not suitable for this protocol: %s' % imgproto)
>  
>  def _verify_platform(supported: Sequence[str] = (),
>                       unsupported: Sequence[str] = ()) -> None:
> @@ -1404,11 +1415,11 @@ def _verify_platform(supported: Sequence[str] = (),
>  
>  def _verify_cache_mode(supported_cache_modes: Sequence[str] = ()) -> None:
>      if supported_cache_modes and (cachemode not in supported_cache_modes):
> -        notrun('not suitable for this cache mode: %s' % cachemode)
> +        skip('not suitable for this cache mode: %s' % cachemode)
>  
>  def _verify_aio_mode(supported_aio_modes: Sequence[str] = ()) -> None:
>      if supported_aio_modes and (aiomode not in supported_aio_modes):
> -        notrun('not suitable for this aio mode: %s' % aiomode)
> +        skip('not suitable for this aio mode: %s' % aiomode)
>  
>  def _verify_formats(required_formats: Sequence[str] = ()) -> None:
>      usf_list = list(set(required_formats) - set(supported_formats()))
> diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
> index 7b322272e9..137dd6e06c 100644
> --- a/tests/qemu-iotests/testrunner.py
> +++ b/tests/qemu-iotests/testrunner.py
> @@ -200,6 +200,8 @@ def test_print_one_line(self, test: str,
>                  col = '\033[1m\033[31m'
>              elif status == 'not run':
>                  col = '\033[33m'
> +            elif status == 'skipped':
> +                col = '\033[34m'
>              else:
>                  col = ''
>  
> @@ -268,8 +270,9 @@ def do_run_test(self, test: str) -> TestResult:
>          f_bad = Path(test_dir, f_test.name + '.out.bad')
>          f_notrun = Path(test_dir, f_test.name + '.notrun')
>          f_casenotrun = Path(test_dir, f_test.name + '.casenotrun')
> +        f_skipped = Path(test_dir, f_test.name + '.skip')
>  
> -        for p in (f_notrun, f_casenotrun):
> +        for p in (f_notrun, f_casenotrun, f_skipped):
>              silent_unlink(p)
>  
>          t0 = time.time()
> @@ -298,6 +301,10 @@ def do_run_test(self, test: str) -> TestResult:
>              return TestResult(
>                  status='not run',
>                  description=f_notrun.read_text(encoding='utf-8').strip())
> +        if f_skipped.exists():
> +            return TestResult(
> +                status='skipped',
> +                description=f_skipped.read_text(encoding='utf-8').strip())
>  
>          casenotrun = ''
>          if f_casenotrun.exists():
> @@ -370,6 +377,7 @@ def run_tests(self, tests: List[str], jobs: int = 1) -> bool:
>          n_run = 0
>          failed = []
>          notrun = []
> +        skipped = []
>          casenotrun = []
>  
>          if self.tap:
> @@ -392,7 +400,7 @@ def run_tests(self, tests: List[str], jobs: int = 1) -> bool:
>              else:
>                  res = self.run_test(t, test_field_width)
>  
> -            assert res.status in ('pass', 'fail', 'not run')
> +            assert res.status in ('pass', 'fail', 'not run', 'skipped')
>  
>              if res.casenotrun:
>                  casenotrun.append(t)
> @@ -409,6 +417,8 @@ def run_tests(self, tests: List[str], jobs: int = 1) -> bool:
>                          print('\n'.join(res.diff))
>              elif res.status == 'not run':
>                  notrun.append(name)
> +            elif res.status == 'skipped':
> +                skipped.append(name)
>              elif res.status == 'pass':
>                  assert res.elapsed is not None
>                  self.last_elapsed.update(t, res.elapsed)
> @@ -418,6 +428,9 @@ def run_tests(self, tests: List[str], jobs: int = 1) -> bool:
>                  break
>  
>          if not self.tap:
> +            if skipped:
> +                print('Skipped:', ' '.join(skipped))
> +
>              if notrun:
>                  print('Not run:', ' '.join(notrun))
>  
> -- 
> 2.34.1
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 3/3] iotests: distinguish 'skipped' and 'not run' states

[Eric Blake]

On Wed, Sep 13, 2023 at 04:47:54PM +0100, Daniel P. Berrangé wrote:
> On Wed, Sep 06, 2023 at 04:09:17PM +0200, Denis V. Lunev wrote:
> > Each particular testcase could skipped intentionally and accidentally.
> > For example the test is not designed for a particular image format or
> > is not run due to the missed library.
> > 
> > The latter case is unwanted in reality. Though the discussion has
> > revealed that failing the test in such a case would be bad. Thus the
> > patch tries to do different thing. It adds additional status for
> > the test case - 'skipped' and bound intentinal cases to that state.
> 
> I'm not convinced this distinction makes sense and I fear it is
> leading us down the same undesirable route as avocado with too
> many distinct states leading to confusion:
> 
>   https://lore.kernel.org/qemu-devel/Yy1gB1KB3YSIUcoC@redhat.com/
> 
> If I looked at the output I can't tell you the difference between
> "not run" and "skipped" - they both sound the same to me.
> 
> IMHO there's alot to be said for the simplicity of sticking with
> nothing more than PASS/FAIL/SKIP as status names.  The descriptive
> text associated with each SKIP would give more context as to the
> reason in each case if needed.

I guess it boils down to whether there is an actionable response in
that message.  If a test is skipped because it is the wrong format
(for example, ./check -raw skipping a test that only works with
qcow2), there's nothing for me to do.  If a test is skipped because my
setup didn't permit running the test, but where I could enhance my
environment (install more software, pick a different file system,
...), then having the skip message call that out is useful if I want
to take action to get more test coverage.  Even if the message is
present, we have so many tests intentionally skipped that it is hard
to see the few tests where a skip could be turned into a pass by
installing a prerequisite.

> > +++ b/tests/qemu-iotests/testrunner.py
> > @@ -200,6 +200,8 @@ def test_print_one_line(self, test: str,
> >                  col = '\033[1m\033[31m'
> >              elif status == 'not run':
> >                  col = '\033[33m'
> > +            elif status == 'skipped':
> > +                col = '\033[34m'
> >              else:
> >                  col = ''

It looks like for now, the only difference in the two designations is
the output color, and even then, only if you are running the tests in
an environment where color matters (CI systems may not be showing
colors as intended).

I'll hold off queuing the patch while conversation continues.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH 3/3] iotests: distinguish 'skipped' and 'not run' states

[Kevin Wolf]

Am 13.09.2023 um 18:31 hat Eric Blake geschrieben:
> On Wed, Sep 13, 2023 at 04:47:54PM +0100, Daniel P. Berrangé wrote:
> > On Wed, Sep 06, 2023 at 04:09:17PM +0200, Denis V. Lunev wrote:
> > > Each particular testcase could skipped intentionally and accidentally.
> > > For example the test is not designed for a particular image format or
> > > is not run due to the missed library.
> > > 
> > > The latter case is unwanted in reality. Though the discussion has
> > > revealed that failing the test in such a case would be bad. Thus the
> > > patch tries to do different thing. It adds additional status for
> > > the test case - 'skipped' and bound intentinal cases to that state.
> > 
> > I'm not convinced this distinction makes sense and I fear it is
> > leading us down the same undesirable route as avocado with too
> > many distinct states leading to confusion:
> > 
> >   https://lore.kernel.org/qemu-devel/Yy1gB1KB3YSIUcoC@redhat.com/
> > 
> > If I looked at the output I can't tell you the difference between
> > "not run" and "skipped" - they both sound the same to me.
> > 
> > IMHO there's alot to be said for the simplicity of sticking with
> > nothing more than PASS/FAIL/SKIP as status names.  The descriptive
> > text associated with each SKIP would give more context as to the
> > reason in each case if needed.
> 
> I guess it boils down to whether there is an actionable response in
> that message.  If a test is skipped because it is the wrong format
> (for example, ./check -raw skipping a test that only works with
> qcow2), there's nothing for me to do.  If a test is skipped because my
> setup didn't permit running the test, but where I could enhance my
> environment (install more software, pick a different file system,
> ...), then having the skip message call that out is useful if I want
> to take action to get more test coverage.

I'm not sure if there is a clear line to define whether there is an
actionable response or not, because that completely depends on what the
user considers an acceptable response.

You have the relatively clear cases like the test being for another
format, though you could argue that the actionable response is running
./check -qcow2 if raw doesn't work - in fact, why don't we allow ./check
-raw -qcow2 to run both and count it as skipped only if it wasn't run at
all?

The relatively clear case on the other end of the spectrum is if a tool
is missing on the host that you could possibly install. Though maybe
your distro doesn't even package it, so is that still a reasonable
action to take?

What about cases where a block driver isn't compiled in or not
whitelisted? (We've seen this often enough with quorum.) That could be
an accident, but more likely it was a conscious decision and while just
enabling it might fix the test case, enabling additional features in
your product just to make tests happy might not be considered
acceptable. So should this be "not run" or "skipped"? [1]

You could find other unclear cases like tests depending on a different
target architecture, or testing different code paths on different target
architectures.

> Even if the message is present, we have so many tests intentionally
> skipped that it is hard to see the few tests where a skip could be
> turned into a pass by installing a prerequisite.
> 
> > > +++ b/tests/qemu-iotests/testrunner.py
> > > @@ -200,6 +200,8 @@ def test_print_one_line(self, test: str,
> > >                  col = '\033[1m\033[31m'
> > >              elif status == 'not run':
> > >                  col = '\033[33m'
> > > +            elif status == 'skipped':
> > > +                col = '\033[34m'
> > >              else:
> > >                  col = ''
> 
> It looks like for now, the only difference in the two designations is
> the output color, and even then, only if you are running the tests in
> an environment where color matters (CI systems may not be showing
> colors as intended).

Well, and the different status text, obviously.

CI will probably use the tap interface, which is not modified at all by
the patch, so no result would be recorded for "skipped" tests (while
"not run" tests will still return "ok ... # SKIP").

Kevin

[1] If you do answer this rhetorical question, please note that I
    already forgot which is which, so maybe also specify if you mean the
    bad skip or the harmless skip.

Re: [PATCH 3/3] iotests: distinguish 'skipped' and 'not run' states

[Peter Maydell]

On Thu, 14 Sept 2023 at 13:29, Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 13.09.2023 um 18:31 hat Eric Blake geschrieben:
> > I guess it boils down to whether there is an actionable response in
> > that message.  If a test is skipped because it is the wrong format
> > (for example, ./check -raw skipping a test that only works with
> > qcow2), there's nothing for me to do.  If a test is skipped because my
> > setup didn't permit running the test, but where I could enhance my
> > environment (install more software, pick a different file system,
> > ...), then having the skip message call that out is useful if I want
> > to take action to get more test coverage.
>
> I'm not sure if there is a clear line to define whether there is an
> actionable response or not, because that completely depends on what the
> user considers an acceptable response.
>
> You have the relatively clear cases like the test being for another
> format, though you could argue that the actionable response is running
> ./check -qcow2 if raw doesn't work - in fact, why don't we allow ./check
> -raw -qcow2 to run both and count it as skipped only if it wasn't run at
> all?
>
> The relatively clear case on the other end of the spectrum is if a tool
> is missing on the host that you could possibly install. Though maybe
> your distro doesn't even package it, so is that still a reasonable
> action to take?

I think for that sort of thing you need to actually collect the
reasons so that at the end you can say "skipped 21 tests because
frobnicator not present on system; skipped 4 tests because
bazulator not present on system". Then when you're running the
tests you can either (a) install the listed tools or (b) know
you can ignore those specific skips as ones you're aware you
can't run. But that would be a lot of pain and effort to implement...

-- PMM