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

[John Snow]

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.

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?)




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.