Re: [PATCH v3 06/10] iotests: add testfinder.py

[Vladimir Sementsov-Ogievskiy]

14.05.2020 08:06, John Snow wrote:
> On 5/14/20 12:54 AM, Vladimir Sementsov-Ogievskiy wrote:
> > 14.05.2020 00:58, John Snow wrote:
> > > On 5/7/20 1:43 PM, Vladimir Sementsov-Ogievskiy wrote:
> > > > 21.04.2020 19:56, Kevin Wolf wrote:
> > > > > Am 21.04.2020 um 09:35 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > > > > Add python script with new logic of searching for tests:
> > > > > >
> > > > > > Current ./check behavior:
> > > > > >     - tests are named [0-9][0-9][0-9]
> > > > > >     - tests must be registered in group file (even if test doesn't
> > > > > > belong
> > > > > >       to any group, like 142)
> > > > > >
> > > > > > Behavior of new test:
> > > > > >     - group file is dropped
> > > > > >     - tests are searched by file-name instead of group file, so it's
> > > > > > not
> > > > > >       needed more to "register the test", just create it with name
> > > > > >       *-test. Old names like [0-9][0-9][0-9] are supported too, but not
> > > > > >       recommended for new tests
> > > > >
> > > > > I wonder if a tests/ subdirectory instead of the -test suffix would
> > > > > organise things a bit better.
> > > >
> > > > It will make more difficult to import iotests.py.. Calling common.rc
> > > > from
> > > > bash tests will need to be modified too.
> > > >
> > > > So, we'll need additional line in all python tests:
> > > >
> > > > sys.path.append(os.path.join(os.path.dirname(__file__), '..'))
> > > >
> > > > which doesn't seem to be good practice.. So, instead we'd better call
> > > > tests with PYTHONPATH set appropriately..
> > >
> > > Just chiming in to say that it's largely bad practice because it
> > > confuses pylint, mypy and friends -- if we want to keep pushing our CI
> > > code analysis for python in that direction, this will be a barrier.
> > >
> > > Using PYTHONPATH is better, because it isolates the script itself from
> > > the environment, but requires you to now always set PYTHONPATH to
> > > execute any of the individual iotests.
> > >
> > > Not actually a big deal, because iotests already expect a large number
> > > of environment variables to be set. It's not really a huge net loss in
> > > convenience, I think.
> > >
> > > looks like that's the direction you're headed in anyway based on
> > > discussion, so that's good.
> >
> > Hm, does PYTHONPATH-way works good with mypy and friends? Probably, it
> > should
> > be set when checking the code? So, actually developers will have to set
> > PYTHONPATH by hand to always contain some directories within qemu source
> > tree?
>
> pylint respects PYTHONPATH but mypy doesn't. mypy uses MYPYPATH, but I
> wouldn't worry about accommodating it. It's a fussy tool and we're only
> ever going to run it from very specific environments.

Hmm, recently I installed dense-analysis/ale plugin into my vim which does mypy 
checking (among other things).. And most probably, I'll have to set these 
variables to keep it working. But it's not a big problem.

> You don't need to worry too much about what environment variables these
> tools take; it's only worth noting that "sys.path" hacks tend to make
> these tools harder to use.
>
>
> As for setting PYTHONPATH by hand ... There are a few places in the QEMU
> tree where we set PYTHONPATH already, and the individual iotests already
> don't work if they're not launched by `check`, because they're missing a
> ton of environment variables.
>
> It's not going to be too bad to set PYTHONPATH in the launcher script,
> is it?
>
> (Or are we replacing the top-level script with a python one?)

Yes we do, bright future is near:) But it's not a problem to set PYTHONPATH in 
it. Anyway, we run all tests as executables, so passing PYTHONPATH is a valid 
thing to do.

> Really, the same is true of pylint, too. It's only annoying to deal with
> sys.path hacking because it can't be worked around in those CQA tools.


--
Best regards,
Vladimir