To error out or to skip tests?

[Reece Dunn]

2009/11/9  <Joerg-Cyril.Hoehle at t-systems.com>:
> Reece Dunn wrote:
>
>>> The question remains:
>>> a) turn sound tests red on machines without sound (with 1
>>error only);
>>> b) turn them blue (clearly marking skipped tests)
>>> c) turn them green -- status quo (for dsound.ok and wave.ok).
>
>> 310     ok(!err,"mci open waveaudio!tempfile.wav returned error: %d\n", err);
>> 311     if(err) {
>> 312         skip("tempfile.wav was not found (not saved), skipping\n");
>>Here, you are doing both (generating an error and skipping)!
>
> On purpose. I wanted an error flagged to point people's attention to the
> issue of machines without sound, so that it can be discussed.

What has this to do with the issue of machines without sound.

The ok statement is saying that "mci open" has failed (for *any*
reason). The skip (activated on *all* error codes) is saying that
tempfile.wav is not found, even for cases such as err ==
MCIERR_CANNOT_LOAD_DRIVER!

>>I think that it is safe to say that if there is no audio driver
>>present, all subsequent mci tests will fail and should also be skipped
>>(ideally with just the single warning, e.g. by returning FALSE from
>>that function and wrapping the other tests in an if).
>
> My current line of thought is that broken and skip are somewhat *bad*
> things, because by not performing tests, we don't learn anything about
> the behaviour in "unusual" circumstances.

The point I am making is that:
  1.  The error and skip statements are saying the same thing;
  2.  But the skip message is misleading.

That is, it is valid to test MCI in the case when there is no sound
device, but the tests are being confused by a lack of tempfile.wav.

For example:
   mci.c:310: Test failed: mci open waveaudio!tempfile.wav returned error: 275
   mci.c:312: Tests skipped: tempfile.wav was not found (not saved), skipping
If I have counted the offsets correctly 275 is
MCIERR_DEVICE_NOT_READY, *not* that tempfile.wav was not found.

> The relevant question becomes: when is a configuration unusual? When
> can it be ignored for the purpose of Wine and when is it relevant?

Answer: no configuration is unusual.

> - I don't care that tests fail on my father in law's machine with a
>  broken sound card (or rather, the card maybe ok, but somehow the
>  sndblast.dll doen't manager to install it).

The point is, if the machine has a broken soundcard (or in Wine does
not have an available sound driver, or is broken due to PulseAudio on
Ubuntu), the tests should still pass (or be skipped) as is
appropriate.

>  Yet testing on that machine was very revealing (black box testing
>  with broken configurations tells you a lot about what's inside, like
>  scientists learn a lot from patients with a local brain disease).

Sure. But (a) the tests should still succeed (and doing so, document
the behaviour for both these cases) and (b) should succeed or fail
*for the right reason*.

> - Ge Geldorp pointing out that some vmware engines do not support
>  sound at all renders Windows without sound not unusual -- unlike
>  what I initially thought.

Sure.

> My conclusion is that we need tests to specifically find out how
> MS-Windows behaves on such machines, so that Wine can mimick it.
> That might be written entirely without skip() or broken(), because
> it becomes expected behaviour.

But please make sure that they are failing in a way that is expected,

It also seems to me that there should be some tests that test opening
an mci stream on a file that we know not to exist (e.g.
this-file-does-not-exist.wav) so that those failure cases can be
properly documented.

> if (wave_GetNumDevs()== 0) {
>   skip("no sound, no tests\n"); return;
> is *not* the answer IMHO.  Turning all lights green is not a primary
> goal, it's a secondary/subordinated one.

You have the basic API and parameter tests that can be done. You have
the playback tests. And you have the playback tests for non-existent
files.

In the above example, you have the case of what happens when there are
no devices available. Here, I would argue with you that skipping at
that point does not make sense. However, skipping at the
mciSendString("open ...") when you revieve an
MCIERR_CANNOT_LOAD_DRIVER (or whatever the error code is that is
returned), *then* you skip (saying "unable to load the mci driver").

> What's the purpose of skip()? Nothing but generate blue colour in
> test.winehq (use trace(); return; if all you need is print something
> and avoid other tests).  What's the purpose of the blue colour?
> - A call for action?
>  + "please run again in the english locale so I can perform locale-dependent tests";
>  + "please install sound so that you'll know whether it works on BSD"
>  + "please install Gecko so I can perform more tests"
> - ...?

No, skip is saying that the tests cannot be run as the environment
does not support the functionality we require. Do you fail the Win9x
tests just because they don't support *W API variants? Or Windows
environments that don't have DirectX 7, 8, 9 or 10 installed?

>>Shouldn't this be something like:
>>      if(err == MCIERR_INVALID_FILE) {
>>          skip("tempfile.wav was not found (not saved?), skipping\n");
> No. If any error happens in open, we can't continue. Perhaps you mean
> if (err) {
>   ok(err==MCIERR_FILE_NOT_FOUND, "...\n"); /* the expected situation */
>   skip("tempfile.wav was not found (not saved?), skipping\n");

I meant MCIERR_FILE_NOT_FOUND, but the point was not to error in this
case. Therefore, something like this:

   err=mciSendString("open ....");
   switch (err)
   {
   case MCIERR_FILE_NOT_FOUND:
      skip("tempfile.wav was not found (not saved?)");
      return;
   case MCIERR_CANNOT_LOAD_DRIVER:
   case MCIERR_...:
      skip("unable to open the mci sound driver (%s) - is sound
enabled?", dbg_mcierr(err));
      return;
   default:
      ok(!err, "mciSendString('open') failed, expected NOERROR, got
%s", dbg_mcierr(err));
   }

But then, as you said, any failure here means that you cannot proceed.
However, the main cases are:
   1.  tempfile.wav is not present;
   2.  sound is not present on the system.

The other errors (MCIERR_UNRECOGNISED_COMMAND, MCIERR_HARDWARE,
MCIERR_OUT_OF_MEMORY) are either:
   1.  situations that cannot occur in the given test;
   2.  are unexpected error cases (i.e. situations that we do not
expect on a normal run).

Therefore, the above code (with the correct error values) should be
sufficient. If we find that other situations need correcting (due to
failing tests), they can be looked at then.

- Reece