| Message ID | 20210113175752.403022-2-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | iotests: Fix 129 and expand 297’s reach | expand |
On 1/13/21 11:57 AM, Max Reitz wrote: > I.e., all Python files in the qemu-iotests/ directory. > > Most files of course 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.) Concur with the choice for default. > > I decided to include the list of files checked in the reference output, > so we do not accidentally lose coverage of anything. That means adding > new Python tests will require a change to 297.out, but that should not > be a problem. I'm okay with that. > > On the other hand, I decided to hide mypy's "Success" lines from the > reference output, because they do not add anything useful. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > tests/qemu-iotests/297 | 66 ++++++++++++++++++++++++++++++++++---- > tests/qemu-iotests/297.out | 6 +++- > 2 files changed, 65 insertions(+), 7 deletions(-) > > diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297 > index 5c5420712b..b1a7d6d5e8 100755 > --- a/tests/qemu-iotests/297 > +++ b/tests/qemu-iotests/297 > @@ -30,13 +30,67 @@ if ! type -p "mypy" > /dev/null; then > _notrun "mypy not found" > fi > > -pylint-3 --score=n iotests.py > +# TODO: Empty this list! :) > +file_list=() > +for file in *; do > + # Check files with a .py extension or a Python shebang > + # (Unless they are in the skip_files list) > + if [ -f "$file" ] && ((echo "$file" | grep -q '\.py$') || > + (head -n 1 "$file" | grep -q '^#!.*python')) Bash has an (obsolete) operator (()) (behaves like a mix of $(()) and 'if'); when nesting subshells, POSIX recommends inserting a space to avoid inadvertent triggering of the alternate semantics of the operator. But why do you need nested subshells? This is equivalent: if [ -f "$file" ] && (echo "$file" | grep -q '\.py$' || head -n 1 "$file" | grep -q '^#!.*python') > + then > + skip_file=false > + for skip in "${skip_files[@]}"; do bashism, but iotests require bash, so fine. > + if [ "$skip" = "$file" ]; then > + skip_file=true > + break > + fi > + done > + > + if ! $skip_file; then > + file_list+=("$file") Likewise. Whether or not you strip the extra (), Reviewed-by: Eric Blake <eblake@redhat.com>
13.01.2021 20:57, Max Reitz wrote: > I.e., all Python files in the qemu-iotests/ directory. > > Most files of course 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.) > > I decided to include the list of files checked in the reference output, > so we do not accidentally lose coverage of anything. That means adding > new Python tests will require a change to 297.out, but that should not > be a problem. I have a parallel series, "Rework iotests/check", one of its aims is drop group file, to avoid these endless conflicts in group file when you want to send series or when you are porting patches to/from downstream. And you are trying to add one another "group" file :) I don't like the idea. Why should we loose accidentally the coverage? Logic is extremely simple: all files except for the list. > > On the other hand, I decided to hide mypy's "Success" lines from the > reference output, because they do not add anything useful. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > tests/qemu-iotests/297 | 66 ++++++++++++++++++++++++++++++++++---- > tests/qemu-iotests/297.out | 6 +++- > 2 files changed, 65 insertions(+), 7 deletions(-) > > diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297 > index 5c5420712b..b1a7d6d5e8 100755 > --- a/tests/qemu-iotests/297 > +++ b/tests/qemu-iotests/297 > @@ -30,13 +30,67 @@ if ! type -p "mypy" > /dev/null; then > _notrun "mypy not found" > fi > > -pylint-3 --score=n iotests.py > +# 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 > +) > > -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 > +file_list=() > +for file in *; do > + # Check files with a .py extension or a Python shebang > + # (Unless they are in the skip_files list) > + if [ -f "$file" ] && ((echo "$file" | grep -q '\.py$') || > + (head -n 1 "$file" | grep -q '^#!.*python')) > + then > + skip_file=false > + for skip in "${skip_files[@]}"; do > + if [ "$skip" = "$file" ]; then > + skip_file=true > + break > + fi > + done > + > + if ! $skip_file; then > + file_list+=("$file") > + fi > + fi > +done > + > +# Emit list of all files that are checked so we will not accidentally > +# lose coverage > +echo 'Files to be checked:' > + > +file_list_str='' > +for file in "${file_list[@]}"; do > + echo " $file" > +done | sort > + > +# We can pass all files to pylint at once... > +pylint-3 --score=n "${file_list[@]}" > + > +# ...but mypy needs to be called once per 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). > +for file in "${file_list[@]}"; do > + mypy_output=$( > + 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 "$file" \ > + 2>&1 > + ) > + > + if [ $? != 0 ]; then > + echo "$mypy_output" > + fi > +done > > # success, all done > echo "*** done" > diff --git a/tests/qemu-iotests/297.out b/tests/qemu-iotests/297.out > index 6acc843649..c5ebbf6a17 100644 > --- a/tests/qemu-iotests/297.out > +++ b/tests/qemu-iotests/297.out > @@ -1,3 +1,7 @@ > QA output created by 297 > -Success: no issues found in 1 source file > +Files to be checked: > + 209 > + 254 > + 283 > + iotests.py > *** done >
13.01.2021 22:27, Vladimir Sementsov-Ogievskiy wrote: > 13.01.2021 20:57, Max Reitz wrote: >> I.e., all Python files in the qemu-iotests/ directory. >> >> Most files of course 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.) >> >> I decided to include the list of files checked in the reference output, >> so we do not accidentally lose coverage of anything. That means adding >> new Python tests will require a change to 297.out, but that should not >> be a problem. > > I have a parallel series, "Rework iotests/check", one of its aims is drop > group file, to avoid these endless conflicts in group file when you want > to send series or when you are porting patches to/from downstream. > > And you are trying to add one another "group" file :) I don't like the idea. > > Why should we loose accidentally the coverage? Logic is extremely simple: > all files except for the list. > Also.. What about checking python in python :) ? I exercised myself, rewriting it into python. Take it if you like: (suddenly, pylint warns about "TODO"s, so I just drop a TODO line.. Probably we'll have to suppress this warning in 297) import os import shutil import subprocess import iotests iotests.script_initialize() def is_python_file(filename): if filename.endswith('.py'): return True with open(filename) as f: try: first_line = f.readline() if first_line.startswith('#!') and 'python' in first_line: return True except UnicodeDecodeError: # ignore core files, etc pass return False for linter in ('pylint-3', 'mypy'): if shutil.which(linter) is None: iotests.notrun(f'{linter} not found') 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' ) files = [f for f in (set(os.listdir('.')) - set(skip_files)) if os.path.isfile(f) and is_python_file(f)] # We can pass all files to pylint at once... subprocess.run(['pylint-3', '--score=n'] + files, check=False) # ...but mypy needs to be called once per 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). os.environ['MYPYPATH'] = '../../python/' for file 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', file], check=False, stdout=subprocess.PIPE, stderr=subprocess.STDOUT) if p.returncode != 0: print(p.stdout)
On 13.01.21 20:01, Eric Blake wrote: > On 1/13/21 11:57 AM, Max Reitz wrote: >> I.e., all Python files in the qemu-iotests/ directory. >> >> Most files of course 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.) > > Concur with the choice for default. > >> >> I decided to include the list of files checked in the reference output, >> so we do not accidentally lose coverage of anything. That means adding >> new Python tests will require a change to 297.out, but that should not >> be a problem. > > I'm okay with that. > >> >> On the other hand, I decided to hide mypy's "Success" lines from the >> reference output, because they do not add anything useful. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> tests/qemu-iotests/297 | 66 ++++++++++++++++++++++++++++++++++---- >> tests/qemu-iotests/297.out | 6 +++- >> 2 files changed, 65 insertions(+), 7 deletions(-) >> >> diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297 >> index 5c5420712b..b1a7d6d5e8 100755 >> --- a/tests/qemu-iotests/297 >> +++ b/tests/qemu-iotests/297 >> @@ -30,13 +30,67 @@ if ! type -p "mypy" > /dev/null; then >> _notrun "mypy not found" >> fi >> >> -pylint-3 --score=n iotests.py >> +# TODO: Empty this list! > > :) > > >> +file_list=() >> +for file in *; do >> + # Check files with a .py extension or a Python shebang >> + # (Unless they are in the skip_files list) >> + if [ -f "$file" ] && ((echo "$file" | grep -q '\.py$') || >> + (head -n 1 "$file" | grep -q '^#!.*python')) > > Bash has an (obsolete) operator (()) (behaves like a mix of $(()) and > 'if'); when nesting subshells, POSIX recommends inserting a space to > avoid inadvertent triggering of the alternate semantics of the operator. > But why do you need nested subshells? This is equivalent: > > if [ -f "$file" ] && (echo "$file" | grep -q '\.py$' || > head -n 1 "$file" | grep -q '^#!.*python') I just wasn’t sure of the order of pipe and ||. Sure, I can change it (depending on whether or not I rewrite it in Python, as suggested by Vladimir). >> + then >> + skip_file=false >> + for skip in "${skip_files[@]}"; do > > bashism, but iotests require bash, so fine. > >> + if [ "$skip" = "$file" ]; then >> + skip_file=true >> + break >> + fi >> + done >> + >> + if ! $skip_file; then >> + file_list+=("$file") > > Likewise. > > Whether or not you strip the extra (), > Reviewed-by: Eric Blake <eblake@redhat.com> Thanks!
On 13.01.21 20:27, Vladimir Sementsov-Ogievskiy wrote: > 13.01.2021 20:57, Max Reitz wrote: >> I.e., all Python files in the qemu-iotests/ directory. >> >> Most files of course 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.) >> >> I decided to include the list of files checked in the reference output, >> so we do not accidentally lose coverage of anything. That means adding >> new Python tests will require a change to 297.out, but that should not >> be a problem. > > I have a parallel series, "Rework iotests/check", one of its aims is drop > group file, to avoid these endless conflicts in group file when you want > to send series or when you are porting patches to/from downstream. > > And you are trying to add one another "group" file :) I don't like the > idea. I understand. > Why should we loose accidentally the coverage? Logic is extremely simple: > all files except for the list. I hope so. I just felt better having a reassurance that we indeed check everything we want to check. But if it isn’t feasible to keep this list, I guess we just can’t. Max
On 13.01.21 21:28, Vladimir Sementsov-Ogievskiy wrote: > 13.01.2021 22:27, Vladimir Sementsov-Ogievskiy wrote: >> 13.01.2021 20:57, Max Reitz wrote: >>> I.e., all Python files in the qemu-iotests/ directory. >>> >>> Most files of course 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.) >>> >>> I decided to include the list of files checked in the reference output, >>> so we do not accidentally lose coverage of anything. That means adding >>> new Python tests will require a change to 297.out, but that should not >>> be a problem. >> >> I have a parallel series, "Rework iotests/check", one of its aims is drop >> group file, to avoid these endless conflicts in group file when you want >> to send series or when you are porting patches to/from downstream. >> >> And you are trying to add one another "group" file :) I don't like the >> idea. >> >> Why should we loose accidentally the coverage? Logic is extremely simple: >> all files except for the list. >> > > Also.. What about checking python in python :) ? I exercised myself, > rewriting it into python. Take it if you like: Why not, actually. > (suddenly, pylint warns about "TODO"s, so I just drop a TODO line.. > Probably > we'll have to suppress this warning in 297) I’d suppress this warning altogether, no? I would like to keep TODO lines in other tests, too, without 297 failing. Max
14.01.2021 12:31, Max Reitz wrote: > On 13.01.21 21:28, Vladimir Sementsov-Ogievskiy wrote: >> 13.01.2021 22:27, Vladimir Sementsov-Ogievskiy wrote: >>> 13.01.2021 20:57, Max Reitz wrote: >>>> I.e., all Python files in the qemu-iotests/ directory. >>>> >>>> Most files of course 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.) >>>> >>>> I decided to include the list of files checked in the reference output, >>>> so we do not accidentally lose coverage of anything. That means adding >>>> new Python tests will require a change to 297.out, but that should not >>>> be a problem. >>> >>> I have a parallel series, "Rework iotests/check", one of its aims is drop >>> group file, to avoid these endless conflicts in group file when you want >>> to send series or when you are porting patches to/from downstream. >>> >>> And you are trying to add one another "group" file :) I don't like the idea. >>> >>> Why should we loose accidentally the coverage? Logic is extremely simple: >>> all files except for the list. >>> >> >> Also.. What about checking python in python :) ? I exercised myself, >> rewriting it into python. Take it if you like: > > Why not, actually. > >> (suddenly, pylint warns about "TODO"s, so I just drop a TODO line.. Probably >> we'll have to suppress this warning in 297) > > I’d suppress this warning altogether, no? I would like to keep TODO lines in other tests, too, without 297 failing. > I'm for. Otherwise it means we just can't use "TODO" things)
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297 index 5c5420712b..b1a7d6d5e8 100755 --- a/tests/qemu-iotests/297 +++ b/tests/qemu-iotests/297 @@ -30,13 +30,67 @@ if ! type -p "mypy" > /dev/null; then _notrun "mypy not found" fi -pylint-3 --score=n iotests.py +# 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 +) -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 +file_list=() +for file in *; do + # Check files with a .py extension or a Python shebang + # (Unless they are in the skip_files list) + if [ -f "$file" ] && ((echo "$file" | grep -q '\.py$') || + (head -n 1 "$file" | grep -q '^#!.*python')) + then + skip_file=false + for skip in "${skip_files[@]}"; do + if [ "$skip" = "$file" ]; then + skip_file=true + break + fi + done + + if ! $skip_file; then + file_list+=("$file") + fi + fi +done + +# Emit list of all files that are checked so we will not accidentally +# lose coverage +echo 'Files to be checked:' + +file_list_str='' +for file in "${file_list[@]}"; do + echo " $file" +done | sort + +# We can pass all files to pylint at once... +pylint-3 --score=n "${file_list[@]}" + +# ...but mypy needs to be called once per 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). +for file in "${file_list[@]}"; do + mypy_output=$( + 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 "$file" \ + 2>&1 + ) + + if [ $? != 0 ]; then + echo "$mypy_output" + fi +done # success, all done echo "*** done" diff --git a/tests/qemu-iotests/297.out b/tests/qemu-iotests/297.out index 6acc843649..c5ebbf6a17 100644 --- a/tests/qemu-iotests/297.out +++ b/tests/qemu-iotests/297.out @@ -1,3 +1,7 @@ QA output created by 297 -Success: no issues found in 1 source file +Files to be checked: + 209 + 254 + 283 + iotests.py *** done
I.e., all Python files in the qemu-iotests/ directory. Most files of course 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.) I decided to include the list of files checked in the reference output, so we do not accidentally lose coverage of anything. That means adding new Python tests will require a change to 297.out, but that should not be a problem. On the other hand, I decided to hide mypy's "Success" lines from the reference output, because they do not add anything useful. Signed-off-by: Max Reitz <mreitz@redhat.com> --- tests/qemu-iotests/297 | 66 ++++++++++++++++++++++++++++++++++---- tests/qemu-iotests/297.out | 6 +++- 2 files changed, 65 insertions(+), 7 deletions(-)