| Message ID | 20230906140917.559129-4-den@openvz.org |
|---|---|
| State | New |
| Headers | show |
| Series | separated wanted and unwanted skips in QEMU iotests | expand |
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.
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
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.
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
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.
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.
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
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)
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
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))
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(-)