* [PATCH v3 01/10] iotests.py: Assume a couple of variables as given
2021-01-14 17:02 [PATCH v3 00/10] iotests: Fix 129 and expand 297’s reach Max Reitz
@ 2021-01-14 17:02 ` Max Reitz
2021-01-15 9:19 ` Vladimir Sementsov-Ogievskiy
2021-01-14 17:02 ` [PATCH v3 02/10] iotests/297: Rewrite in Python and extend reach Max Reitz
` (9 subsequent siblings)
10 siblings, 1 reply; 29+ messages in thread
From: Max Reitz @ 2021-01-14 17:02 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz
There are a couple of environment variables that we fetch with
os.environ.get() without supplying a default. Clearly they are required
and expected to be set by the ./check script (as evidenced by
execute_setup_common(), which checks for test_dir and
qemu_default_machine to be set, and aborts if they are not).
Using .get() this way has the disadvantage of returning an Optional[str]
type, which mypy will complain about when tests just assume these values
to be str.
Use [] instead, which raises a KeyError for environment variables that
are not set. When this exception is raised, catch it and move the abort
code from execute_setup_common() there.
Drop the 'assert iotests.sock_dir is not None' from iotest 300, because
that sort of thing is precisely what this patch wants to prevent.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/300 | 1 -
tests/qemu-iotests/iotests.py | 26 +++++++++++++-------------
2 files changed, 13 insertions(+), 14 deletions(-)
diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
index 5b75121b84..b864a21d5e 100755
--- a/tests/qemu-iotests/300
+++ b/tests/qemu-iotests/300
@@ -27,7 +27,6 @@ import qemu
BlockBitmapMapping = List[Dict[str, Union[str, List[Dict[str, str]]]]]
-assert iotests.sock_dir is not None
mig_sock = os.path.join(iotests.sock_dir, 'mig_sock')
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index dcdcd0387f..52facb8e04 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -75,12 +75,20 @@ qemu_opts = os.environ.get('QEMU_OPTIONS', '').strip().split(' ')
imgfmt = os.environ.get('IMGFMT', 'raw')
imgproto = os.environ.get('IMGPROTO', 'file')
-test_dir = os.environ.get('TEST_DIR')
-sock_dir = os.environ.get('SOCK_DIR')
output_dir = os.environ.get('OUTPUT_DIR', '.')
-cachemode = os.environ.get('CACHEMODE')
-aiomode = os.environ.get('AIOMODE')
-qemu_default_machine = os.environ.get('QEMU_DEFAULT_MACHINE')
+
+try:
+ test_dir = os.environ['TEST_DIR']
+ sock_dir = os.environ['SOCK_DIR']
+ cachemode = os.environ['CACHEMODE']
+ aiomode = os.environ['AIOMODE']
+ qemu_default_machine = os.environ['QEMU_DEFAULT_MACHINE']
+except KeyError:
+ # We are using these variables as proxies to indicate that we're
+ # not being run via "check". There may be other things set up by
+ # "check" that individual test cases rely on.
+ sys.stderr.write('Please run this test via the "check" script\n')
+ sys.exit(os.EX_USAGE)
socket_scm_helper = os.environ.get('SOCKET_SCM_HELPER', 'socket_scm_helper')
@@ -1294,14 +1302,6 @@ def execute_setup_common(supported_fmts: Sequence[str] = (),
"""
# Note: Python 3.6 and pylint do not like 'Collection' so use 'Sequence'.
- # We are using TEST_DIR and QEMU_DEFAULT_MACHINE as proxies to
- # indicate that we're not being run via "check". There may be
- # other things set up by "check" that individual test cases rely
- # on.
- if test_dir is None or qemu_default_machine is None:
- sys.stderr.write('Please run this test via the "check" script\n')
- sys.exit(os.EX_USAGE)
-
debug = '-d' in sys.argv
if debug:
sys.argv.remove('-d')
--
2.29.2
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v3 01/10] iotests.py: Assume a couple of variables as given
2021-01-14 17:02 ` [PATCH v3 01/10] iotests.py: Assume a couple of variables as given Max Reitz
@ 2021-01-15 9:19 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 29+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-01-15 9:19 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
14.01.2021 20:02, Max Reitz wrote:
> There are a couple of environment variables that we fetch with
> os.environ.get() without supplying a default. Clearly they are required
> and expected to be set by the ./check script (as evidenced by
> execute_setup_common(), which checks for test_dir and
> qemu_default_machine to be set, and aborts if they are not).
>
> Using .get() this way has the disadvantage of returning an Optional[str]
> type, which mypy will complain about when tests just assume these values
> to be str.
>
> Use [] instead, which raises a KeyError for environment variables that
> are not set. When this exception is raised, catch it and move the abort
> code from execute_setup_common() there.
>
> Drop the 'assert iotests.sock_dir is not None' from iotest 300, because
> that sort of thing is precisely what this patch wants to prevent.
>
> Signed-off-by: Max Reitz<mreitz@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v3 02/10] iotests/297: Rewrite in Python and extend reach
2021-01-14 17:02 [PATCH v3 00/10] iotests: Fix 129 and expand 297’s reach Max Reitz
2021-01-14 17:02 ` [PATCH v3 01/10] iotests.py: Assume a couple of variables as given Max Reitz
@ 2021-01-14 17:02 ` Max Reitz
2021-01-15 11:13 ` Vladimir Sementsov-Ogievskiy
2021-01-14 17:02 ` [PATCH v3 03/10] iotests: Move try_remove to iotests.py Max Reitz
` (8 subsequent siblings)
10 siblings, 1 reply; 29+ messages in thread
From: Max Reitz @ 2021-01-14 17:02 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz
Instead of checking iotests.py only, check all Python files in the
qemu-iotests/ directory. Of course, most of them do not pass, so there
is an extensive skip list for now. (The only files that do pass are
209, 254, 283, and iotests.py.)
(Alternatively, we could have the opposite, i.e. an explicit list of
files that we do want to check, but I think it is better to check files
by default.)
Unless started in debug mode (./check -d), the output has no information
on which files are tested, so we will not have a problem e.g. with
backports, where some files may be missing when compared to upstream.
Besides the technical rewrite, some more things are changed:
- For the pylint invocation, PYTHONPATH is adjusted. This mirrors
setting MYPYPATH for mypy.
- Also, MYPYPATH is now derived from PYTHONPATH, so that we include
paths set by the environment. Maybe at some point we want to let the
check script add '../../python/' to PYTHONPATH so that iotests.py does
not need to do that.
- Passing --notes=FIXME,XXX to pylint suppresses warnings for TODO
comments. TODO is fine, we do not need 297 to complain about such
comments.
- The "Success" line from mypy's output is suppressed, because (A) it
does not add useful information, and (B) it would leak information
about the files having been tested to the reference output, which we
decidedly do not want.
Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/297 | 109 +++++++++++++++++++++++++++++--------
tests/qemu-iotests/297.out | 5 +-
2 files changed, 89 insertions(+), 25 deletions(-)
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 5c5420712b..bfa26d280b 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -1,4 +1,4 @@
-#!/usr/bin/env bash
+#!/usr/bin/env python3
#
# Copyright (C) 2020 Red Hat, Inc.
#
@@ -15,30 +15,95 @@
# You should have received a copy of the GNU General Public License
# along with this program. If not, see <http://www.gnu.org/licenses/>.
-seq=$(basename $0)
-echo "QA output created by $seq"
+import os
+import re
+import shutil
+import subprocess
+import sys
-status=1 # failure is the default!
+import iotests
-# get standard environment
-. ./common.rc
-if ! type -p "pylint-3" > /dev/null; then
- _notrun "pylint-3 not found"
-fi
-if ! type -p "mypy" > /dev/null; then
- _notrun "mypy not found"
-fi
+# TODO: Empty this list!
+SKIP_FILES = (
+ '030', '040', '041', '044', '045', '055', '056', '057', '065', '093',
+ '096', '118', '124', '129', '132', '136', '139', '147', '148', '149',
+ '151', '152', '155', '163', '165', '169', '194', '196', '199', '202',
+ '203', '205', '206', '207', '208', '210', '211', '212', '213', '216',
+ '218', '219', '222', '224', '228', '234', '235', '236', '237', '238',
+ '240', '242', '245', '246', '248', '255', '256', '257', '258', '260',
+ '262', '264', '266', '274', '277', '280', '281', '295', '296', '298',
+ '299', '300', '302', '303', '304', '307',
+ 'nbd-fault-injector.py', 'qcow2.py', 'qcow2_format.py', 'qed.py'
+)
-pylint-3 --score=n iotests.py
-MYPYPATH=../../python/ mypy --warn-unused-configs --disallow-subclassing-any \
- --disallow-any-generics --disallow-incomplete-defs \
- --disallow-untyped-decorators --no-implicit-optional \
- --warn-redundant-casts --warn-unused-ignores \
- --no-implicit-reexport iotests.py
+def is_python_file(filename):
+ if not os.path.isfile(filename):
+ return False
-# success, all done
-echo "*** done"
-rm -f $seq.full
-status=0
+ if filename.endswith('.py'):
+ return True
+
+ with open(filename) as f:
+ try:
+ first_line = f.readline()
+ return re.match('^#!.*python', first_line) is not None
+ except UnicodeDecodeError: # Ignore binary files
+ return False
+
+def run_linters():
+ files = [filename for filename in (set(os.listdir('.')) - set(SKIP_FILES))
+ if is_python_file(filename)]
+
+ iotests.logger.debug('Files to be checked:')
+ iotests.logger.debug(', '.join(sorted(files)))
+
+ print('=== pylint ===')
+ sys.stdout.flush()
+
+ # Todo notes are fine, but fixme's or xxx's should probably just be
+ # fixed (in tests, at least)
+ env = os.environ
+ try:
+ env['PYTHONPATH'] += ':../../python/'
+ except KeyError:
+ env['PYTHONPATH'] = '../../python/'
+ subprocess.run(('pylint-3', '--score=n', '--notes=FIXME,XXX', *files),
+ env=env, check=False)
+
+ print('=== mypy ===')
+ sys.stdout.flush()
+
+ # We have to call mypy separately for each file. Otherwise, it
+ # will interpret all given files as belonging together (i.e., they
+ # may not both define the same classes, etc.; most notably, they
+ # must not both define the __main__ module).
+ env['MYPYPATH'] = env['PYTHONPATH']
+ for filename in files:
+ p = subprocess.run(('mypy',
+ '--warn-unused-configs',
+ '--disallow-subclassing-any',
+ '--disallow-any-generics',
+ '--disallow-incomplete-defs',
+ '--disallow-untyped-decorators',
+ '--no-implicit-optional',
+ '--warn-redundant-casts',
+ '--warn-unused-ignores',
+ '--no-implicit-reexport',
+ filename),
+ env=env,
+ check=False,
+ stdout=subprocess.PIPE,
+ stderr=subprocess.STDOUT,
+ text=True)
+
+ if p.returncode != 0:
+ print(p.stdout)
+
+
+for linter in ('pylint-3', 'mypy'):
+ if shutil.which(linter) is None:
+ iotests.notrun(f'{linter} not found')
+
+iotests.script_main(run_linters)
diff --git a/tests/qemu-iotests/297.out b/tests/qemu-iotests/297.out
index 6acc843649..f2e1314d10 100644
--- a/tests/qemu-iotests/297.out
+++ b/tests/qemu-iotests/297.out
@@ -1,3 +1,2 @@
-QA output created by 297
-Success: no issues found in 1 source file
-*** done
+=== pylint ===
+=== mypy ===
--
2.29.2
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v3 02/10] iotests/297: Rewrite in Python and extend reach
2021-01-14 17:02 ` [PATCH v3 02/10] iotests/297: Rewrite in Python and extend reach Max Reitz
@ 2021-01-15 11:13 ` Vladimir Sementsov-Ogievskiy
2021-01-15 11:57 ` Max Reitz
0 siblings, 1 reply; 29+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-01-15 11:13 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
14.01.2021 20:02, Max Reitz wrote:
> Instead of checking iotests.py only, check all Python files in the
> qemu-iotests/ directory. Of course, most of them do not pass, so there
> is an extensive skip list for now. (The only files that do pass are
> 209, 254, 283, and iotests.py.)
>
> (Alternatively, we could have the opposite, i.e. an explicit list of
> files that we do want to check, but I think it is better to check files
> by default.)
>
> Unless started in debug mode (./check -d), the output has no information
> on which files are tested, so we will not have a problem e.g. with
> backports, where some files may be missing when compared to upstream.
>
> Besides the technical rewrite, some more things are changed:
>
> - For the pylint invocation, PYTHONPATH is adjusted. This mirrors
> setting MYPYPATH for mypy.
>
> - Also, MYPYPATH is now derived from PYTHONPATH, so that we include
> paths set by the environment. Maybe at some point we want to let the
> check script add '../../python/' to PYTHONPATH so that iotests.py does
> not need to do that.
>
> - Passing --notes=FIXME,XXX to pylint suppresses warnings for TODO
> comments. TODO is fine, we do not need 297 to complain about such
> comments.
>
> - The "Success" line from mypy's output is suppressed, because (A) it
> does not add useful information, and (B) it would leak information
> about the files having been tested to the reference output, which we
> decidedly do not want.
>
> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> tests/qemu-iotests/297 | 109 +++++++++++++++++++++++++++++--------
> tests/qemu-iotests/297.out | 5 +-
> 2 files changed, 89 insertions(+), 25 deletions(-)
>
> diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
> index 5c5420712b..bfa26d280b 100755
> --- a/tests/qemu-iotests/297
> +++ b/tests/qemu-iotests/297
> @@ -1,4 +1,4 @@
> -#!/usr/bin/env bash
> +#!/usr/bin/env python3
> #
> # Copyright (C) 2020 Red Hat, Inc.
> #
> @@ -15,30 +15,95 @@
> # You should have received a copy of the GNU General Public License
> # along with this program. If not, see <http://www.gnu.org/licenses/>.
>
> -seq=$(basename $0)
> -echo "QA output created by $seq"
> +import os
> +import re
> +import shutil
> +import subprocess
> +import sys
>
> -status=1 # failure is the default!
> +import iotests
>
> -# get standard environment
> -. ./common.rc
>
> -if ! type -p "pylint-3" > /dev/null; then
> - _notrun "pylint-3 not found"
> -fi
> -if ! type -p "mypy" > /dev/null; then
> - _notrun "mypy not found"
> -fi
> +# TODO: Empty this list!
> +SKIP_FILES = (
> + '030', '040', '041', '044', '045', '055', '056', '057', '065', '093',
> + '096', '118', '124', '129', '132', '136', '139', '147', '148', '149',
> + '151', '152', '155', '163', '165', '169', '194', '196', '199', '202',
> + '203', '205', '206', '207', '208', '210', '211', '212', '213', '216',
> + '218', '219', '222', '224', '228', '234', '235', '236', '237', '238',
> + '240', '242', '245', '246', '248', '255', '256', '257', '258', '260',
> + '262', '264', '266', '274', '277', '280', '281', '295', '296', '298',
> + '299', '300', '302', '303', '304', '307',
> + 'nbd-fault-injector.py', 'qcow2.py', 'qcow2_format.py', 'qed.py'
> +)
>
> -pylint-3 --score=n iotests.py
>
> -MYPYPATH=../../python/ mypy --warn-unused-configs --disallow-subclassing-any \
> - --disallow-any-generics --disallow-incomplete-defs \
> - --disallow-untyped-decorators --no-implicit-optional \
> - --warn-redundant-casts --warn-unused-ignores \
> - --no-implicit-reexport iotests.py
> +def is_python_file(filename):
> + if not os.path.isfile(filename):
> + return False
>
> -# success, all done
> -echo "*** done"
> -rm -f $seq.full
> -status=0
> + if filename.endswith('.py'):
> + return True
> +
> + with open(filename) as f:
> + try:
> + first_line = f.readline()
> + return re.match('^#!.*python', first_line) is not None
> + except UnicodeDecodeError: # Ignore binary files
PEP8 asks for two spaces before inline comment
> + return False
> +
and two empty lines here
(good ALE vim plugin runs flake8, mypy and pylint asynchronously for me and shows these things)
> +def run_linters():
> + files = [filename for filename in (set(os.listdir('.')) - set(SKIP_FILES))
> + if is_python_file(filename)]
> +
> + iotests.logger.debug('Files to be checked:')
> + iotests.logger.debug(', '.join(sorted(files)))
O, good.
> +
> + print('=== pylint ===')
> + sys.stdout.flush()
Should not hurt.. But why? Should be flushed anyway as print will put a '\n'.. And why you care about it at all?
> +
> + # Todo notes are fine, but fixme's or xxx's should probably just be
> + # fixed (in tests, at least)
> + env = os.environ
> + try:
> + env['PYTHONPATH'] += ':../../python/'
> + except KeyError:
> + env['PYTHONPATH'] = '../../python/'
> + subprocess.run(('pylint-3', '--score=n', '--notes=FIXME,XXX', *files),
> + env=env, check=False)
as I understand, you don't need env argument, as os.environ is inherited by default.
> +
> + print('=== mypy ===')
> + sys.stdout.flush()
> +
> + # We have to call mypy separately for each file. Otherwise, it
> + # will interpret all given files as belonging together (i.e., they
> + # may not both define the same classes, etc.; most notably, they
> + # must not both define the __main__ module).
> + env['MYPYPATH'] = env['PYTHONPATH']
> + for filename in files:
> + p = subprocess.run(('mypy',
> + '--warn-unused-configs',
> + '--disallow-subclassing-any',
> + '--disallow-any-generics',
> + '--disallow-incomplete-defs',
> + '--disallow-untyped-decorators',
> + '--no-implicit-optional',
> + '--warn-redundant-casts',
> + '--warn-unused-ignores',
> + '--no-implicit-reexport',
> + filename),
> + env=env,
> + check=False,
> + stdout=subprocess.PIPE,
> + stderr=subprocess.STDOUT,
> + text=True)
Can you really use "text" ? We require python 3.6 (in check), but text comes from 3.7..
It may break some test environments. I think we need old good universal_newlines=True which is the same.
With s/text/universal_newlines/:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> +
> + if p.returncode != 0:
> + print(p.stdout)
> +
> +
> +for linter in ('pylint-3', 'mypy'):
> + if shutil.which(linter) is None:
> + iotests.notrun(f'{linter} not found')
> +
> +iotests.script_main(run_linters)
> diff --git a/tests/qemu-iotests/297.out b/tests/qemu-iotests/297.out
> index 6acc843649..f2e1314d10 100644
> --- a/tests/qemu-iotests/297.out
> +++ b/tests/qemu-iotests/297.out
> @@ -1,3 +1,2 @@
> -QA output created by 297
> -Success: no issues found in 1 source file
> -*** done
> +=== pylint ===
> +=== mypy ===
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v3 02/10] iotests/297: Rewrite in Python and extend reach
2021-01-15 11:13 ` Vladimir Sementsov-Ogievskiy
@ 2021-01-15 11:57 ` Max Reitz
2021-01-15 12:37 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 29+ messages in thread
From: Max Reitz @ 2021-01-15 11:57 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block; +Cc: Kevin Wolf, qemu-devel
On 15.01.21 12:13, Vladimir Sementsov-Ogievskiy wrote:
> 14.01.2021 20:02, Max Reitz wrote:
>> Instead of checking iotests.py only, check all Python files in the
>> qemu-iotests/ directory. Of course, most of them do not pass, so there
>> is an extensive skip list for now. (The only files that do pass are
>> 209, 254, 283, and iotests.py.)
>>
>> (Alternatively, we could have the opposite, i.e. an explicit list of
>> files that we do want to check, but I think it is better to check files
>> by default.)
>>
>> Unless started in debug mode (./check -d), the output has no information
>> on which files are tested, so we will not have a problem e.g. with
>> backports, where some files may be missing when compared to upstream.
>>
>> Besides the technical rewrite, some more things are changed:
>>
>> - For the pylint invocation, PYTHONPATH is adjusted. This mirrors
>> setting MYPYPATH for mypy.
>>
>> - Also, MYPYPATH is now derived from PYTHONPATH, so that we include
>> paths set by the environment. Maybe at some point we want to let the
>> check script add '../../python/' to PYTHONPATH so that iotests.py does
>> not need to do that.
>>
>> - Passing --notes=FIXME,XXX to pylint suppresses warnings for TODO
>> comments. TODO is fine, we do not need 297 to complain about such
>> comments.
>>
>> - The "Success" line from mypy's output is suppressed, because (A) it
>> does not add useful information, and (B) it would leak information
>> about the files having been tested to the reference output, which we
>> decidedly do not want.
>>
>> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> tests/qemu-iotests/297 | 109 +++++++++++++++++++++++++++++--------
>> tests/qemu-iotests/297.out | 5 +-
>> 2 files changed, 89 insertions(+), 25 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
>> index 5c5420712b..bfa26d280b 100755
>> --- a/tests/qemu-iotests/297
>> +++ b/tests/qemu-iotests/297
>> @@ -1,4 +1,4 @@
>> -#!/usr/bin/env bash
>> +#!/usr/bin/env python3
>> #
>> # Copyright (C) 2020 Red Hat, Inc.
>> #
>> @@ -15,30 +15,95 @@
>> # You should have received a copy of the GNU General Public License
>> # along with this program. If not, see <http://www.gnu.org/licenses/>.
>> -seq=$(basename $0)
>> -echo "QA output created by $seq"
>> +import os
>> +import re
>> +import shutil
>> +import subprocess
>> +import sys
>> -status=1 # failure is the default!
>> +import iotests
>> -# get standard environment
>> -. ./common.rc
>> -if ! type -p "pylint-3" > /dev/null; then
>> - _notrun "pylint-3 not found"
>> -fi
>> -if ! type -p "mypy" > /dev/null; then
>> - _notrun "mypy not found"
>> -fi
>> +# TODO: Empty this list!
>> +SKIP_FILES = (
>> + '030', '040', '041', '044', '045', '055', '056', '057', '065',
>> '093',
>> + '096', '118', '124', '129', '132', '136', '139', '147', '148',
>> '149',
>> + '151', '152', '155', '163', '165', '169', '194', '196', '199',
>> '202',
>> + '203', '205', '206', '207', '208', '210', '211', '212', '213',
>> '216',
>> + '218', '219', '222', '224', '228', '234', '235', '236', '237',
>> '238',
>> + '240', '242', '245', '246', '248', '255', '256', '257', '258',
>> '260',
>> + '262', '264', '266', '274', '277', '280', '281', '295', '296',
>> '298',
>> + '299', '300', '302', '303', '304', '307',
>> + 'nbd-fault-injector.py', 'qcow2.py', 'qcow2_format.py', 'qed.py'
>> +)
>> -pylint-3 --score=n iotests.py
>> -MYPYPATH=../../python/ mypy --warn-unused-configs
>> --disallow-subclassing-any \
>> - --disallow-any-generics --disallow-incomplete-defs \
>> - --disallow-untyped-decorators --no-implicit-optional \
>> - --warn-redundant-casts --warn-unused-ignores \
>> - --no-implicit-reexport iotests.py
>> +def is_python_file(filename):
>> + if not os.path.isfile(filename):
>> + return False
>> -# success, all done
>> -echo "*** done"
>> -rm -f $seq.full
>> -status=0
>> + if filename.endswith('.py'):
>> + return True
>> +
>> + with open(filename) as f:
>> + try:
>> + first_line = f.readline()
>> + return re.match('^#!.*python', first_line) is not None
>> + except UnicodeDecodeError: # Ignore binary files
>
> PEP8 asks for two spaces before inline comment
Wow. I really hate PEP8.
>> + return False
>> +
>
> and two empty lines here
>
> (good ALE vim plugin runs flake8, mypy and pylint asynchronously for me
> and shows these things)
I’d like to argue that that isn’t good, but I need to quench my anger.
Perhaps one day I can quench it sufficiently to install ALE myself.
>> +def run_linters():
>> + files = [filename for filename in (set(os.listdir('.')) -
>> set(SKIP_FILES))
>> + if is_python_file(filename)]
>> +
>> + iotests.logger.debug('Files to be checked:')
>> + iotests.logger.debug(', '.join(sorted(files)))
>
> O, good.
>
>> +
>> + print('=== pylint ===')
>> + sys.stdout.flush()
>
> Should not hurt.. But why? Should be flushed anyway as print will put a
> '\n'.. And why you care about it at all?
When pylint fails, I can see its result above the '=== pylint ===' line,
even though its output is on stdout. I suspect the Python output indeed
isn’t flushed on \n.
(Test it by dropping the flush(), and then also e.g. 041 from the ignore
list. Perhaps you get a different result, but for me, the errors appear
above the '=== pylint ===' line.)
>> +
>> + # Todo notes are fine, but fixme's or xxx's should probably just be
>> + # fixed (in tests, at least)
>> + env = os.environ
>> + try:
>> + env['PYTHONPATH'] += ':../../python/'
>> + except KeyError:
>> + env['PYTHONPATH'] = '../../python/'
>> + subprocess.run(('pylint-3', '--score=n', '--notes=FIXME,XXX',
>> *files),
>> + env=env, check=False)
>
> as I understand, you don't need env argument, as os.environ is inherited
> by default.
Yes, but os.environ['PYTHONPATH'] doesn’t include ../../python/, which
is why I copy it.
On second though, I don’t copy it. I think the “env = os.environ” line
should be “env = os.environ.copy()”, actually, or I’ll modify 297’s own
PYTHONPATH.
>> +
>> + print('=== mypy ===')
>> + sys.stdout.flush()
>> +
>> + # We have to call mypy separately for each file. Otherwise, it
>> + # will interpret all given files as belonging together (i.e., they
>> + # may not both define the same classes, etc.; most notably, they
>> + # must not both define the __main__ module).
>> + env['MYPYPATH'] = env['PYTHONPATH']
>> + for filename in files:
>> + p = subprocess.run(('mypy',
>> + '--warn-unused-configs',
>> + '--disallow-subclassing-any',
>> + '--disallow-any-generics',
>> + '--disallow-incomplete-defs',
>> + '--disallow-untyped-decorators',
>> + '--no-implicit-optional',
>> + '--warn-redundant-casts',
>> + '--warn-unused-ignores',
>> + '--no-implicit-reexport',
>> + filename),
>> + env=env,
>> + check=False,
>> + stdout=subprocess.PIPE,
>> + stderr=subprocess.STDOUT,
>> + text=True)
>
> Can you really use "text" ? We require python 3.6 (in check), but text
> comes from 3.7..
Oh well. My fault for just reading the current docs.
> It may break some test environments. I think we need old good
> universal_newlines=True which is the same.
Good enough for me.
> With s/text/universal_newlines/:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Thanks! I’ll clean up the PEP8 violations, make the “env = os.environ”
line an “env = os.environ.copy()”, and use universal_newlines here.
Max
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v3 02/10] iotests/297: Rewrite in Python and extend reach
2021-01-15 11:57 ` Max Reitz
@ 2021-01-15 12:37 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 29+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-01-15 12:37 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
15.01.2021 14:57, Max Reitz wrote:
> On 15.01.21 12:13, Vladimir Sementsov-Ogievskiy wrote:
>> 14.01.2021 20:02, Max Reitz wrote:
>>> Instead of checking iotests.py only, check all Python files in the
>>> qemu-iotests/ directory. Of course, most of them do not pass, so there
>>> is an extensive skip list for now. (The only files that do pass are
>>> 209, 254, 283, and iotests.py.)
>>>
>>> (Alternatively, we could have the opposite, i.e. an explicit list of
>>> files that we do want to check, but I think it is better to check files
>>> by default.)
>>>
>>> Unless started in debug mode (./check -d), the output has no information
>>> on which files are tested, so we will not have a problem e.g. with
>>> backports, where some files may be missing when compared to upstream.
>>>
>>> Besides the technical rewrite, some more things are changed:
>>>
>>> - For the pylint invocation, PYTHONPATH is adjusted. This mirrors
>>> setting MYPYPATH for mypy.
>>>
>>> - Also, MYPYPATH is now derived from PYTHONPATH, so that we include
>>> paths set by the environment. Maybe at some point we want to let the
>>> check script add '../../python/' to PYTHONPATH so that iotests.py does
>>> not need to do that.
>>>
>>> - Passing --notes=FIXME,XXX to pylint suppresses warnings for TODO
>>> comments. TODO is fine, we do not need 297 to complain about such
>>> comments.
>>>
>>> - The "Success" line from mypy's output is suppressed, because (A) it
>>> does not add useful information, and (B) it would leak information
>>> about the files having been tested to the reference output, which we
>>> decidedly do not want.
>>>
>>> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>> ---
>>> tests/qemu-iotests/297 | 109 +++++++++++++++++++++++++++++--------
>>> tests/qemu-iotests/297.out | 5 +-
>>> 2 files changed, 89 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
>>> index 5c5420712b..bfa26d280b 100755
>>> --- a/tests/qemu-iotests/297
>>> +++ b/tests/qemu-iotests/297
>>> @@ -1,4 +1,4 @@
>>> -#!/usr/bin/env bash
>>> +#!/usr/bin/env python3
>>> #
>>> # Copyright (C) 2020 Red Hat, Inc.
>>> #
>>> @@ -15,30 +15,95 @@
>>> # You should have received a copy of the GNU General Public License
>>> # along with this program. If not, see <http://www.gnu.org/licenses/>.
>>> -seq=$(basename $0)
>>> -echo "QA output created by $seq"
>>> +import os
>>> +import re
>>> +import shutil
>>> +import subprocess
>>> +import sys
>>> -status=1 # failure is the default!
>>> +import iotests
>>> -# get standard environment
>>> -. ./common.rc
>>> -if ! type -p "pylint-3" > /dev/null; then
>>> - _notrun "pylint-3 not found"
>>> -fi
>>> -if ! type -p "mypy" > /dev/null; then
>>> - _notrun "mypy not found"
>>> -fi
>>> +# TODO: Empty this list!
>>> +SKIP_FILES = (
>>> + '030', '040', '041', '044', '045', '055', '056', '057', '065', '093',
>>> + '096', '118', '124', '129', '132', '136', '139', '147', '148', '149',
>>> + '151', '152', '155', '163', '165', '169', '194', '196', '199', '202',
>>> + '203', '205', '206', '207', '208', '210', '211', '212', '213', '216',
>>> + '218', '219', '222', '224', '228', '234', '235', '236', '237', '238',
>>> + '240', '242', '245', '246', '248', '255', '256', '257', '258', '260',
>>> + '262', '264', '266', '274', '277', '280', '281', '295', '296', '298',
>>> + '299', '300', '302', '303', '304', '307',
>>> + 'nbd-fault-injector.py', 'qcow2.py', 'qcow2_format.py', 'qed.py'
>>> +)
>>> -pylint-3 --score=n iotests.py
>>> -MYPYPATH=../../python/ mypy --warn-unused-configs --disallow-subclassing-any \
>>> - --disallow-any-generics --disallow-incomplete-defs \
>>> - --disallow-untyped-decorators --no-implicit-optional \
>>> - --warn-redundant-casts --warn-unused-ignores \
>>> - --no-implicit-reexport iotests.py
>>> +def is_python_file(filename):
>>> + if not os.path.isfile(filename):
>>> + return False
>>> -# success, all done
>>> -echo "*** done"
>>> -rm -f $seq.full
>>> -status=0
>>> + if filename.endswith('.py'):
>>> + return True
>>> +
>>> + with open(filename) as f:
>>> + try:
>>> + first_line = f.readline()
>>> + return re.match('^#!.*python', first_line) is not None
>>> + except UnicodeDecodeError: # Ignore binary files
>>
>> PEP8 asks for two spaces before inline comment
>
> Wow. I really hate PEP8.
Wow, it's unexpected :) I like it since first meet..
>
>>> + return False
>>> +
>>
>> and two empty lines here
>>
>> (good ALE vim plugin runs flake8, mypy and pylint asynchronously for me and shows these things)
>
> I’d like to argue that that isn’t good, but I need to quench my anger. Perhaps one day I can quench it sufficiently to install ALE myself.
>
>>> +def run_linters():
>>> + files = [filename for filename in (set(os.listdir('.')) - set(SKIP_FILES))
>>> + if is_python_file(filename)]
>>> +
>>> + iotests.logger.debug('Files to be checked:')
>>> + iotests.logger.debug(', '.join(sorted(files)))
>>
>> O, good.
>>
>>> +
>>> + print('=== pylint ===')
>>> + sys.stdout.flush()
>>
>> Should not hurt.. But why? Should be flushed anyway as print will put a '\n'.. And why you care about it at all?
>
> When pylint fails, I can see its result above the '=== pylint ===' line, even though its output is on stdout. I suspect the Python output indeed isn’t flushed on \n.
>
> (Test it by dropping the flush(), and then also e.g. 041 from the ignore list. Perhaps you get a different result, but for me, the errors appear above the '=== pylint ===' line.)
Checked. Hmm, you are right. Interesting. Probably it explains similar issues with failed iotests output I see sometimes..
>
>>> +
>>> + # Todo notes are fine, but fixme's or xxx's should probably just be
>>> + # fixed (in tests, at least)
>>> + env = os.environ
>>> + try:
>>> + env['PYTHONPATH'] += ':../../python/'
>>> + except KeyError:
>>> + env['PYTHONPATH'] = '../../python/'
>>> + subprocess.run(('pylint-3', '--score=n', '--notes=FIXME,XXX', *files),
>>> + env=env, check=False)
>>
>> as I understand, you don't need env argument, as os.environ is inherited by default.
>
> Yes, but os.environ['PYTHONPATH'] doesn’t include ../../python/, which is why I copy it.
>
> On second though, I don’t copy it. I think the “env = os.environ” line should be “env = os.environ.copy()”, actually, or I’ll modify 297’s own PYTHONPATH.
I think modifying os.environ won't hurt in this case.. But copying is more strict of course.
>
>>> +
>>> + print('=== mypy ===')
>>> + sys.stdout.flush()
>>> +
>>> + # We have to call mypy separately for each file. Otherwise, it
>>> + # will interpret all given files as belonging together (i.e., they
>>> + # may not both define the same classes, etc.; most notably, they
>>> + # must not both define the __main__ module).
>>> + env['MYPYPATH'] = env['PYTHONPATH']
>>> + for filename in files:
>>> + p = subprocess.run(('mypy',
>>> + '--warn-unused-configs',
>>> + '--disallow-subclassing-any',
>>> + '--disallow-any-generics',
>>> + '--disallow-incomplete-defs',
>>> + '--disallow-untyped-decorators',
>>> + '--no-implicit-optional',
>>> + '--warn-redundant-casts',
>>> + '--warn-unused-ignores',
>>> + '--no-implicit-reexport',
>>> + filename),
>>> + env=env,
>>> + check=False,
>>> + stdout=subprocess.PIPE,
>>> + stderr=subprocess.STDOUT,
>>> + text=True)
>>
>> Can you really use "text" ? We require python 3.6 (in check), but text comes from 3.7..
>
> Oh well. My fault for just reading the current docs.
>
>> It may break some test environments. I think we need old good universal_newlines=True which is the same.
>
> Good enough for me.
>
>> With s/text/universal_newlines/:
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> Thanks! I’ll clean up the PEP8 violations, make the “env = os.environ” line an “env = os.environ.copy()”, and use universal_newlines here.
>
OK, thanks, keep my r-b with it.
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v3 03/10] iotests: Move try_remove to iotests.py
2021-01-14 17:02 [PATCH v3 00/10] iotests: Fix 129 and expand 297’s reach Max Reitz
2021-01-14 17:02 ` [PATCH v3 01/10] iotests.py: Assume a couple of variables as given Max Reitz
2021-01-14 17:02 ` [PATCH v3 02/10] iotests/297: Rewrite in Python and extend reach Max Reitz
@ 2021-01-14 17:02 ` Max Reitz
2021-01-15 11:14 ` Vladimir Sementsov-Ogievskiy
2021-01-14 17:02 ` [PATCH v3 04/10] iotests/129: Remove test images in tearDown() Max Reitz
` (7 subsequent siblings)
10 siblings, 1 reply; 29+ messages in thread
From: Max Reitz @ 2021-01-14 17:02 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
tests/qemu-iotests/124 | 8 +-------
tests/qemu-iotests/iotests.py | 11 +++++++----
2 files changed, 8 insertions(+), 11 deletions(-)
diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
index 3705cbb6b3..e40eeb50b9 100755
--- a/tests/qemu-iotests/124
+++ b/tests/qemu-iotests/124
@@ -22,6 +22,7 @@
import os
import iotests
+from iotests import try_remove
def io_write_patterns(img, patterns):
@@ -29,13 +30,6 @@ def io_write_patterns(img, patterns):
iotests.qemu_io('-c', 'write -P%s %s %s' % pattern, img)
-def try_remove(img):
- try:
- os.remove(img)
- except OSError:
- pass
-
-
def transaction_action(action, **kwargs):
return {
'type': action,
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 52facb8e04..a69b4cdc4e 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -523,12 +523,15 @@ class FilePath:
return False
+def try_remove(img):
+ try:
+ os.remove(img)
+ except OSError:
+ pass
+
def file_path_remover():
for path in reversed(file_path_remover.paths):
- try:
- os.remove(path)
- except OSError:
- pass
+ try_remove(path)
def file_path(*names, base_dir=test_dir):
--
2.29.2
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v3 04/10] iotests/129: Remove test images in tearDown()
2021-01-14 17:02 [PATCH v3 00/10] iotests: Fix 129 and expand 297’s reach Max Reitz
` (2 preceding siblings ...)
2021-01-14 17:02 ` [PATCH v3 03/10] iotests: Move try_remove to iotests.py Max Reitz
@ 2021-01-14 17:02 ` Max Reitz
2021-01-15 14:48 ` Willian Rampazzo
2021-01-14 17:02 ` [PATCH v3 05/10] iotests/129: Do not check @busy Max Reitz
` (6 subsequent siblings)
10 siblings, 1 reply; 29+ messages in thread
From: Max Reitz @ 2021-01-14 17:02 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
tests/qemu-iotests/129 | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
index 0e13244d85..2fc65ada6a 100755
--- a/tests/qemu-iotests/129
+++ b/tests/qemu-iotests/129
@@ -47,6 +47,8 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
result = self.vm.qmp("block_set_io_throttle", conv_keys=False,
**params)
self.vm.shutdown()
+ for img in (self.test_img, self.target_img, self.base_img):
+ iotests.try_remove(img)
def do_test_stop(self, cmd, **args):
"""Test 'stop' while block job is running on a throttled drive.
--
2.29.2
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v3 05/10] iotests/129: Do not check @busy
2021-01-14 17:02 [PATCH v3 00/10] iotests: Fix 129 and expand 297’s reach Max Reitz
` (3 preceding siblings ...)
2021-01-14 17:02 ` [PATCH v3 04/10] iotests/129: Remove test images in tearDown() Max Reitz
@ 2021-01-14 17:02 ` Max Reitz
2021-01-14 17:03 ` [PATCH v3 06/10] iotests/129: Use throttle node Max Reitz
` (5 subsequent siblings)
10 siblings, 0 replies; 29+ messages in thread
From: Max Reitz @ 2021-01-14 17:02 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz
@busy is false when the job is paused, which happens all the time
because that is how jobs yield (e.g. for mirror at least since commit
565ac01f8d3).
Back when 129 was added (2015), perhaps there was no better way of
checking whether the job was still actually running. Now we have the
@status field (as of 58b295ba52c, i.e. 2018), which can give us exactly
that information.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
tests/qemu-iotests/129 | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
index 2fc65ada6a..dd23bb2e5a 100755
--- a/tests/qemu-iotests/129
+++ b/tests/qemu-iotests/129
@@ -69,7 +69,7 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
result = self.vm.qmp("stop")
self.assert_qmp(result, 'return', {})
result = self.vm.qmp("query-block-jobs")
- self.assert_qmp(result, 'return[0]/busy', True)
+ self.assert_qmp(result, 'return[0]/status', 'running')
self.assert_qmp(result, 'return[0]/ready', False)
def test_drive_mirror(self):
--
2.29.2
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v3 06/10] iotests/129: Use throttle node
2021-01-14 17:02 [PATCH v3 00/10] iotests: Fix 129 and expand 297’s reach Max Reitz
` (4 preceding siblings ...)
2021-01-14 17:02 ` [PATCH v3 05/10] iotests/129: Do not check @busy Max Reitz
@ 2021-01-14 17:03 ` Max Reitz
2021-01-15 11:16 ` Vladimir Sementsov-Ogievskiy
2021-01-14 17:03 ` [PATCH v3 07/10] iotests/129: Actually test a commit job Max Reitz
` (4 subsequent siblings)
10 siblings, 1 reply; 29+ messages in thread
From: Max Reitz @ 2021-01-14 17:03 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz
Throttling on the BB has not affected block jobs in a while, so it is
possible that one of the jobs in 129 finishes before the VM is stopped.
We can fix that by running the job from a throttle node.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
tests/qemu-iotests/129 | 37 +++++++++++++------------------------
1 file changed, 13 insertions(+), 24 deletions(-)
diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
index dd23bb2e5a..58536bc6ee 100755
--- a/tests/qemu-iotests/129
+++ b/tests/qemu-iotests/129
@@ -32,20 +32,18 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
iotests.qemu_img('create', '-f', iotests.imgfmt, self.test_img,
"-b", self.base_img, '-F', iotests.imgfmt)
iotests.qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x5d 1M 128M', self.test_img)
- self.vm = iotests.VM().add_drive(self.test_img)
+ self.vm = iotests.VM()
+ self.vm.add_object('throttle-group,id=tg0,x-bps-total=1024')
+
+ source_drive = 'driver=throttle,' \
+ 'throttle-group=tg0,' \
+ f'file.driver={iotests.imgfmt},' \
+ f'file.file.filename={self.test_img}'
+
+ self.vm.add_drive(None, source_drive)
self.vm.launch()
def tearDown(self):
- params = {"device": "drive0",
- "bps": 0,
- "bps_rd": 0,
- "bps_wr": 0,
- "iops": 0,
- "iops_rd": 0,
- "iops_wr": 0,
- }
- result = self.vm.qmp("block_set_io_throttle", conv_keys=False,
- **params)
self.vm.shutdown()
for img in (self.test_img, self.target_img, self.base_img):
iotests.try_remove(img)
@@ -53,33 +51,24 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
def do_test_stop(self, cmd, **args):
"""Test 'stop' while block job is running on a throttled drive.
The 'stop' command shouldn't drain the job"""
- params = {"device": "drive0",
- "bps": 1024,
- "bps_rd": 0,
- "bps_wr": 0,
- "iops": 0,
- "iops_rd": 0,
- "iops_wr": 0,
- }
- result = self.vm.qmp("block_set_io_throttle", conv_keys=False,
- **params)
- self.assert_qmp(result, 'return', {})
result = self.vm.qmp(cmd, **args)
self.assert_qmp(result, 'return', {})
+
result = self.vm.qmp("stop")
self.assert_qmp(result, 'return', {})
result = self.vm.qmp("query-block-jobs")
+
self.assert_qmp(result, 'return[0]/status', 'running')
self.assert_qmp(result, 'return[0]/ready', False)
def test_drive_mirror(self):
self.do_test_stop("drive-mirror", device="drive0",
- target=self.target_img,
+ target=self.target_img, format=iotests.imgfmt,
sync="full")
def test_drive_backup(self):
self.do_test_stop("drive-backup", device="drive0",
- target=self.target_img,
+ target=self.target_img, format=iotests.imgfmt,
sync="full")
def test_block_commit(self):
--
2.29.2
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v3 06/10] iotests/129: Use throttle node
2021-01-14 17:03 ` [PATCH v3 06/10] iotests/129: Use throttle node Max Reitz
@ 2021-01-15 11:16 ` Vladimir Sementsov-Ogievskiy
2021-01-15 11:58 ` Max Reitz
0 siblings, 1 reply; 29+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-01-15 11:16 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
14.01.2021 20:03, Max Reitz wrote:
> Throttling on the BB has not affected block jobs in a while, so it is
> possible that one of the jobs in 129 finishes before the VM is stopped.
> We can fix that by running the job from a throttle node.
>
> Signed-off-by: Max Reitz<mreitz@redhat.com>
> Reviewed-by: Eric Blake<eblake@redhat.com>
you forget my :)
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 06/10] iotests/129: Use throttle node
2021-01-15 11:16 ` Vladimir Sementsov-Ogievskiy
@ 2021-01-15 11:58 ` Max Reitz
0 siblings, 0 replies; 29+ messages in thread
From: Max Reitz @ 2021-01-15 11:58 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block; +Cc: Kevin Wolf, qemu-devel
On 15.01.21 12:16, Vladimir Sementsov-Ogievskiy wrote:
> 14.01.2021 20:03, Max Reitz wrote:
>> Throttling on the BB has not affected block jobs in a while, so it is
>> possible that one of the jobs in 129 finishes before the VM is stopped.
>> We can fix that by running the job from a throttle node.
>>
>> Signed-off-by: Max Reitz<mreitz@redhat.com>
>> Reviewed-by: Eric Blake<eblake@redhat.com>
>
> you forget my :)
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Sorry O:)
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v3 07/10] iotests/129: Actually test a commit job
2021-01-14 17:02 [PATCH v3 00/10] iotests: Fix 129 and expand 297’s reach Max Reitz
` (5 preceding siblings ...)
2021-01-14 17:03 ` [PATCH v3 06/10] iotests/129: Use throttle node Max Reitz
@ 2021-01-14 17:03 ` Max Reitz
2021-01-14 17:03 ` [PATCH v3 08/10] iotests/129: Limit mirror job's buffer size Max Reitz
` (3 subsequent siblings)
10 siblings, 0 replies; 29+ messages in thread
From: Max Reitz @ 2021-01-14 17:03 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz
Before this patch, test_block_commit() performs an active commit, which
under the hood is a mirror job. If we want to test various different
block jobs, we should perhaps run an actual commit job instead.
Doing so requires adding an overlay above the source node before the
commit is done (and then specifying the source node as the top node for
the commit job).
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
tests/qemu-iotests/129 | 27 +++++++++++++++++++++++++--
1 file changed, 25 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
index 58536bc6ee..7b4b6649f0 100755
--- a/tests/qemu-iotests/129
+++ b/tests/qemu-iotests/129
@@ -26,6 +26,7 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
test_img = os.path.join(iotests.test_dir, 'test.img')
target_img = os.path.join(iotests.test_dir, 'target.img')
base_img = os.path.join(iotests.test_dir, 'base.img')
+ overlay_img = os.path.join(iotests.test_dir, 'overlay.img')
def setUp(self):
iotests.qemu_img('create', '-f', iotests.imgfmt, self.base_img, "1G")
@@ -36,6 +37,7 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
self.vm.add_object('throttle-group,id=tg0,x-bps-total=1024')
source_drive = 'driver=throttle,' \
+ 'node-name=source,' \
'throttle-group=tg0,' \
f'file.driver={iotests.imgfmt},' \
f'file.file.filename={self.test_img}'
@@ -45,7 +47,8 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
def tearDown(self):
self.vm.shutdown()
- for img in (self.test_img, self.target_img, self.base_img):
+ for img in (self.test_img, self.target_img, self.base_img,
+ self.overlay_img):
iotests.try_remove(img)
def do_test_stop(self, cmd, **args):
@@ -72,7 +75,27 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
sync="full")
def test_block_commit(self):
- self.do_test_stop("block-commit", device="drive0")
+ # Add overlay above the source node so that we actually use a
+ # commit job instead of a mirror job
+
+ iotests.qemu_img('create', '-f', iotests.imgfmt, self.overlay_img,
+ '1G')
+
+ result = self.vm.qmp('blockdev-add', **{
+ 'node-name': 'overlay',
+ 'driver': iotests.imgfmt,
+ 'file': {
+ 'driver': 'file',
+ 'filename': self.overlay_img
+ }
+ })
+ self.assert_qmp(result, 'return', {})
+
+ result = self.vm.qmp('blockdev-snapshot',
+ node='source', overlay='overlay')
+ self.assert_qmp(result, 'return', {})
+
+ self.do_test_stop('block-commit', device='drive0', top_node='source')
if __name__ == '__main__':
iotests.main(supported_fmts=["qcow2"],
--
2.29.2
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v3 08/10] iotests/129: Limit mirror job's buffer size
2021-01-14 17:02 [PATCH v3 00/10] iotests: Fix 129 and expand 297’s reach Max Reitz
` (6 preceding siblings ...)
2021-01-14 17:03 ` [PATCH v3 07/10] iotests/129: Actually test a commit job Max Reitz
@ 2021-01-14 17:03 ` Max Reitz
2021-01-15 14:49 ` Willian Rampazzo
2021-01-14 17:03 ` [PATCH v3 09/10] iotests/129: Clean up pylint and mypy complaints Max Reitz
` (2 subsequent siblings)
10 siblings, 1 reply; 29+ messages in thread
From: Max Reitz @ 2021-01-14 17:03 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz
Issuing 'stop' on the VM drains all nodes. If the mirror job has many
large requests in flight, this may lead to significant I/O that looks a
bit like 'stop' would make the job try to complete (which is what 129
should verify not to happen).
We can limit the I/O in flight by limiting the buffer size, so mirror
will make very little progress during the 'stop' drain.
(We do not need to do anything about commit, which has a buffer size of
512 kB by default; or backup, which goes cluster by cluster. Once we
have asynchronous requests for backup, that will change, but then we can
fine-tune the backup job to only perform a single request on a very
small chunk, too.)
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
tests/qemu-iotests/129 | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
index 7b4b6649f0..6d21470cd7 100755
--- a/tests/qemu-iotests/129
+++ b/tests/qemu-iotests/129
@@ -67,7 +67,7 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
def test_drive_mirror(self):
self.do_test_stop("drive-mirror", device="drive0",
target=self.target_img, format=iotests.imgfmt,
- sync="full")
+ sync="full", buf_size=65536)
def test_drive_backup(self):
self.do_test_stop("drive-backup", device="drive0",
--
2.29.2
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v3 08/10] iotests/129: Limit mirror job's buffer size
2021-01-14 17:03 ` [PATCH v3 08/10] iotests/129: Limit mirror job's buffer size Max Reitz
@ 2021-01-15 14:49 ` Willian Rampazzo
0 siblings, 0 replies; 29+ messages in thread
From: Willian Rampazzo @ 2021-01-15 14:49 UTC (permalink / raw)
To: Max Reitz
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block
On Thu, Jan 14, 2021 at 2:37 PM Max Reitz <mreitz@redhat.com> wrote:
>
> Issuing 'stop' on the VM drains all nodes. If the mirror job has many
> large requests in flight, this may lead to significant I/O that looks a
> bit like 'stop' would make the job try to complete (which is what 129
> should verify not to happen).
>
> We can limit the I/O in flight by limiting the buffer size, so mirror
> will make very little progress during the 'stop' drain.
>
> (We do not need to do anything about commit, which has a buffer size of
> 512 kB by default; or backup, which goes cluster by cluster. Once we
> have asynchronous requests for backup, that will change, but then we can
> fine-tune the backup job to only perform a single request on a very
> small chunk, too.)
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> tests/qemu-iotests/129 | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
Reviewed-by: Willian Rampazzo <willianr@redhat.com>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v3 09/10] iotests/129: Clean up pylint and mypy complaints
2021-01-14 17:02 [PATCH v3 00/10] iotests: Fix 129 and expand 297’s reach Max Reitz
` (7 preceding siblings ...)
2021-01-14 17:03 ` [PATCH v3 08/10] iotests/129: Limit mirror job's buffer size Max Reitz
@ 2021-01-14 17:03 ` Max Reitz
2021-01-14 20:02 ` Willian Rampazzo
2021-01-15 11:18 ` Vladimir Sementsov-Ogievskiy
2021-01-14 17:03 ` [PATCH v3 10/10] iotests/300: " Max Reitz
2021-01-15 11:53 ` [PATCH v3 11/10] iotests: add flake8 linter Vladimir Sementsov-Ogievskiy
10 siblings, 2 replies; 29+ messages in thread
From: Max Reitz @ 2021-01-14 17:03 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/129 | 4 ++--
tests/qemu-iotests/297 | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
index 6d21470cd7..201d9e0a0b 100755
--- a/tests/qemu-iotests/129
+++ b/tests/qemu-iotests/129
@@ -20,7 +20,6 @@
import os
import iotests
-import time
class TestStopWithBlockJob(iotests.QMPTestCase):
test_img = os.path.join(iotests.test_dir, 'test.img')
@@ -32,7 +31,8 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
iotests.qemu_img('create', '-f', iotests.imgfmt, self.base_img, "1G")
iotests.qemu_img('create', '-f', iotests.imgfmt, self.test_img,
"-b", self.base_img, '-F', iotests.imgfmt)
- iotests.qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x5d 1M 128M', self.test_img)
+ iotests.qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x5d 1M 128M',
+ self.test_img)
self.vm = iotests.VM()
self.vm.add_object('throttle-group,id=tg0,x-bps-total=1024')
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index bfa26d280b..1dce1d1b1c 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -27,7 +27,7 @@ import iotests
# TODO: Empty this list!
SKIP_FILES = (
'030', '040', '041', '044', '045', '055', '056', '057', '065', '093',
- '096', '118', '124', '129', '132', '136', '139', '147', '148', '149',
+ '096', '118', '124', '132', '136', '139', '147', '148', '149',
'151', '152', '155', '163', '165', '169', '194', '196', '199', '202',
'203', '205', '206', '207', '208', '210', '211', '212', '213', '216',
'218', '219', '222', '224', '228', '234', '235', '236', '237', '238',
--
2.29.2
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v3 09/10] iotests/129: Clean up pylint and mypy complaints
2021-01-14 17:03 ` [PATCH v3 09/10] iotests/129: Clean up pylint and mypy complaints Max Reitz
@ 2021-01-14 20:02 ` Willian Rampazzo
2021-01-15 9:30 ` Max Reitz
2021-01-15 11:18 ` Vladimir Sementsov-Ogievskiy
1 sibling, 1 reply; 29+ messages in thread
From: Willian Rampazzo @ 2021-01-14 20:02 UTC (permalink / raw)
To: Max Reitz
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block
On Thu, Jan 14, 2021 at 2:41 PM Max Reitz <mreitz@redhat.com> wrote:
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> tests/qemu-iotests/129 | 4 ++--
> tests/qemu-iotests/297 | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
> index 6d21470cd7..201d9e0a0b 100755
> --- a/tests/qemu-iotests/129
> +++ b/tests/qemu-iotests/129
> @@ -20,7 +20,6 @@
>
> import os
> import iotests
> -import time
>
> class TestStopWithBlockJob(iotests.QMPTestCase):
> test_img = os.path.join(iotests.test_dir, 'test.img')
> @@ -32,7 +31,8 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
> iotests.qemu_img('create', '-f', iotests.imgfmt, self.base_img, "1G")
> iotests.qemu_img('create', '-f', iotests.imgfmt, self.test_img,
> "-b", self.base_img, '-F', iotests.imgfmt)
> - iotests.qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x5d 1M 128M', self.test_img)
> + iotests.qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x5d 1M 128M',
> + self.test_img)
> self.vm = iotests.VM()
> self.vm.add_object('throttle-group,id=tg0,x-bps-total=1024')
>
> diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
> index bfa26d280b..1dce1d1b1c 100755
> --- a/tests/qemu-iotests/297
> +++ b/tests/qemu-iotests/297
> @@ -27,7 +27,7 @@ import iotests
> # TODO: Empty this list!
> SKIP_FILES = (
> '030', '040', '041', '044', '045', '055', '056', '057', '065', '093',
> - '096', '118', '124', '129', '132', '136', '139', '147', '148', '149',
> + '096', '118', '124', '132', '136', '139', '147', '148', '149',
Is this also part of mypy/pylint cleanup? It seems you are doing more
than that here. It would be good to have this listed in the commit
message. Despite that,
Reviewed-by: Willian Rampazzo <willianr@redhat.com>
> '151', '152', '155', '163', '165', '169', '194', '196', '199', '202',
> '203', '205', '206', '207', '208', '210', '211', '212', '213', '216',
> '218', '219', '222', '224', '228', '234', '235', '236', '237', '238',
> --
> 2.29.2
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v3 09/10] iotests/129: Clean up pylint and mypy complaints
2021-01-14 20:02 ` Willian Rampazzo
@ 2021-01-15 9:30 ` Max Reitz
2021-01-15 14:43 ` Willian Rampazzo
0 siblings, 1 reply; 29+ messages in thread
From: Max Reitz @ 2021-01-15 9:30 UTC (permalink / raw)
To: Willian Rampazzo
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block
On 14.01.21 21:02, Willian Rampazzo wrote:
> On Thu, Jan 14, 2021 at 2:41 PM Max Reitz <mreitz@redhat.com> wrote:
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> tests/qemu-iotests/129 | 4 ++--
>> tests/qemu-iotests/297 | 2 +-
>> 2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
>> index 6d21470cd7..201d9e0a0b 100755
>> --- a/tests/qemu-iotests/129
>> +++ b/tests/qemu-iotests/129
>> @@ -20,7 +20,6 @@
>>
>> import os
>> import iotests
>> -import time
>>
>> class TestStopWithBlockJob(iotests.QMPTestCase):
>> test_img = os.path.join(iotests.test_dir, 'test.img')
>> @@ -32,7 +31,8 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
>> iotests.qemu_img('create', '-f', iotests.imgfmt, self.base_img, "1G")
>> iotests.qemu_img('create', '-f', iotests.imgfmt, self.test_img,
>> "-b", self.base_img, '-F', iotests.imgfmt)
>> - iotests.qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x5d 1M 128M', self.test_img)
>> + iotests.qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x5d 1M 128M',
>> + self.test_img)
>> self.vm = iotests.VM()
>> self.vm.add_object('throttle-group,id=tg0,x-bps-total=1024')
>>
>> diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
>> index bfa26d280b..1dce1d1b1c 100755
>> --- a/tests/qemu-iotests/297
>> +++ b/tests/qemu-iotests/297
>> @@ -27,7 +27,7 @@ import iotests
>> # TODO: Empty this list!
>> SKIP_FILES = (
>> '030', '040', '041', '044', '045', '055', '056', '057', '065', '093',
>> - '096', '118', '124', '129', '132', '136', '139', '147', '148', '149',
>> + '096', '118', '124', '132', '136', '139', '147', '148', '149',
>
> Is this also part of mypy/pylint cleanup? It seems you are doing more
> than that here. It would be good to have this listed in the commit
> message.
Sure, why not. Something like “And consequentially drop it from 297's
skip list.”?
Though I think making a test pass pylint+mypy complaints basically means
exactly to remove it from 297's skip list and then making 297 pass, so
I’m not entirely sure it’s necessary. But it can’t hurt, so.
> Despite that,
>
> Reviewed-by: Willian Rampazzo <willianr@redhat.com>
Thanks!
Max
>> '151', '152', '155', '163', '165', '169', '194', '196', '199', '202',
>> '203', '205', '206', '207', '208', '210', '211', '212', '213', '216',
>> '218', '219', '222', '224', '228', '234', '235', '236', '237', '238',
>> --
>> 2.29.2
>>
>>
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v3 09/10] iotests/129: Clean up pylint and mypy complaints
2021-01-15 9:30 ` Max Reitz
@ 2021-01-15 14:43 ` Willian Rampazzo
0 siblings, 0 replies; 29+ messages in thread
From: Willian Rampazzo @ 2021-01-15 14:43 UTC (permalink / raw)
To: Max Reitz
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block
On Fri, Jan 15, 2021 at 6:30 AM Max Reitz <mreitz@redhat.com> wrote:
>
> On 14.01.21 21:02, Willian Rampazzo wrote:
> > On Thu, Jan 14, 2021 at 2:41 PM Max Reitz <mreitz@redhat.com> wrote:
> >>
> >> Signed-off-by: Max Reitz <mreitz@redhat.com>
> >> ---
> >> tests/qemu-iotests/129 | 4 ++--
> >> tests/qemu-iotests/297 | 2 +-
> >> 2 files changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
> >> index 6d21470cd7..201d9e0a0b 100755
> >> --- a/tests/qemu-iotests/129
> >> +++ b/tests/qemu-iotests/129
> >> @@ -20,7 +20,6 @@
> >>
> >> import os
> >> import iotests
> >> -import time
> >>
> >> class TestStopWithBlockJob(iotests.QMPTestCase):
> >> test_img = os.path.join(iotests.test_dir, 'test.img')
> >> @@ -32,7 +31,8 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
> >> iotests.qemu_img('create', '-f', iotests.imgfmt, self.base_img, "1G")
> >> iotests.qemu_img('create', '-f', iotests.imgfmt, self.test_img,
> >> "-b", self.base_img, '-F', iotests.imgfmt)
> >> - iotests.qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x5d 1M 128M', self.test_img)
> >> + iotests.qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x5d 1M 128M',
> >> + self.test_img)
> >> self.vm = iotests.VM()
> >> self.vm.add_object('throttle-group,id=tg0,x-bps-total=1024')
> >>
> >> diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
> >> index bfa26d280b..1dce1d1b1c 100755
> >> --- a/tests/qemu-iotests/297
> >> +++ b/tests/qemu-iotests/297
> >> @@ -27,7 +27,7 @@ import iotests
> >> # TODO: Empty this list!
> >> SKIP_FILES = (
> >> '030', '040', '041', '044', '045', '055', '056', '057', '065', '093',
> >> - '096', '118', '124', '129', '132', '136', '139', '147', '148', '149',
> >> + '096', '118', '124', '132', '136', '139', '147', '148', '149',
> >
> > Is this also part of mypy/pylint cleanup? It seems you are doing more
> > than that here. It would be good to have this listed in the commit
> > message.
>
> Sure, why not. Something like “And consequentially drop it from 297's
> skip list.”?
+1 Thanks!
>
> Though I think making a test pass pylint+mypy complaints basically means
> exactly to remove it from 297's skip list and then making 297 pass, so
> I’m not entirely sure it’s necessary. But it can’t hurt, so.
>
> > Despite that,
> >
> > Reviewed-by: Willian Rampazzo <willianr@redhat.com>
>
> Thanks!
>
> Max
>
> >> '151', '152', '155', '163', '165', '169', '194', '196', '199', '202',
> >> '203', '205', '206', '207', '208', '210', '211', '212', '213', '216',
> >> '218', '219', '222', '224', '228', '234', '235', '236', '237', '238',
> >> --
> >> 2.29.2
> >>
> >>
> >
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 09/10] iotests/129: Clean up pylint and mypy complaints
2021-01-14 17:03 ` [PATCH v3 09/10] iotests/129: Clean up pylint and mypy complaints Max Reitz
2021-01-14 20:02 ` Willian Rampazzo
@ 2021-01-15 11:18 ` Vladimir Sementsov-Ogievskiy
1 sibling, 0 replies; 29+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-01-15 11:18 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
14.01.2021 20:03, Max Reitz wrote:
> Signed-off-by: Max Reitz<mreitz@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v3 10/10] iotests/300: Clean up pylint and mypy complaints
2021-01-14 17:02 [PATCH v3 00/10] iotests: Fix 129 and expand 297’s reach Max Reitz
` (8 preceding siblings ...)
2021-01-14 17:03 ` [PATCH v3 09/10] iotests/129: Clean up pylint and mypy complaints Max Reitz
@ 2021-01-14 17:03 ` Max Reitz
2021-01-15 11:30 ` Vladimir Sementsov-Ogievskiy
2021-01-15 11:53 ` [PATCH v3 11/10] iotests: add flake8 linter Vladimir Sementsov-Ogievskiy
10 siblings, 1 reply; 29+ messages in thread
From: Max Reitz @ 2021-01-14 17:03 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/297 | 2 +-
tests/qemu-iotests/300 | 18 +++++++++++++++---
2 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 1dce1d1b1c..03d8604538 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -33,7 +33,7 @@ SKIP_FILES = (
'218', '219', '222', '224', '228', '234', '235', '236', '237', '238',
'240', '242', '245', '246', '248', '255', '256', '257', '258', '260',
'262', '264', '266', '274', '277', '280', '281', '295', '296', '298',
- '299', '300', '302', '303', '304', '307',
+ '299', '302', '303', '304', '307',
'nbd-fault-injector.py', 'qcow2.py', 'qcow2_format.py', 'qed.py'
)
diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
index b864a21d5e..64d388a8fa 100755
--- a/tests/qemu-iotests/300
+++ b/tests/qemu-iotests/300
@@ -22,7 +22,11 @@ import os
import random
import re
from typing import Dict, List, Optional, Union
+
import iotests
+
+# Import qemu after iotests.py has amended the PYTHONPATH
+# pylint: disable=wrong-import-order
import qemu
BlockBitmapMapping = List[Dict[str, Union[str, List[Dict[str, str]]]]]
@@ -110,10 +114,14 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
If @msg is None, check that there has not been any error.
"""
self.vm_b.shutdown()
+
+ log = self.vm_b.get_log()
+ assert log is not None # Loaded after shutdown
+
if msg is None:
- self.assertNotIn('qemu-system-', self.vm_b.get_log())
+ self.assertNotIn('qemu-system-', log)
else:
- self.assertIn(msg, self.vm_b.get_log())
+ self.assertIn(msg, log)
@staticmethod
def mapping(node_name: str, node_alias: str,
@@ -445,9 +453,13 @@ class TestBlockBitmapMappingErrors(TestDirtyBitmapMigration):
# Check for the error in the source's log
self.vm_a.shutdown()
+
+ log = self.vm_a.get_log()
+ assert log is not None # Loaded after shutdown
+
self.assertIn(f"Cannot migrate bitmap '{name}' on node "
f"'{self.src_node_name}': Name is longer than 255 bytes",
- self.vm_a.get_log())
+ log)
# Expect abnormal shutdown of the destination VM because of
# the failed migration
--
2.29.2
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v3 10/10] iotests/300: Clean up pylint and mypy complaints
2021-01-14 17:03 ` [PATCH v3 10/10] iotests/300: " Max Reitz
@ 2021-01-15 11:30 ` Vladimir Sementsov-Ogievskiy
2021-01-15 11:59 ` Max Reitz
0 siblings, 1 reply; 29+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-01-15 11:30 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
14.01.2021 20:03, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> tests/qemu-iotests/297 | 2 +-
> tests/qemu-iotests/300 | 18 +++++++++++++++---
> 2 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
> index 1dce1d1b1c..03d8604538 100755
> --- a/tests/qemu-iotests/297
> +++ b/tests/qemu-iotests/297
> @@ -33,7 +33,7 @@ SKIP_FILES = (
> '218', '219', '222', '224', '228', '234', '235', '236', '237', '238',
> '240', '242', '245', '246', '248', '255', '256', '257', '258', '260',
> '262', '264', '266', '274', '277', '280', '281', '295', '296', '298',
> - '299', '300', '302', '303', '304', '307',
> + '299', '302', '303', '304', '307',
> 'nbd-fault-injector.py', 'qcow2.py', 'qcow2_format.py', 'qed.py'
> )
>
> diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
> index b864a21d5e..64d388a8fa 100755
> --- a/tests/qemu-iotests/300
> +++ b/tests/qemu-iotests/300
> @@ -22,7 +22,11 @@ import os
> import random
> import re
> from typing import Dict, List, Optional, Union
> +
> import iotests
> +
> +# Import qemu after iotests.py has amended the PYTHONPATH
you mean os.path, exactly,
sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
yes?
> +# pylint: disable=wrong-import-order
> import qemu
>
> BlockBitmapMapping = List[Dict[str, Union[str, List[Dict[str, str]]]]]
> @@ -110,10 +114,14 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
> If @msg is None, check that there has not been any error.
> """
> self.vm_b.shutdown()
> +
> + log = self.vm_b.get_log()
> + assert log is not None # Loaded after shutdown
> +
> if msg is None:
> - self.assertNotIn('qemu-system-', self.vm_b.get_log())
> + self.assertNotIn('qemu-system-', log)
> else:
> - self.assertIn(msg, self.vm_b.get_log())
> + self.assertIn(msg, log)
>
> @staticmethod
> def mapping(node_name: str, node_alias: str,
> @@ -445,9 +453,13 @@ class TestBlockBitmapMappingErrors(TestDirtyBitmapMigration):
>
> # Check for the error in the source's log
> self.vm_a.shutdown()
> +
> + log = self.vm_a.get_log()
> + assert log is not None # Loaded after shutdown
> +
> self.assertIn(f"Cannot migrate bitmap '{name}' on node "
> f"'{self.src_node_name}': Name is longer than 255 bytes",
> - self.vm_a.get_log())
> + log)
>
> # Expect abnormal shutdown of the destination VM because of
> # the failed migration
>
Your new comments are the only PEP8 complains in the 300 iotest:
flake8 300
300:119:31: E261 at least two spaces before inline comment
300:458:31: E261 at least two spaces before inline comment
So, I'd fix them.
anyway:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v3 10/10] iotests/300: Clean up pylint and mypy complaints
2021-01-15 11:30 ` Vladimir Sementsov-Ogievskiy
@ 2021-01-15 11:59 ` Max Reitz
0 siblings, 0 replies; 29+ messages in thread
From: Max Reitz @ 2021-01-15 11:59 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block; +Cc: Kevin Wolf, qemu-devel
On 15.01.21 12:30, Vladimir Sementsov-Ogievskiy wrote:
> 14.01.2021 20:03, Max Reitz wrote:
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> tests/qemu-iotests/297 | 2 +-
>> tests/qemu-iotests/300 | 18 +++++++++++++++---
>> 2 files changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
>> index 1dce1d1b1c..03d8604538 100755
>> --- a/tests/qemu-iotests/297
>> +++ b/tests/qemu-iotests/297
>> @@ -33,7 +33,7 @@ SKIP_FILES = (
>> '218', '219', '222', '224', '228', '234', '235', '236', '237',
>> '238',
>> '240', '242', '245', '246', '248', '255', '256', '257', '258',
>> '260',
>> '262', '264', '266', '274', '277', '280', '281', '295', '296',
>> '298',
>> - '299', '300', '302', '303', '304', '307',
>> + '299', '302', '303', '304', '307',
>
>> 'nbd-fault-injector.py', 'qcow2.py', 'qcow2_format.py', 'qed.py'
>> )
>> diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
>> index b864a21d5e..64d388a8fa 100755
>> --- a/tests/qemu-iotests/300
>> +++ b/tests/qemu-iotests/300
>> @@ -22,7 +22,11 @@ import os
>> import random
>> import re
>> from typing import Dict, List, Optional, Union
>> +
>> import iotests
>> +
>> +# Import qemu after iotests.py has amended the PYTHONPATH
>
> you mean os.path, exactly,
>
> sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..',
> 'python'))
>
> yes?
Indeed.
>> +# pylint: disable=wrong-import-order
>> import qemu
>> BlockBitmapMapping = List[Dict[str, Union[str, List[Dict[str, str]]]]]
>> @@ -110,10 +114,14 @@ class
>> TestDirtyBitmapMigration(iotests.QMPTestCase):
>> If @msg is None, check that there has not been any error.
>> """
>> self.vm_b.shutdown()
>> +
>> + log = self.vm_b.get_log()
>> + assert log is not None # Loaded after shutdown
>> +
>> if msg is None:
>> - self.assertNotIn('qemu-system-', self.vm_b.get_log())
>> + self.assertNotIn('qemu-system-', log)
>> else:
>> - self.assertIn(msg, self.vm_b.get_log())
>> + self.assertIn(msg, log)
>> @staticmethod
>> def mapping(node_name: str, node_alias: str,
>> @@ -445,9 +453,13 @@ class
>> TestBlockBitmapMappingErrors(TestDirtyBitmapMigration):
>> # Check for the error in the source's log
>> self.vm_a.shutdown()
>> +
>> + log = self.vm_a.get_log()
>> + assert log is not None # Loaded after shutdown
>> +
>> self.assertIn(f"Cannot migrate bitmap '{name}' on node "
>> f"'{self.src_node_name}': Name is longer than
>> 255 bytes",
>> - self.vm_a.get_log())
>> + log)
>> # Expect abnormal shutdown of the destination VM because of
>> # the failed migration
>>
>
> Your new comments are the only PEP8 complains in the 300 iotest:
>
> flake8 300
> 300:119:31: E261 at least two spaces before inline comment
> 300:458:31: E261 at least two spaces before inline comment
>
> So, I'd fix them.
Haha, sure :)
> anyway:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Thanks!
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v3 11/10] iotests: add flake8 linter
2021-01-14 17:02 [PATCH v3 00/10] iotests: Fix 129 and expand 297’s reach Max Reitz
` (9 preceding siblings ...)
2021-01-14 17:03 ` [PATCH v3 10/10] iotests/300: " Max Reitz
@ 2021-01-15 11:53 ` Vladimir Sementsov-Ogievskiy
2021-01-15 12:03 ` Max Reitz
10 siblings, 1 reply; 29+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-01-15 11:53 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, vsementsov, qemu-devel, mreitz
pylint is good, but doesn't cover the PEP8. Let's add flake8, to be
sure that our code sutisfy PEP8. Add new linter and fix some code style
in checked files.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
Hi!
Here is my small addition to Max's series, hope you like it!
Note, that this is not the first occurrence of 'flake8' in Qemu:
# git grep flake8
python/qemu/.flake8:[flake8]
scripts/qapi/.flake8:[flake8]
scripts/qapi/.flake8:extend-ignore = E722 # Prefer pylint's bare-except checks to flake8's
tests/qemu-iotests/129 | 6 ++-
tests/qemu-iotests/254 | 2 +-
tests/qemu-iotests/297 | 21 ++++++---
tests/qemu-iotests/297.out | 1 +
tests/qemu-iotests/300 | 4 +-
tests/qemu-iotests/iotests.py | 88 +++++++++++++++++++++++++++++++++--
6 files changed, 106 insertions(+), 16 deletions(-)
diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
index 201d9e0a0b..28e6666c1d 100755
--- a/tests/qemu-iotests/129
+++ b/tests/qemu-iotests/129
@@ -21,6 +21,7 @@
import os
import iotests
+
class TestStopWithBlockJob(iotests.QMPTestCase):
test_img = os.path.join(iotests.test_dir, 'test.img')
target_img = os.path.join(iotests.test_dir, 'target.img')
@@ -39,8 +40,8 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
source_drive = 'driver=throttle,' \
'node-name=source,' \
'throttle-group=tg0,' \
- f'file.driver={iotests.imgfmt},' \
- f'file.file.filename={self.test_img}'
+ f'file.driver={iotests.imgfmt},' \
+ f'file.file.filename={self.test_img}'
self.vm.add_drive(None, source_drive)
self.vm.launch()
@@ -97,6 +98,7 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
self.do_test_stop('block-commit', device='drive0', top_node='source')
+
if __name__ == '__main__':
iotests.main(supported_fmts=["qcow2"],
supported_protocols=["file"])
diff --git a/tests/qemu-iotests/254 b/tests/qemu-iotests/254
index 150e58be8e..798f316e36 100755
--- a/tests/qemu-iotests/254
+++ b/tests/qemu-iotests/254
@@ -74,7 +74,7 @@ log("query-block: device = {}, node-name = {}, dirty-bitmaps:".format(
result['device'], result['inserted']['node-name']))
log(result['dirty-bitmaps'], indent=2)
log("\nbitmaps in backing image:")
-log(result['inserted']['image']['backing-image']['format-specific'] \
+log(result['inserted']['image']['backing-image']['format-specific']
['data']['bitmaps'], indent=2)
vm.shutdown()
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 03d8604538..b79c341a3c 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -49,9 +49,10 @@ def is_python_file(filename):
try:
first_line = f.readline()
return re.match('^#!.*python', first_line) is not None
- except UnicodeDecodeError: # Ignore binary files
+ except UnicodeDecodeError: # Ignore binary files
return False
+
def run_linters():
files = [filename for filename in (set(os.listdir('.')) - set(SKIP_FILES))
if is_python_file(filename)]
@@ -59,16 +60,22 @@ def run_linters():
iotests.logger.debug('Files to be checked:')
iotests.logger.debug(', '.join(sorted(files)))
- print('=== pylint ===')
- sys.stdout.flush()
-
- # Todo notes are fine, but fixme's or xxx's should probably just be
- # fixed (in tests, at least)
env = os.environ
try:
env['PYTHONPATH'] += ':../../python/'
except KeyError:
env['PYTHONPATH'] = '../../python/'
+
+ print('=== flake8 ===')
+ sys.stdout.flush()
+
+ subprocess.run(('flake8', *files), env=env, check=False)
+
+ print('=== pylint ===')
+ sys.stdout.flush()
+
+ # Todo notes are fine, but fixme's or xxx's should probably just be
+ # fixed (in tests, at least)
subprocess.run(('pylint-3', '--score=n', '--notes=FIXME,XXX', *files),
env=env, check=False)
@@ -102,7 +109,7 @@ def run_linters():
print(p.stdout)
-for linter in ('pylint-3', 'mypy'):
+for linter in ('flake8', 'pylint-3', 'mypy'):
if shutil.which(linter) is None:
iotests.notrun(f'{linter} not found')
diff --git a/tests/qemu-iotests/297.out b/tests/qemu-iotests/297.out
index f2e1314d10..46bf3e665d 100644
--- a/tests/qemu-iotests/297.out
+++ b/tests/qemu-iotests/297.out
@@ -1,2 +1,3 @@
+=== flake8 ===
=== pylint ===
=== mypy ===
diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
index 64d388a8fa..bbd227ff73 100755
--- a/tests/qemu-iotests/300
+++ b/tests/qemu-iotests/300
@@ -116,7 +116,7 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
self.vm_b.shutdown()
log = self.vm_b.get_log()
- assert log is not None # Loaded after shutdown
+ assert log is not None # Loaded after shutdown
if msg is None:
self.assertNotIn('qemu-system-', log)
@@ -455,7 +455,7 @@ class TestBlockBitmapMappingErrors(TestDirtyBitmapMigration):
self.vm_a.shutdown()
log = self.vm_a.get_log()
- assert log is not None # Loaded after shutdown
+ assert log is not None # Loaded after shutdown
self.assertIn(f"Cannot migrate bitmap '{name}' on node "
f"'{self.src_node_name}': Name is longer than 255 bytes",
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index a69b4cdc4e..e9cf8e6c28 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -37,8 +37,8 @@ from contextlib import contextmanager
# pylint: disable=import-error, wrong-import-position
sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-from qemu import qtest
-from qemu.qmp import QMPMessage
+from qemu import qtest # noqa: E402
+from qemu.qmp import QMPMessage # noqa: E402
# Use this logger for logging messages directly from the iotests module
logger = logging.getLogger('qemu.iotests')
@@ -114,6 +114,7 @@ def qemu_tool_pipe_and_status(tool: str, args: Sequence[str],
' '.join(qemu_img_args + list(args))))
return (output, subp.returncode)
+
def qemu_img_pipe_and_status(*args: str) -> Tuple[str, int]:
"""
Run qemu-img and return both its output and its exit code
@@ -121,10 +122,12 @@ def qemu_img_pipe_and_status(*args: str) -> Tuple[str, int]:
full_args = qemu_img_args + list(args)
return qemu_tool_pipe_and_status('qemu-img', full_args)
+
def qemu_img(*args: str) -> int:
'''Run qemu-img and return the exit code'''
return qemu_img_pipe_and_status(*args)[1]
+
def ordered_qmp(qmsg, conv_keys=True):
# Dictionaries are not ordered prior to 3.6, therefore:
if isinstance(qmsg, list):
@@ -138,6 +141,7 @@ def ordered_qmp(qmsg, conv_keys=True):
return od
return qmsg
+
def qemu_img_create(*args):
args = list(args)
@@ -157,12 +161,15 @@ def qemu_img_create(*args):
return qemu_img(*args)
+
def qemu_img_measure(*args):
return json.loads(qemu_img_pipe("measure", "--output", "json", *args))
+
def qemu_img_check(*args):
return json.loads(qemu_img_pipe("check", "--output", "json", *args))
+
def qemu_img_verbose(*args):
'''Run qemu-img without suppressing its output and return the exit code'''
exitcode = subprocess.call(qemu_img_args + list(args))
@@ -171,15 +178,18 @@ def qemu_img_verbose(*args):
% (-exitcode, ' '.join(qemu_img_args + list(args))))
return exitcode
+
def qemu_img_pipe(*args: str) -> str:
'''Run qemu-img and return its output'''
return qemu_img_pipe_and_status(*args)[0]
+
def qemu_img_log(*args):
result = qemu_img_pipe(*args)
log(result, filters=[filter_testfiles])
return result
+
def img_info_log(filename, filter_path=None, imgopts=False, extra_args=()):
args = ['info']
if imgopts:
@@ -194,6 +204,7 @@ def img_info_log(filename, filter_path=None, imgopts=False, extra_args=()):
filter_path = filename
log(filter_img_info(output, filter_path))
+
def qemu_io(*args):
'''Run qemu-io and return the stdout data'''
args = qemu_io_args + list(args)
@@ -206,11 +217,13 @@ def qemu_io(*args):
% (-subp.returncode, ' '.join(args)))
return output
+
def qemu_io_log(*args):
result = qemu_io(*args)
log(result, filters=[filter_testfiles, filter_qemu_io])
return result
+
def qemu_io_silent(*args):
'''Run qemu-io and return the exit code, suppressing stdout'''
if '-f' in args or '--image-opts' in args:
@@ -225,6 +238,7 @@ def qemu_io_silent(*args):
(-exitcode, ' '.join(args)))
return exitcode
+
def qemu_io_silent_check(*args):
'''Run qemu-io and return the true if subprocess returned 0'''
args = qemu_io_args + list(args)
@@ -232,11 +246,13 @@ def qemu_io_silent_check(*args):
stderr=subprocess.STDOUT)
return exitcode == 0
+
def get_virtio_scsi_device():
if qemu_default_machine == 's390-ccw-virtio':
return 'virtio-scsi-ccw'
return 'virtio-scsi-pci'
+
class QemuIoInteractive:
def __init__(self, *args):
self.args = qemu_io_args_no_fmt + list(args)
@@ -286,6 +302,7 @@ def qemu_nbd(*args):
'''Run qemu-nbd in daemon mode and return the parent's exit code'''
return subprocess.call(qemu_nbd_args + ['--fork'] + list(args))
+
def qemu_nbd_early_pipe(*args: str) -> Tuple[int, str]:
'''Run qemu-nbd in daemon mode and return both the parent's exit code
and its output in case of an error'''
@@ -294,6 +311,7 @@ def qemu_nbd_early_pipe(*args: str) -> Tuple[int, str]:
connect_stderr=False)
return returncode, output if returncode else ''
+
def qemu_nbd_list_log(*args: str) -> str:
'''Run qemu-nbd to list remote exports'''
full_args = [qemu_nbd_prog, '-L'] + list(args)
@@ -301,6 +319,7 @@ def qemu_nbd_list_log(*args: str) -> str:
log(output, filters=[filter_testfiles, filter_nbd_exports])
return output
+
@contextmanager
def qemu_nbd_popen(*args):
'''Context manager running qemu-nbd within the context'''
@@ -326,11 +345,13 @@ def qemu_nbd_popen(*args):
p.kill()
p.wait()
+
def compare_images(img1, img2, fmt1=imgfmt, fmt2=imgfmt):
'''Return True if two image files are identical'''
return qemu_img('compare', '-f', fmt1,
'-F', fmt2, img1, img2) == 0
+
def create_image(name, size):
'''Create a fully-allocated raw image with sector markers'''
file = open(name, 'wb')
@@ -341,34 +362,49 @@ def create_image(name, size):
i = i + 512
file.close()
+
def image_size(img):
'''Return image's virtual size'''
r = qemu_img_pipe('info', '--output=json', '-f', imgfmt, img)
return json.loads(r)['virtual-size']
+
def is_str(val):
return isinstance(val, str)
+
test_dir_re = re.compile(r"%s" % test_dir)
+
+
def filter_test_dir(msg):
return test_dir_re.sub("TEST_DIR", msg)
+
win32_re = re.compile(r"\r")
+
+
def filter_win32(msg):
return win32_re.sub("", msg)
+
qemu_io_re = re.compile(r"[0-9]* ops; [0-9\/:. sec]* "
r"\([0-9\/.inf]* [EPTGMKiBbytes]*\/sec "
r"and [0-9\/.inf]* ops\/sec\)")
+
+
def filter_qemu_io(msg):
msg = filter_win32(msg)
return qemu_io_re.sub("X ops; XX:XX:XX.X "
"(XXX YYY/sec and XXX ops/sec)", msg)
+
chown_re = re.compile(r"chown [0-9]+:[0-9]+")
+
+
def filter_chown(msg):
return chown_re.sub("chown UID:GID", msg)
+
def filter_qmp_event(event):
'''Filter a QMP event dict'''
event = dict(event)
@@ -377,6 +413,7 @@ def filter_qmp_event(event):
event['timestamp']['microseconds'] = 'USECS'
return event
+
def filter_qmp(qmsg, filter_fn):
'''Given a string filter, filter a QMP object's values.
filter_fn takes a (key, value) pair.'''
@@ -393,11 +430,13 @@ def filter_qmp(qmsg, filter_fn):
qmsg[k] = filter_fn(k, v)
return qmsg
+
def filter_testfiles(msg):
pref1 = os.path.join(test_dir, "%s-" % (os.getpid()))
pref2 = os.path.join(sock_dir, "%s-" % (os.getpid()))
return msg.replace(pref1, 'TEST_DIR/PID-').replace(pref2, 'SOCK_DIR/PID-')
+
def filter_qmp_testfiles(qmsg):
def _filter(_key, value):
if is_str(value):
@@ -405,9 +444,11 @@ def filter_qmp_testfiles(qmsg):
return value
return filter_qmp(qmsg, _filter)
+
def filter_virtio_scsi(output: str) -> str:
return re.sub(r'(virtio-scsi)-(ccw|pci)', r'\1', output)
+
def filter_qmp_virtio_scsi(qmsg):
def _filter(_key, value):
if is_str(value):
@@ -415,9 +456,11 @@ def filter_qmp_virtio_scsi(qmsg):
return value
return filter_qmp(qmsg, _filter)
+
def filter_generated_node_ids(msg):
return re.sub("#block[0-9]+", "NODE_NAME", msg)
+
def filter_img_info(output, filename):
lines = []
for line in output.split('\n'):
@@ -434,9 +477,11 @@ def filter_img_info(output, filename):
lines.append(line)
return '\n'.join(lines)
+
def filter_imgfmt(msg):
return msg.replace(imgfmt, 'IMGFMT')
+
def filter_qmp_imgfmt(qmsg):
def _filter(_key, value):
if is_str(value):
@@ -444,12 +489,14 @@ def filter_qmp_imgfmt(qmsg):
return value
return filter_qmp(qmsg, _filter)
+
def filter_nbd_exports(output: str) -> str:
return re.sub(r'((min|opt|max) block): [0-9]+', r'\1: XXX', output)
Msg = TypeVar('Msg', Dict[str, Any], List[Any], str)
+
def log(msg: Msg,
filters: Iterable[Callable[[Msg], Msg]] = (),
indent: Optional[int] = None) -> None:
@@ -466,23 +513,29 @@ def log(msg: Msg,
else:
test_logger.info(msg)
+
class Timeout:
def __init__(self, seconds, errmsg="Timeout"):
self.seconds = seconds
self.errmsg = errmsg
+
def __enter__(self):
signal.signal(signal.SIGALRM, self.timeout)
signal.setitimer(signal.ITIMER_REAL, self.seconds)
return self
+
def __exit__(self, exc_type, value, traceback):
signal.setitimer(signal.ITIMER_REAL, 0)
return False
+
def timeout(self, signum, frame):
raise Exception(self.errmsg)
+
def file_pattern(name):
return "{0}-{1}".format(os.getpid(), name)
+
class FilePath:
"""
Context manager generating multiple file names. The generated files are
@@ -529,6 +582,7 @@ def try_remove(img):
except OSError:
pass
+
def file_path_remover():
for path in reversed(file_path_remover.paths):
try_remove(path)
@@ -556,6 +610,7 @@ def file_path(*names, base_dir=test_dir):
return paths[0] if len(paths) == 1 else paths
+
def remote_filename(path):
if imgproto == 'file':
return path
@@ -564,6 +619,7 @@ def remote_filename(path):
else:
raise Exception("Protocol %s not supported" % (imgproto))
+
class VM(qtest.QEMUQtestMachine):
'''A QEMU VM'''
@@ -664,7 +720,7 @@ class VM(qtest.QEMUQtestMachine):
for key in obj:
self.flatten_qmp_object(obj[key], output, basestr + key + '.')
else:
- output[basestr[:-1]] = obj # Strip trailing '.'
+ output[basestr[:-1]] = obj # Strip trailing '.'
return output
def qmp_to_opts(self, obj):
@@ -884,8 +940,10 @@ class VM(qtest.QEMUQtestMachine):
'Found node %s under %s (but expected %s)' % \
(node['name'], path, expected_node)
+
index_re = re.compile(r'([^\[]+)\[([^\]]+)\]')
+
class QMPTestCase(unittest.TestCase):
'''Abstract base class for QMP test cases'''
@@ -992,7 +1050,6 @@ class QMPTestCase(unittest.TestCase):
elif event['event'] == 'JOB_STATUS_CHANGE':
self.assert_qmp(event, 'data/id', drive)
-
self.assert_no_active_block_jobs()
return result
@@ -1078,6 +1135,7 @@ def notrun(reason):
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
@@ -1090,6 +1148,7 @@ def case_notrun(reason):
open('%s/%s.casenotrun' % (output_dir, seq), 'a').write(
' [case not run] ' + reason + '\n')
+
def _verify_image_format(supported_fmts: Sequence[str] = (),
unsupported_fmts: Sequence[str] = ()) -> None:
if 'generic' in supported_fmts and \
@@ -1106,6 +1165,7 @@ def _verify_image_format(supported_fmts: Sequence[str] = (),
if imgfmt == 'luks':
verify_working_luks()
+
def _verify_protocol(supported: Sequence[str] = (),
unsupported: Sequence[str] = ()) -> None:
assert not (supported and unsupported)
@@ -1117,6 +1177,7 @@ def _verify_protocol(supported: Sequence[str] = (),
if not_sup or (imgproto in unsupported):
notrun('not suitable for this protocol: %s' % imgproto)
+
def _verify_platform(supported: Sequence[str] = (),
unsupported: Sequence[str] = ()) -> None:
if any((sys.platform.startswith(x) for x in unsupported)):
@@ -1126,27 +1187,33 @@ def _verify_platform(supported: Sequence[str] = (),
if not any((sys.platform.startswith(x) for x in supported)):
notrun('not suitable for this OS: %s' % sys.platform)
+
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)
+
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)
+
def _verify_formats(required_formats: Sequence[str] = ()) -> None:
usf_list = list(set(required_formats) - set(supported_formats()))
if usf_list:
notrun(f'formats {usf_list} are not whitelisted')
+
def supports_quorum():
return 'quorum' in qemu_img_pipe('--help')
+
def verify_quorum():
'''Skip test suite if quorum support is not available'''
if not supports_quorum():
notrun('quorum support missing')
+
def has_working_luks() -> Tuple[bool, str]:
"""
Check whether our LUKS driver can actually create images
@@ -1178,6 +1245,7 @@ def has_working_luks() -> Tuple[bool, str]:
else:
return (True, '')
+
def verify_working_luks():
"""
Skip test suite if LUKS does not work
@@ -1186,6 +1254,7 @@ def verify_working_luks():
if not working:
notrun(reason)
+
def qemu_pipe(*args: str) -> str:
"""
Run qemu with an option to print something and exit (e.g. a help option).
@@ -1196,6 +1265,7 @@ def qemu_pipe(*args: str) -> str:
output, _ = qemu_tool_pipe_and_status('qemu', full_args)
return output
+
def supported_formats(read_only=False):
'''Set 'read_only' to True to check ro-whitelist
Otherwise, rw-whitelist is checked'''
@@ -1211,6 +1281,7 @@ def supported_formats(read_only=False):
return supported_formats.formats[read_only]
+
def skip_if_unsupported(required_formats=(), read_only=False):
'''Skip Test Decorator
Runs the test if all the required formats are whitelisted'''
@@ -1231,6 +1302,7 @@ def skip_if_unsupported(required_formats=(), read_only=False):
return func_wrapper
return skip_test_decorator
+
def skip_for_formats(formats: Sequence[str] = ()) \
-> Callable[[Callable[[QMPTestCase, List[Any], Dict[str, Any]], None]],
Callable[[QMPTestCase, List[Any], Dict[str, Any]], None]]:
@@ -1247,6 +1319,7 @@ def skip_for_formats(formats: Sequence[str] = ()) \
return func_wrapper
return skip_test_decorator
+
def skip_if_user_is_root(func):
'''Skip Test Decorator
Runs the test only without root permissions'''
@@ -1258,6 +1331,7 @@ def skip_if_user_is_root(func):
return func(*args, **kwargs)
return func_wrapper
+
def execute_unittest(debug=False):
"""Executes unittests within the calling module."""
@@ -1290,6 +1364,7 @@ def execute_unittest(debug=False):
sys.stderr.write(out)
+
def execute_setup_common(supported_fmts: Sequence[str] = (),
supported_platforms: Sequence[str] = (),
supported_cache_modes: Sequence[str] = (),
@@ -1319,6 +1394,7 @@ def execute_setup_common(supported_fmts: Sequence[str] = (),
return debug
+
def execute_test(*args, test_function=None, **kwargs):
"""Run either unittest or script-style tests."""
@@ -1328,6 +1404,7 @@ def execute_test(*args, test_function=None, **kwargs):
else:
test_function()
+
def activate_logging():
"""Activate iotests.log() output to stdout for script-style tests."""
handler = logging.StreamHandler(stream=sys.stdout)
@@ -1337,18 +1414,21 @@ def activate_logging():
test_logger.setLevel(logging.INFO)
test_logger.propagate = False
+
# This is called from script-style iotests without a single point of entry
def script_initialize(*args, **kwargs):
"""Initialize script-style tests without running any tests."""
activate_logging()
execute_setup_common(*args, **kwargs)
+
# This is called from script-style iotests with a single point of entry
def script_main(test_function, *args, **kwargs):
"""Run script-style tests outside of the unittest framework"""
activate_logging()
execute_test(*args, test_function=test_function, **kwargs)
+
# This is called from unittest style iotests
def main(*args, **kwargs):
"""Run tests using the unittest framework"""
--
2.29.2
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v3 11/10] iotests: add flake8 linter
2021-01-15 11:53 ` [PATCH v3 11/10] iotests: add flake8 linter Vladimir Sementsov-Ogievskiy
@ 2021-01-15 12:03 ` Max Reitz
2021-01-15 13:30 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 29+ messages in thread
From: Max Reitz @ 2021-01-15 12:03 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block; +Cc: kwolf, qemu-devel
On 15.01.21 12:53, Vladimir Sementsov-Ogievskiy wrote:
> pylint is good, but doesn't cover the PEP8. Let's add flake8, to be
> sure that our code sutisfy PEP8. Add new linter and fix some code style
> in checked files.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>
> Hi!
>
> Here is my small addition to Max's series, hope you like it!
>
> Note, that this is not the first occurrence of 'flake8' in Qemu:
>
> # git grep flake8
> python/qemu/.flake8:[flake8]
> scripts/qapi/.flake8:[flake8]
> scripts/qapi/.flake8:extend-ignore = E722 # Prefer pylint's bare-except checks to flake8's
>
>
> tests/qemu-iotests/129 | 6 ++-
> tests/qemu-iotests/254 | 2 +-
> tests/qemu-iotests/297 | 21 ++++++---
> tests/qemu-iotests/297.out | 1 +
> tests/qemu-iotests/300 | 4 +-
> tests/qemu-iotests/iotests.py | 88 +++++++++++++++++++++++++++++++++--
> 6 files changed, 106 insertions(+), 16 deletions(-)
Looks reasonable to me, but perhaps it should just be a dedicated
series. I think there’s enough in here to justify that.
> diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
> index 201d9e0a0b..28e6666c1d 100755
> --- a/tests/qemu-iotests/129
> +++ b/tests/qemu-iotests/129
> @@ -21,6 +21,7 @@
> import os
> import iotests
>
> +
> class TestStopWithBlockJob(iotests.QMPTestCase):
> test_img = os.path.join(iotests.test_dir, 'test.img')
> target_img = os.path.join(iotests.test_dir, 'target.img')
> @@ -39,8 +40,8 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
> source_drive = 'driver=throttle,' \
> 'node-name=source,' \
> 'throttle-group=tg0,' \
> - f'file.driver={iotests.imgfmt},' \
> - f'file.file.filename={self.test_img}'
> + f'file.driver={iotests.imgfmt},' \
> + f'file.file.filename={self.test_img}'
Interesting, when indenting this, I was wondering whether pylint would
complain. I was so glad it didn’t. I really don’t like PEP8.
(Though I understand that style guides like PEP8 are there specifically
so when someone like me goes “but I like this style better :(”, everyone
else can say “but you’re objectively wrong”. So me hating it kind of is
its point, I guess.)
Max
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v3 11/10] iotests: add flake8 linter
2021-01-15 12:03 ` Max Reitz
@ 2021-01-15 13:30 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 29+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-01-15 13:30 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: kwolf, qemu-devel
15.01.2021 15:03, Max Reitz wrote:
> On 15.01.21 12:53, Vladimir Sementsov-Ogievskiy wrote:
>> pylint is good, but doesn't cover the PEP8. Let's add flake8, to be
>> sure that our code sutisfy PEP8. Add new linter and fix some code style
>> in checked files.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>
>> Hi!
>>
>> Here is my small addition to Max's series, hope you like it!
>>
>> Note, that this is not the first occurrence of 'flake8' in Qemu:
>>
>> # git grep flake8
>> python/qemu/.flake8:[flake8]
>> scripts/qapi/.flake8:[flake8]
>> scripts/qapi/.flake8:extend-ignore = E722 # Prefer pylint's bare-except checks to flake8's
>>
>>
>> tests/qemu-iotests/129 | 6 ++-
>> tests/qemu-iotests/254 | 2 +-
>> tests/qemu-iotests/297 | 21 ++++++---
>> tests/qemu-iotests/297.out | 1 +
>> tests/qemu-iotests/300 | 4 +-
>> tests/qemu-iotests/iotests.py | 88 +++++++++++++++++++++++++++++++++--
>> 6 files changed, 106 insertions(+), 16 deletions(-)
>
> Looks reasonable to me, but perhaps it should just be a dedicated series. I think there’s enough in here to justify that.
Not a problem, I can resend :)
>
>> diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
>> index 201d9e0a0b..28e6666c1d 100755
>> --- a/tests/qemu-iotests/129
>> +++ b/tests/qemu-iotests/129
>> @@ -21,6 +21,7 @@
>> import os
>> import iotests
>> +
>> class TestStopWithBlockJob(iotests.QMPTestCase):
>> test_img = os.path.join(iotests.test_dir, 'test.img')
>> target_img = os.path.join(iotests.test_dir, 'target.img')
>> @@ -39,8 +40,8 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
>> source_drive = 'driver=throttle,' \
>> 'node-name=source,' \
>> 'throttle-group=tg0,' \
>> - f'file.driver={iotests.imgfmt},' \
>> - f'file.file.filename={self.test_img}'
>> + f'file.driver={iotests.imgfmt},' \
>> + f'file.file.filename={self.test_img}'
>
> Interesting, when indenting this, I was wondering whether pylint would complain. I was so glad it didn’t. I really don’t like PEP8.
>
> (Though I understand that style guides like PEP8 are there specifically so when someone like me goes “but I like this style better :(”, everyone else can say “but you’re objectively wrong”. So me hating it kind of is its point, I guess.)
>
When I noted how you indent 'string' with f'string', I thought "o, interesting idea".. I just very like the idea of standardized code-style for the language, so I just used to follow PEP8 since first time I learned about it.
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 29+ messages in thread