On 19.04.2016 14:22, Sascha Silbe wrote:
> Dear Max,
>
> Max Reitz <mre...@redhat.com> writes:
>
>> On 14.04.2016 13:32, Sascha Silbe wrote:
> [tests/qemu-iotests/iotests.py]
> [...]
>>> def main(supported_fmts=[], supported_oses=['linux']):
>>> '''Run tests'''
>>>
>>> + if test_dir is None or qemu_default_machine is None:
>>
>> I think checking test_dir would have been sufficient; this makes it look
>> like it might want to be a complete list of variables that have to be
>> set, but then "cachemode" is missing.
>
> Markus Armbruster basically pointed out the same issue, so how about
> this version:
>
> # We are using TEST_DIR and QEMU_DEFAULT_MACHINE as proxies to
> # indicate that we're not being run via "check". There may be
> # other things set up by "check" that individual test cases rely
> # on.
> if test_dir is None or qemu_default_machine is None:
> sys.stderr.write('Please run this test via the "check" script\n')
> sys.exit(os.EX_USAGE)
I'm OK with that. I can imagine Markus isn't, because it's implying that
you should only run this test through "check", whereas Markus says that
maybe you have your own script and that is fine, too.
I think two things:
1. I'm not sure why you would want your own script. If the check script
isn't good enough, extend it.
2. If you do want your own script, I guess setting up the necessary
environment for each test is complicated enough to require you to know
what you're doing. And if you know what you're doing, you won't really
care about the wording of this warning anyway.
I think this is just a warning for unwary users who just try to execute
a python test directly because they think that maybe they can. And then
this line is just telling them that no, that is not how the test is
supposed to be run; instead, it tells them the most convenient and
common way to run it.
I think it would be confusing if we printed the exact technical details
here which basically nobody cares about anyway.
So I'm completely fine with this version.
>>> + sys.stderr.write('Please run this test via ./check\n')
>>
>> Or not ./, for people who have separate build and source trees, you
>> don't want to invoke check in the source tree. ;-)
>
> Yeah, was thinking about that, but ultimately considered ./check to be
> the best indication of a) "check" being the name of a script and b)
> residing in the same directory as the test. But I don't care much about
> this, so see the above version.
Yes, that looks fine to me.
Regarding b): If you separate the build tree from the source tree, you
have to run the check script from the build tree. During the build
process, qemu will in fact automatically create a symlink in the build
tree. Therefore, in that case, you don't want to execute "./check" in
the same directory as the test is in.
Max
signature.asc
Description: OpenPGP digital signature
