On Wed, Oct 13, 2021 at 8:11 AM Hanna Reitz <hre...@redhat.com> wrote:
> On 04.10.21 23:05, John Snow wrote: > > We need at least a tiny little shim here to join test file discovery > > with test invocation. This logic could conceivably be hosted somewhere > > in python/, but I felt it was strictly the least-rude thing to keep the > > test logic here in iotests/, even if this small function isn't itself an > > iotest. > > > > Note that we don't actually even need the executable bit here, we'll be > > relying on the ability to run this module as a script using Python CLI > > arguments. No chance it gets misunderstood as an actual iotest that way. > > > > (It's named, not in tests/, doesn't have the execute bit, and doesn't > > have an execution shebang.) > > > > Signed-off-by: John Snow <js...@redhat.com> > > > > --- > > > > (1) I think that the test file discovery logic and skip list belong > together, > > and that those items belong in iotests/. I think they also belong in > > whichever directory pylintrc and mypy.ini are in, also in iotests/. > > Agreed. > > > (2) Moving this logic into python/tests/ is challenging because I'd have > > to import iotests code from elsewhere in the source tree, which just > > inverts an existing problem I have been trying to rid us of -- > > needing to muck around with PYTHONPATH or sys.path hacking in python > > scripts. I'm keen to avoid this. > > OK. > > > (3) If we moved all python tests into tests/ and gave them *.py > > extensions, we wouldn't even need the test discovery functions > > anymore, and all of linters.py could be removed entirely, including > > this execution shim. We could rely on mypy/pylint's own file > > discovery mechanisms at that point. More work than I'm up for with > > just this series, but I could be coaxed into doing it if there was > > some promise of not rejecting all that busywork ;) > > I believe the only real value this would gain is that the tests would > have nicer names and we would have to delint them. If we find those > goals to justify the effort, then we can put in the effort; and we can > do that slowly, test by test. I don’t think we must do it now in one > big lump just to get rid of the file discovery functions. > > Yeah, I agree -- just do it over time and as-needed. I'm sure I will be bothered by something-or-other sooner-or-later and I'll wind up doing it anyway. Just maybe not this week! > > Signed-off-by: John Snow <js...@redhat.com> > > --- > > tests/qemu-iotests/linters.py | 18 ++++++++++++++++++ > > 1 file changed, 18 insertions(+) > > > > diff --git a/tests/qemu-iotests/linters.py > b/tests/qemu-iotests/linters.py > > index f6a2dc139fd..191df173064 100644 > > --- a/tests/qemu-iotests/linters.py > > +++ b/tests/qemu-iotests/linters.py > > @@ -16,6 +16,7 @@ > > import os > > import re > > import subprocess > > +import sys > > from typing import List, Mapping, Optional > > > > > > @@ -81,3 +82,20 @@ def run_linter( > > > > return p.returncode > > > > + > > +def main() -> int: > > + """ > > + Used by the Python CI system as an entry point to run these linters. > > + """ > > + files = get_test_files() > > + > > + if sys.argv[1] == '--pylint': > > + return run_linter('pylint', files) > > + elif sys.argv[1] == '--mypy': > > + return run_linter('mypy', files) > > So I can run it as `python linters.py --pylint foo bar` and it won’t > complain? :) > > I don’t feel like it’s important, but, well, it isn’t right either. > > Alright. I hacked it together to be "minimal" in terms of SLOC, but I can make it ... less minimal.