| Message ID | 20220208124432.84924-1-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Revert "check-block: replace -makecheck with TAP output" | expand |
On Tue, 8 Feb 2022 at 12:44, Paolo Bonzini <pbonzini@redhat.com> wrote: > > This reverts commit d316859f4e28c74ab8b618895d2a5e0a865d3cf1. The > TAP output is inferior in that it does not include the diff for > failed tests. > > Reported-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Thanks; I tested and confirmed that failing iotests now print the diff output again. Tested-by: Peter Maydell <peter.maydell@linaro.org> -- PMM
On 08/02/2022 13.44, Paolo Bonzini wrote: > This reverts commit d316859f4e28c74ab8b618895d2a5e0a865d3cf1. The > TAP output is inferior in that it does not include the diff for > failed tests. What about dumping the output of meson-logs/testlog.txt instead? The diff also shows up there! Thomas
On 2/8/22 15:51, Thomas Huth wrote: > On 08/02/2022 13.44, Paolo Bonzini wrote: >> This reverts commit d316859f4e28c74ab8b618895d2a5e0a865d3cf1. The >> TAP output is inferior in that it does not include the diff for >> failed tests. > > What about dumping the output of meson-logs/testlog.txt instead? The > diff also shows up there! It does, but sort of by accident because it's not valid TAP (and in fact it continues with a TAP parsing error). Paolo
On 08/02/2022 17.21, Paolo Bonzini wrote: > On 2/8/22 15:51, Thomas Huth wrote: >> On 08/02/2022 13.44, Paolo Bonzini wrote: >>> This reverts commit d316859f4e28c74ab8b618895d2a5e0a865d3cf1. The >>> TAP output is inferior in that it does not include the diff for >>> failed tests. >> >> What about dumping the output of meson-logs/testlog.txt instead? The diff >> also shows up there! > > It does, but sort of by accident because it's not valid TAP (and in fact it > continues with a TAP parsing error). Ok, then let's try to go with my series (I'm working on a v2 right now) and the additional patch that prints out the diff to stderr instead of stdout, please ... I hope that should be good enough, and certainly looks way nicer than reverting to the old way of running the iotests. Thomas
On Tue, Feb 08, 2022 at 07:44:32AM -0500, Paolo Bonzini wrote: > This reverts commit d316859f4e28c74ab8b618895d2a5e0a865d3cf1. The > TAP output is inferior in that it does not include the diff for > failed tests. > > Reported-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > tests/check-block.sh | 6 ++-- > tests/qemu-iotests/check | 6 ++-- > tests/qemu-iotests/meson.build | 1 - > tests/qemu-iotests/testenv.py | 30 +++++++++---------- > tests/qemu-iotests/testrunner.py | 49 +++++++++++++++----------------- > 5 files changed, 44 insertions(+), 48 deletions(-) Peter mentioned that he's seeing the CentOS 8 CI job fail continuously: eg https://gitlab.com/qemu-project/qemu/-/jobs/2163919781 [quote] 1/1 qemu:block / qemu-iotests qcow2 ERROR 163.04s 120 subtests passed Summary of Failures: 1/1 qemu:block / qemu-iotests qcow2 ERROR 163.04s 120 subtests passed Ok: 0 Expected Fail: 0 Fail: 1 Unexpected Pass: 0 Skipped: 0 Timeout: 0 Full log written to /home/gitlab-runner/builds/Jpwtyaz7/0/qemu-project/qemu/build/meson-logs/iotestslog.txt make: *** [/home/gitlab-runner/builds/Jpwtyaz7/0/qemu-project/qemu/tests/Makefile.include:156: check-block] Error 1 [/quote] None of the individual tests are showing ERROR though, only the final status is error. We also don't have access to iotestslog.txt which is very annoying. When i run it locally I got the iotests.log and found that it contained an error message saying the TAP outout was malformed and couldn't be parsed. When I ran the iotests manually I confirmed this, it prints: ok qcow2 037 ok qcow2 038 ok qcow2 039 ok qcow2 040 ok qcow2 041 [case not run] test_after_a_quorum_snapshot (__main__.TestRepairQuorum): formats ['quorum'] are not whitelisted [case not run] test_cancel (__main__.TestRepairQuorum): formats ['quorum'] are not whitelisted [case not run] test_cancel_after_ready (__main__.TestRepairQuorum): formats ['quorum'] are not whitelisted [case not run] test_complete (__main__.TestRepairQuorum): formats ['quorum'] are not whitelisted [case not run] test_device_not_found (__main__.TestRepairQuorum): formats ['quorum'] are not whitelisted [case not run] test_image_not_found (__main__.TestRepairQuorum): formats ['quorum'] are not whitelisted [case not run] test_medium_not_found (__main__.TestRepairQuorum): formats ['quorum'] are not whitelisted [case not run] test_no_node_name (__main__.TestRepairQuorum): formats ['quorum'] are not whitelisted [case not run] test_nonexistent_replaces (__main__.TestRepairQuorum): formats ['quorum'] are not whitelisted [case not run] test_pause (__main__.TestRepairQuorum): formats ['quorum'] are not whitelisted [case not run] test_with_other_parent (__main__.TestRepairQuorum): formats ['quorum'] are not whitelisted [case not run] test_with_other_parents_after_mirror_start (__main__.TestRepairQuorum): formats ['quorum'] are not whitelisted [case not run] test_wrong_sync_mode (__main__.TestRepairQuorum): formats ['quorum'] are not whitelisted ok qcow2 042 ok qcow2 043 ok qcow2 046 All those '[case not run]' lines are invalid tap output. The need to start with a leading '#' to be treated as a diagnostic message. I briefly considered sending a patch, until I remember this change to stop using TAP. Anyway I'd say the commit message here needs updating. It is not merely improving diff output. It is adressing the fact that the I/O tests runner is not producing valid TAP output, and so causing bogus failures. Regards, Daniel
On 04/03/2022 17.57, Daniel P. Berrangé wrote: > On Tue, Feb 08, 2022 at 07:44:32AM -0500, Paolo Bonzini wrote: >> This reverts commit d316859f4e28c74ab8b618895d2a5e0a865d3cf1. The >> TAP output is inferior in that it does not include the diff for >> failed tests. >> >> Reported-by: Peter Maydell <peter.maydell@linaro.org> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> --- >> tests/check-block.sh | 6 ++-- >> tests/qemu-iotests/check | 6 ++-- >> tests/qemu-iotests/meson.build | 1 - >> tests/qemu-iotests/testenv.py | 30 +++++++++---------- >> tests/qemu-iotests/testrunner.py | 49 +++++++++++++++----------------- >> 5 files changed, 44 insertions(+), 48 deletions(-) > > Peter mentioned that he's seeing the CentOS 8 CI job fail > continuously: eg > > https://gitlab.com/qemu-project/qemu/-/jobs/2163919781 > > [quote] > 1/1 qemu:block / qemu-iotests qcow2 ERROR 163.04s 120 subtests passed > Summary of Failures: > 1/1 qemu:block / qemu-iotests qcow2 ERROR 163.04s 120 subtests passed > Ok: 0 > Expected Fail: 0 > Fail: 1 > Unexpected Pass: 0 > Skipped: 0 > Timeout: 0 > Full log written to /home/gitlab-runner/builds/Jpwtyaz7/0/qemu-project/qemu/build/meson-logs/iotestslog.txt > make: *** [/home/gitlab-runner/builds/Jpwtyaz7/0/qemu-project/qemu/tests/Makefile.include:156: check-block] Error 1 > > [/quote] > > None of the individual tests are showing ERROR though, only the final > status is error. > > We also don't have access to iotestslog.txt which is very annoying. > > When i run it locally I got the iotests.log and found that it > contained an error message saying the TAP outout was malformed > and couldn't be parsed. > > When I ran the iotests manually I confirmed this, it prints: > > ok qcow2 037 > ok qcow2 038 > ok qcow2 039 > ok qcow2 040 > ok qcow2 041 > [case not run] test_after_a_quorum_snapshot (__main__.TestRepairQuorum): formats ['quorum'] are not whitelisted > [case not run] test_cancel (__main__.TestRepairQuorum): formats ['quorum'] are not whitelisted > [case not run] test_cancel_after_ready (__main__.TestRepairQuorum): formats ['quorum'] are not whitelisted > [case not run] test_complete (__main__.TestRepairQuorum): formats ['quorum'] are not whitelisted > [case not run] test_device_not_found (__main__.TestRepairQuorum): formats ['quorum'] are not whitelisted > [case not run] test_image_not_found (__main__.TestRepairQuorum): formats ['quorum'] are not whitelisted > [case not run] test_medium_not_found (__main__.TestRepairQuorum): formats ['quorum'] are not whitelisted > [case not run] test_no_node_name (__main__.TestRepairQuorum): formats ['quorum'] are not whitelisted > [case not run] test_nonexistent_replaces (__main__.TestRepairQuorum): formats ['quorum'] are not whitelisted > [case not run] test_pause (__main__.TestRepairQuorum): formats ['quorum'] are not whitelisted > [case not run] test_with_other_parent (__main__.TestRepairQuorum): formats ['quorum'] are not whitelisted > [case not run] test_with_other_parents_after_mirror_start (__main__.TestRepairQuorum): formats ['quorum'] are not whitelisted > [case not run] test_wrong_sync_mode (__main__.TestRepairQuorum): formats ['quorum'] are not whitelisted > > ok qcow2 042 > ok qcow2 043 > ok qcow2 046 > > > All those '[case not run]' lines are invalid tap output. The > need to start with a leading '#' to be treated as a diagnostic > message. I briefly considered sending a patch, until I remember > this change to stop using TAP. A patch for fixing the TAP problem is already on the list and has been queued by Hanna: https://lists.gnu.org/archive/html/qemu-devel/2022-02/msg05074.html So that should hopefully fix the problem with the CentOS 8 CI job once it gets merged. Thomas
diff --git a/tests/check-block.sh b/tests/check-block.sh index 720a46bc36..88e02453d2 100755 --- a/tests/check-block.sh +++ b/tests/check-block.sh @@ -14,8 +14,8 @@ else fi skip() { - echo "1..0 #SKIP $*" - exit 0 + echo "$*" + exit 77 } if grep -q "CONFIG_GPROF=y" config-host.mak 2>/dev/null ; then @@ -79,7 +79,7 @@ JOBS=$(echo "$MAKEFLAGS" | sed -n 's/\(^\|.* \)-j\([0-9]\+\)\( .*\|$\)/-j \2/p') ret=0 for fmt in $format_list ; do - ${PYTHON} ./check $JOBS -tap -$fmt $group || ret=1 + ${PYTHON} ./check $JOBS -makecheck -$fmt $group || ret=1 done exit $ret diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check index 75de1b4691..0c27721a41 100755 --- a/tests/qemu-iotests/check +++ b/tests/qemu-iotests/check @@ -32,6 +32,8 @@ def make_argparser() -> argparse.ArgumentParser: p.add_argument('-n', '--dry-run', action='store_true', help='show me, do not run tests') + p.add_argument('-makecheck', action='store_true', + help='pretty print output for make check') p.add_argument('-j', dest='jobs', type=int, default=1, help='run tests in multiple parallel jobs') @@ -51,8 +53,6 @@ def make_argparser() -> argparse.ArgumentParser: p.add_argument('--color', choices=['on', 'off', 'auto'], default='auto', help="use terminal colors. The default " "'auto' value means use colors if terminal stdout detected") - p.add_argument('-tap', action='store_true', - help='produce TAP output') g_env = p.add_argument_group('test environment options') mg = g_env.add_mutually_exclusive_group() @@ -164,7 +164,7 @@ if __name__ == '__main__': if args.dry_run: print('\n'.join(tests)) else: - with TestRunner(env, tap=args.tap, + with TestRunner(env, makecheck=args.makecheck, color=args.color) as tr: paths = [os.path.join(env.source_iotests, t) for t in tests] ok = tr.run_tests(paths, args.jobs) diff --git a/tests/qemu-iotests/meson.build b/tests/qemu-iotests/meson.build index 5be3c74127..3a9425d15c 100644 --- a/tests/qemu-iotests/meson.build +++ b/tests/qemu-iotests/meson.build @@ -22,7 +22,6 @@ if have_tools and targetos != 'windows' endif test('qemu-iotests ' + format, sh, args: [files('../check-block.sh'), format], depends: qemu_iotests_binaries, env: qemu_iotests_env, - protocol: 'tap', suite: suites, timeout: 0, is_parallel: false) diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py index 0f32897fe8..c33454fa68 100644 --- a/tests/qemu-iotests/testenv.py +++ b/tests/qemu-iotests/testenv.py @@ -287,21 +287,21 @@ def __enter__(self) -> 'TestEnv': def __exit__(self, exc_type: Any, exc_value: Any, traceback: Any) -> None: self.close() - def print_env(self, prefix: str = '') -> None: + def print_env(self) -> None: template = """\ -{prefix}QEMU -- "{QEMU_PROG}" {QEMU_OPTIONS} -{prefix}QEMU_IMG -- "{QEMU_IMG_PROG}" {QEMU_IMG_OPTIONS} -{prefix}QEMU_IO -- "{QEMU_IO_PROG}" {QEMU_IO_OPTIONS} -{prefix}QEMU_NBD -- "{QEMU_NBD_PROG}" {QEMU_NBD_OPTIONS} -{prefix}IMGFMT -- {IMGFMT}{imgopts} -{prefix}IMGPROTO -- {IMGPROTO} -{prefix}PLATFORM -- {platform} -{prefix}TEST_DIR -- {TEST_DIR} -{prefix}SOCK_DIR -- {SOCK_DIR} -{prefix}GDB_OPTIONS -- {GDB_OPTIONS} -{prefix}VALGRIND_QEMU -- {VALGRIND_QEMU} -{prefix}PRINT_QEMU_OUTPUT -- {PRINT_QEMU} -{prefix}""" +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 -- {IMGFMT}{imgopts} +IMGPROTO -- {IMGPROTO} +PLATFORM -- {platform} +TEST_DIR -- {TEST_DIR} +SOCK_DIR -- {SOCK_DIR} +GDB_OPTIONS -- {GDB_OPTIONS} +VALGRIND_QEMU -- {VALGRIND_QEMU} +PRINT_QEMU_OUTPUT -- {PRINT_QEMU} +""" args = collections.defaultdict(str, self.get_env()) @@ -310,5 +310,5 @@ def print_env(self, prefix: str = '') -> None: u = os.uname() args['platform'] = f'{u.sysname}/{u.machine} {u.nodename} {u.release}' - args['prefix'] = prefix + print(template.format_map(args)) diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py index 0eace147b8..15788f919e 100644 --- a/tests/qemu-iotests/testrunner.py +++ b/tests/qemu-iotests/testrunner.py @@ -152,10 +152,10 @@ def run_tests_pool(self, tests: List[str], return results - def __init__(self, env: TestEnv, tap: bool = False, + def __init__(self, env: TestEnv, makecheck: bool = False, color: str = 'auto') -> None: self.env = env - self.tap = tap + self.makecheck = makecheck self.last_elapsed = LastElapsedTime('.last-elapsed-cache', env) assert color in ('auto', 'on', 'off') @@ -185,16 +185,13 @@ def test_print_one_line(self, test: str, """ Print short test info before/after test run """ test = os.path.basename(test) - if test_field_width is None: - test_field_width = 8 + if self.makecheck and status != '...': + if status and status != 'pass': + status = f' [{status}]' + else: + status = '' - if self.tap: - if status == 'pass': - print(f'ok {self.env.imgfmt} {test}') - elif status == 'fail': - print(f'not ok {self.env.imgfmt} {test}') - elif status == 'not run': - print(f'ok {self.env.imgfmt} {test} # SKIP') + print(f' TEST iotest-{self.env.imgfmt}: {test}{status}') return if lasttime: @@ -346,7 +343,7 @@ def run_test(self, test: str, last_el = self.last_elapsed.get(test) start = datetime.datetime.now().strftime('%H:%M:%S') - if not self.tap: + if not self.makecheck: self.test_print_one_line(test=test, test_field_width=test_field_width, status = 'started' if mp else '...', @@ -375,9 +372,7 @@ def run_tests(self, tests: List[str], jobs: int = 1) -> bool: notrun = [] casenotrun = [] - if self.tap: - self.env.print_env('# ') - else: + if not self.makecheck: self.env.print_env() test_field_width = max(len(os.path.basename(t)) for t in tests) + 2 @@ -403,6 +398,8 @@ def run_tests(self, tests: List[str], jobs: int = 1) -> bool: if res.status == 'fail': failed.append(name) + if self.makecheck: + self.env.print_env() if res.diff: print('\n'.join(res.diff)) elif res.status == 'not run': @@ -415,16 +412,16 @@ def run_tests(self, tests: List[str], jobs: int = 1) -> bool: if res.interrupted: break - if not self.tap: - if notrun: - print('Not run:', ' '.join(notrun)) + if notrun: + print('Not run:', ' '.join(notrun)) - if casenotrun: - print('Some cases not run in:', ' '.join(casenotrun)) + if casenotrun: + print('Some cases not run in:', ' '.join(casenotrun)) - if failed: - print('Failures:', ' '.join(failed)) - print(f'Failed {len(failed)} of {n_run} iotests') - else: - print(f'Passed all {n_run} iotests') - return not failed + if failed: + print('Failures:', ' '.join(failed)) + print(f'Failed {len(failed)} of {n_run} iotests') + return False + else: + print(f'Passed all {n_run} iotests') + return True
This reverts commit d316859f4e28c74ab8b618895d2a5e0a865d3cf1. The TAP output is inferior in that it does not include the diff for failed tests. Reported-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- tests/check-block.sh | 6 ++-- tests/qemu-iotests/check | 6 ++-- tests/qemu-iotests/meson.build | 1 - tests/qemu-iotests/testenv.py | 30 +++++++++---------- tests/qemu-iotests/testrunner.py | 49 +++++++++++++++----------------- 5 files changed, 44 insertions(+), 48 deletions(-)