Re: [Qemu-devel] [PATCH for-2.6?] qemu-iotests: iotests: fail hard if not run via "check"

[Markus Armbruster <armbru@redhat.com>]

Sascha Silbe <silbe@linux.vnet.ibm.com> writes:

> Dear Markus,
>
> Markus Armbruster <armbru@redhat.com> writes:
>
>>> Running an iotests-based Python test directly might appear to work,
>>> but may fail in subtle ways and is insecure:
>>>
>>> - It creates files with predictable file names in a world-writable
>>>   location (/var/tmp).
>>>
>>> - Tests expect the environment to be set up by check. E.g. 041 and 055
>>>   may take the wrong code paths if QEMU_DEFAULT_MACHINE is not
>>>   set. This can lead to false negatives.
>>>
>>> Instead fail hard and tell the user we want to be run via "check".
>>
>> Are the problems serious enough to warrant rejecting the attempt to run
>> the tests directly outright?
>>
>> Judging from your description, I'm inclined to "no".
>
> Having tests do unexpected things and / or fail silently is "serious" in
> my book. After all, what's the value of tests if they don't yell when
> something is broken?
>
>
> [tests/qemu-iotests/iotests.py]
> [...]
>>> +    if test_dir is None or qemu_default_machine is None:
>>> +        sys.stderr.write('Please run this test via ./check\n')
>>> +        sys.exit(os.EX_USAGE)
>>> +
>>
>> Aha, you're not actually rejecting the attempt to run the tests
>> directly, you're merely requiring two environment variables.  That's
>> okay with me. [...]
>
> I should probably add a comment that these two environment variables are
> merely used as proxies that indicate we haven't been run via
> "check". They are the ones where there isn't any useful default value we
> could fall back, but without a full audit we can't tell what else some
> of the tests may expect that would normally be set up by "check" (or at
> least I can't).

Say you had an accurate way to find out whether we're running under
"check".  You could then reject any attempt to run the test directly.
I'd oppose that.

It's okay to have test wrapper scripts to configure the tests just so.
It's okay to tell people to use them.  But "you can't do that, Dave" is
not okay.

It's okay to require certain environment variables to be set.  That's
what your patch does (but your commit message doesn't quite say).

It would not be okay to interfere with my ability to run tests the way
*I* want.  If a test breaks when I run it my way, I get to keep the
pieces.