Re: [PATCH 07/10] iotests/297: return error code from run_linters()
On Tue, Jul 13, 2021 at 5:44 AM Vladimir Sementsov-Ogievskiy <
vsement...@virtuozzo.com> wrote:
> 25.06.2021 21:20, John Snow wrote:
> > This turns run_linters() into a bit of a hybrid test; returning non-zero
> > on failed execution while also printing diffable information. This is
> > done for the benefit of the avocado simple test runner, which will soon
> > be attempting to execute this test from a different environment.
> >
> > (Note: universal_newlines is added to the pylint invocation for type
> > consistency with the mypy run -- it's not strictly necessary, but it
> > avoids some typing errors caused by our re-use of the 'p' variable.)
> >
> > Signed-off-by: John Snow
> > ---
> > tests/qemu-iotests/297 | 10 --
> > 1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
> > index 1e8334d1d4..7db1f9ed45 100755
> > --- a/tests/qemu-iotests/297
> > +++ b/tests/qemu-iotests/297
> > @@ -68,19 +68,22 @@ def run_linters(
> > files: List[str],
> > directory: str = '.',
> > env: Optional[Mapping[str, str]] = None,
> > -) -> None:
> > +) -> int:
> > +ret = 0
> >
> > 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(
> > +p = subprocess.run(
> > ('python3', '-m', 'pylint', '--score=n', '--notes=FIXME,XXX',
> *files),
> > cwd=directory,
> > env=env,
> > check=False,
> > +universal_newlines=True,
>
> Why you need this universal_newlines=True, if you don't handle output?
>
>
Typing consistency. It changes the type of `p`, which we re-use. Could also
be solved by using two different local binding names.
--js
> with or without it:
> Reviewed-by: Vladimir Sementsov-Ogievskiy
>
>
Tentatively, I'll take this RB.
> > )
> > +ret += p.returncode
> >
> > print('=== mypy ===')
> > sys.stdout.flush()
> > @@ -113,9 +116,12 @@ def run_linters(
> > universal_newlines=True
> > )
> >
> > +ret += p.returncode
> > if p.returncode != 0:
> > print(p.stdout)
> >
> > +return ret
> > +
> >
> > def main() -> None:
> > for linter in ('pylint-3', 'mypy'):
> >
>
>
> --
> Best regards,
> Vladimir
>
>
Re: [PATCH 07/10] iotests/297: return error code from run_linters()
25.06.2021 21:20, John Snow wrote:
This turns run_linters() into a bit of a hybrid test; returning non-zero
on failed execution while also printing diffable information. This is
done for the benefit of the avocado simple test runner, which will soon
be attempting to execute this test from a different environment.
(Note: universal_newlines is added to the pylint invocation for type
consistency with the mypy run -- it's not strictly necessary, but it
avoids some typing errors caused by our re-use of the 'p' variable.)
Signed-off-by: John Snow
---
tests/qemu-iotests/297 | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 1e8334d1d4..7db1f9ed45 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -68,19 +68,22 @@ def run_linters(
files: List[str],
directory: str = '.',
env: Optional[Mapping[str, str]] = None,
-) -> None:
+) -> int:
+ret = 0
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(
+p = subprocess.run(
('python3', '-m', 'pylint', '--score=n', '--notes=FIXME,XXX', *files),
cwd=directory,
env=env,
check=False,
+universal_newlines=True,
Why you need this universal_newlines=True, if you don't handle output?
with or without it:
Reviewed-by: Vladimir Sementsov-Ogievskiy
)
+ret += p.returncode
print('=== mypy ===')
sys.stdout.flush()
@@ -113,9 +116,12 @@ def run_linters(
universal_newlines=True
)
+ret += p.returncode
if p.returncode != 0:
print(p.stdout)
+return ret
+
def main() -> None:
for linter in ('pylint-3', 'mypy'):
--
Best regards,
Vladimir
Re: [PATCH 07/10] iotests/297: return error code from run_linters()
13.07.2021 02:56, John Snow wrote:
On Tue, Jul 6, 2021 at 5:49 AM Vladimir Sementsov-Ogievskiy mailto:vsement...@virtuozzo.com>> wrote:
25.06.2021 21:20, John Snow wrote:
> This turns run_linters() into a bit of a hybrid test; returning non-zero
> on failed execution while also printing diffable information. This is
> done for the benefit of the avocado simple test runner, which will soon
> be attempting to execute this test from a different environment.
>
> (Note: universal_newlines is added to the pylint invocation for type
> consistency with the mypy run -- it's not strictly necessary, but it
> avoids some typing errors caused by our re-use of the 'p' variable.)
>
> Signed-off-by: John Snow mailto:js...@redhat.com>>
> ---
> tests/qemu-iotests/297 | 10 --
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
> index 1e8334d1d4..7db1f9ed45 100755
> --- a/tests/qemu-iotests/297
> +++ b/tests/qemu-iotests/297
> @@ -68,19 +68,22 @@ def run_linters(
> files: List[str],
> directory: str = '.',
> env: Optional[Mapping[str, str]] = None,
> -) -> None:
> +) -> int:
> + ret = 0
>
> 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(
> + p = subprocess.run(
> ('python3', '-m', 'pylint', '--score=n', '--notes=FIXME,XXX',
*files),
> cwd=directory,
> env=env,
> check=False,
> + universal_newlines=True,
> )
> + ret += p.returncode
>
> print('=== mypy ===')
> sys.stdout.flush()
> @@ -113,9 +116,12 @@ def run_linters(
> universal_newlines=True
> )
>
> + ret += p.returncode
> if p.returncode != 0:
> print(p.stdout)
>
> + return ret
> +
>
> def main() -> None:
> for linter in ('pylint-3', 'mypy'):
>
Hmm..
1. Rather unusual for a function in python to return int error-code, more
usual is raising exceptions..
It is strange, but I felt that if these tests were going to run in "two
contexts" that I would avoid raising Exceptions and trying to understand how it
would affect either call stack.
2. making a sum of return codes looks odd to me
Just a cheap way to state that a 0 return is good, and a non-zero return code
is failure.
3. Do we really want to run mypy if pylint failed? Maybe better not doing
it, and just switch s/check=False/check=True/ ? This way:
I suppose we could. For the sake of CI, I like seeing more output instead of
less so that you can save yourself the trouble and fix everything before
re-submitting the CI job. What do you think?
Hmm, that's reasonable.
3.1 the function becomes native wrapper for subprocess.run, and raise same
exceptions
3.2 we don't waste CI time by running mypy when pylint failed anyway
--
Best regards,
Vladimir
--
Best regards,
Vladimir
Re: [PATCH 07/10] iotests/297: return error code from run_linters()
On Tue, Jul 6, 2021 at 5:49 AM Vladimir Sementsov-Ogievskiy <
vsement...@virtuozzo.com> wrote:
> 25.06.2021 21:20, John Snow wrote:
> > This turns run_linters() into a bit of a hybrid test; returning non-zero
> > on failed execution while also printing diffable information. This is
> > done for the benefit of the avocado simple test runner, which will soon
> > be attempting to execute this test from a different environment.
> >
> > (Note: universal_newlines is added to the pylint invocation for type
> > consistency with the mypy run -- it's not strictly necessary, but it
> > avoids some typing errors caused by our re-use of the 'p' variable.)
> >
> > Signed-off-by: John Snow
> > ---
> > tests/qemu-iotests/297 | 10 --
> > 1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
> > index 1e8334d1d4..7db1f9ed45 100755
> > --- a/tests/qemu-iotests/297
> > +++ b/tests/qemu-iotests/297
> > @@ -68,19 +68,22 @@ def run_linters(
> > files: List[str],
> > directory: str = '.',
> > env: Optional[Mapping[str, str]] = None,
> > -) -> None:
> > +) -> int:
> > +ret = 0
> >
> > 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(
> > +p = subprocess.run(
> > ('python3', '-m', 'pylint', '--score=n', '--notes=FIXME,XXX',
> *files),
> > cwd=directory,
> > env=env,
> > check=False,
> > +universal_newlines=True,
> > )
> > +ret += p.returncode
> >
> > print('=== mypy ===')
> > sys.stdout.flush()
> > @@ -113,9 +116,12 @@ def run_linters(
> > universal_newlines=True
> > )
> >
> > +ret += p.returncode
> > if p.returncode != 0:
> > print(p.stdout)
> >
> > +return ret
> > +
> >
> > def main() -> None:
> > for linter in ('pylint-3', 'mypy'):
> >
>
> Hmm..
>
> 1. Rather unusual for a function in python to return int error-code, more
> usual is raising exceptions..
>
>
It is strange, but I felt that if these tests were going to run in "two
contexts" that I would avoid raising Exceptions and trying to understand
how it would affect either call stack.
> 2. making a sum of return codes looks odd to me
>
>
Just a cheap way to state that a 0 return is good, and a non-zero return
code is failure.
> 3. Do we really want to run mypy if pylint failed? Maybe better not doing
> it, and just switch s/check=False/check=True/ ? This way:
>
>
I suppose we could. For the sake of CI, I like seeing more output instead
of less so that you can save yourself the trouble and fix everything before
re-submitting the CI job. What do you think?
> 3.1 the function becomes native wrapper for subprocess.run, and raise same
> exceptions
> 3.2 we don't waste CI time by running mypy when pylint failed anyway
>
>
> --
> Best regards,
> Vladimir
>
>
Re: [PATCH 07/10] iotests/297: return error code from run_linters()
25.06.2021 21:20, John Snow wrote:
This turns run_linters() into a bit of a hybrid test; returning non-zero
on failed execution while also printing diffable information. This is
done for the benefit of the avocado simple test runner, which will soon
be attempting to execute this test from a different environment.
(Note: universal_newlines is added to the pylint invocation for type
consistency with the mypy run -- it's not strictly necessary, but it
avoids some typing errors caused by our re-use of the 'p' variable.)
Signed-off-by: John Snow
---
tests/qemu-iotests/297 | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 1e8334d1d4..7db1f9ed45 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -68,19 +68,22 @@ def run_linters(
files: List[str],
directory: str = '.',
env: Optional[Mapping[str, str]] = None,
-) -> None:
+) -> int:
+ret = 0
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(
+p = subprocess.run(
('python3', '-m', 'pylint', '--score=n', '--notes=FIXME,XXX', *files),
cwd=directory,
env=env,
check=False,
+universal_newlines=True,
)
+ret += p.returncode
print('=== mypy ===')
sys.stdout.flush()
@@ -113,9 +116,12 @@ def run_linters(
universal_newlines=True
)
+ret += p.returncode
if p.returncode != 0:
print(p.stdout)
+return ret
+
def main() -> None:
for linter in ('pylint-3', 'mypy'):
Hmm..
1. Rather unusual for a function in python to return int error-code, more usual
is raising exceptions..
2. making a sum of return codes looks odd to me
3. Do we really want to run mypy if pylint failed? Maybe better not doing it,
and just switch s/check=False/check=True/ ? This way:
3.1 the function becomes native wrapper for subprocess.run, and raise same
exceptions
3.2 we don't waste CI time by running mypy when pylint failed anyway
--
Best regards,
Vladimir
[PATCH 07/10] iotests/297: return error code from run_linters()
This turns run_linters() into a bit of a hybrid test; returning non-zero
on failed execution while also printing diffable information. This is
done for the benefit of the avocado simple test runner, which will soon
be attempting to execute this test from a different environment.
(Note: universal_newlines is added to the pylint invocation for type
consistency with the mypy run -- it's not strictly necessary, but it
avoids some typing errors caused by our re-use of the 'p' variable.)
Signed-off-by: John Snow
---
tests/qemu-iotests/297 | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 1e8334d1d4..7db1f9ed45 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -68,19 +68,22 @@ def run_linters(
files: List[str],
directory: str = '.',
env: Optional[Mapping[str, str]] = None,
-) -> None:
+) -> int:
+ret = 0
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(
+p = subprocess.run(
('python3', '-m', 'pylint', '--score=n', '--notes=FIXME,XXX', *files),
cwd=directory,
env=env,
check=False,
+universal_newlines=True,
)
+ret += p.returncode
print('=== mypy ===')
sys.stdout.flush()
@@ -113,9 +116,12 @@ def run_linters(
universal_newlines=True
)
+ret += p.returncode
if p.returncode != 0:
print(p.stdout)
+return ret
+
def main() -> None:
for linter in ('pylint-3', 'mypy'):
--
2.31.1
