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

["Daniel P. Berrangé" <berrange@redhat.com>]

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