Re: mmdevapi/tests: Add format tests.

[Andrew Eikum]

Hi J�rg,

Thanks for adding these tests. It would have been nicer to split the
patch up into discrete chunks. That would make the patch much easier
to review. Overall it looks pretty good. Some remarks follow...
> BTW, I've not touched
> render.c:223: Test failed: IsFormatSupported(0xffffffff) call returns 8889000e
> capture.ok has a similar random failure.  I believe that test is bogus.  It
> should avoid ending up calling the exclusive mode "audio endpoint" and hope/show
> that the shared mode mixer aka. "audio engine" reports E_INVALIDARG.  Perhaps
> using ~AUDCLNT_SHAREMODE_EXCLUSIVE is a means to enforce this, even though
> that's actually an enum, not a flag.

Yeah, that's a pretty uninteresting test. If it's flaky, I don't see
any compelling reason to keep it around.
> - unk = (void*)0xDEADF00D;
> hr = IAudioClient_GetService(ac, &IID_IAudioStreamVolume, (void**)&unk);
> ok(hr == AUDCLNT_E_NOT_INITIALIZED, "Uninitialized GetService call returns %08x\n", hr);
> - todo_wine ok(broken(unk != NULL), "GetService %08x &out pointer %p\n", hr, unk);

This test is strange. broken() basically means "should fail on
Windows, should not fail in Wine." It's used when some particular
version of Windows does something unexpected which we are uninterested
in duplicating. Do you expect unk to always be non-NULL? If so, then
just test for that without the broken() macro.
> +static void test_formats(AUDCLNT_SHAREMODE mode)

The code in this function is very difficult to follow. I wonder if it
couldn't be written more clearly with explicit conditions and a bit
more code duplication.
> -
>    ok((hr == S_FALSE)^(pwfx2 == NULL), "hr %x<->suggest %p\n", hr, pwfx2);
> -
>    if (pwfx2 == (WAVEFORMATEX*)0xDEADF00D) /* bug in Wine */
> -
>        pwfx2 = NULL;

Would be nicer to both fix the bug in Wine and have a test showing the
correct behavior.
> @@ -1378,7 +1535,7 @@ START_TEST(render)
>      hr = CoCreateInstance(&CLSID_MMDeviceEnumerator, NULL, CLSCTX_INPROC_SERVER, &IID_IMMDeviceEnumerator, (void**)&mme);
>      if (FAILED(hr))
>      {
> -
>    skip("mmdevapi not available: 0x%08x\n", hr);
> -
>    win_skip("mmdevapi not available: 0x%08x\n", hr);

If the user has an invalid driver, CoCreateInstance() will fail to
create the enumerator object. I think that's valid behavior, so we
shouldn't fail.

Andrew

mmdevapi/tests: Add format tests.

Hi,

Andrew Eikum wrote:
> - todo_wine ok(broken(unk != NULL), ...

I'll comment on that separately.
> > +static void test_formats(AUDCLNT_SHAREMODE mode)
>
> The code in this function is very difficult to follow.

Indeed, it became more and more convoluted as I dug into native's
"exclusive mode disabled" and other exceptions to the rule.
And it's not even handling 5:1 cards correctly!
Luckily, I needn't add todo_wine on top of that...
For instance, winmm:wave.ok is so convoluted that
one cannot tell any more what it actually checks.
I'll think about a different approach with better expressiveness.
For now, I consider it's time to get more tests into git
and to gather results on test.winehq.org.
> > -
> >    ok((hr == S_FALSE)^(pwfx2 == NULL), "hr %x<->suggest %p\n", hr, pwfx2);
> > -
> >    if (pwfx2 == (WAVEFORMATEX*)0xDEADF00D) /* bug in Wine */
>
> Would be nicer to both fix the bug in Wine and have a test showing the correct behavior.

That is the correct behavior AFAICT: S_FALSE <=> set pwfx2
I've not yet submitted all of my patches about IsFormatSupported
but you're right, that's an easy patch away.  I forgot that one.
For sure 5:1 needs more testing and I have no such HW.  You have.
> > -
> >    win_skip("mmdevapi not available: 0x%08x\n", hr);
>
> I think that's valid behavior, so we shouldn't fail.

Ouch.  I thought "Wine has dlls/mmdevapi, so it should always load",
but test.winehq.org shows that Solaris doesn't appear to have mmdevapi. Why?

Regards,
    Jörg Höhle

todo_wine, broken() and a SW anti-pattern

Hi,

The question about how to use todo_wine and broken() comes up from
time to time. Here's new light on it, as I'm suggesting in mmdevapi/tests/render.c:

hr = IAudioClient_GetService(ac, &IID_IAudioStreamVolume, (void**)&out);
ok(hr == AUDCLNT_E_NOT_INITIALIZED, "... call returns %08x\n", hr);
todo_wine ok(broken(out != NULL), "GetService %08x &out pointer %p\n", hr, out);

1. broken() documents that I consider native's observable behaviour
   broken because I judge it unsafe not to zero the out pointer in case
   of failure.  Furthermore, MSDN documents that it should be zeroed.
http://msdn.microsoft.com/en-us/library/windows/desktop/dd370873%28v=vs.85%2...

2. Not writing ok(broken(...) || <other condition>)
   allows to detect should MS ever change behaviour.  Currently,
   neither Vista, w2k8 nor w7 implement the documented behavior.

3. By using todo_wine, I acknowledge that Wine should continue to
   disagree with native's behavior and safely reset the out pointer.
   I mentioned "wine_dont" in
http://www.winehq.org/pipermail/wine-devel/2011-August/091764.html

That's not "100% bug compatible", but there's a small chance that
   it helps apps *not* crash in Wine when Wine's GetService returns
   E_NOTIMPL or some such that might never occur in native, hence the
   app is not prepared for it.

More generally, a function with both a return code and an out pointer
is often misused.  Typically, the specification says:
  "The out pointer is only set when the return code indicates success".
But actually, many program(mer)s use:
  "The pointer is non-NULL if and only if the return code indicates success."

Here's an example of broken code that follows:
 /* When FAILED(hr), it is *not* the case that out == NULL.
  * Hence the anti-pattern:
  */
     hr = GetService(..., &out);
     if (SUCCEEDED(hr)) {
         ...
         if (!...) goto exit;
         ...
     }
   exit:
     if (out) { Release(out); } /* bogus */

I once found such a case in Wine but I forgot where...
(not just in mmdevapi/tests/render.c which after all is "only" test code)
Michael, a challenge for your static analysis tools?

Bogus too: set out = NULL prior to calling the function, because
you don't know whether the function may write something initially,
only to bail out later with an error code such as OUTOFMEMORY.

Regards,
    Jörg Höhle

todo_wine, broken() and a SW anti-pattern

[Juan Lang]

Hi Joerg,
> hr = IAudioClient_GetService(ac, &IID_IAudioStreamVolume, (void**)&out);
> ok(hr == AUDCLNT_E_NOT_INITIALIZED, "... call returns %08x\n", hr);
> todo_wine ok(broken(out != NULL), "GetService %08x &out pointer %p\n", hr, out);
> - broken() documents that I consider native's observable behaviour
>
> broken because I judge it unsafe not to zero the out pointer in case
>   of failure.  Furthermore, MSDN documents that it should be zeroed.
> http://msdn.microsoft.com/en-us/library/windows/desktop/dd370873%28v=vs.85%2...

I think, philosophically, you're correct.  On the other hand, Wine
isn't meant to be a "philosophically correct" implementation of the
Win32 API.  And, the whole point of regression tests is that MSDN
isn't to be trusted.

So, while Microsoft says that the out pointer must be NULL on failure,
both in this case and more generally for COM (specifically for
QueryInterface), our guide has always been to trust what Microsoft
does more than what it says.

Our guideline for using broken() is to mark as broken the things that
Microsoft has changed.  Older implementations often had bugs that have
subsequently been fixed, so we mark those as broken and conform to the
newer behavior.

On the other hand, if Windows has always had a specific behavior, than
that behavior is, by definition, correct.  broken() isn't appropriate
in this case.  Again, we must do as Microsoft does, not as it says.

So I believe the correct behavior here is not to use broken(),
irrespective of what MSDN says.  A comment stating that MSDN is
incorrect might be in order.
--Juan