[dplayx 02/29] Tests for DirectPlayCreate

[Michael Karcher]

Am Mittwoch, den 09.07.2008, 20:15 +0300 schrieb Ismael Barros:
> Thanks for the review :)
No problem.


> On 7/9/08, Michael Karcher <wine at mkarcher.dialup.fu-berlin.de> wrote:
> > I dislike the order of the parameters and the name. You would write
> Because _okHR compares HRESULT codes, but there's also _ok to compare
> integers, or _okFlags to compare flags (the main difference being how
> to print info about the expected and actual values), or _okStr to
> compare strings.
OK, taking back the criticism on parameter order. Passing the actual
value last is common practice in the user32 test, so your variant has a
precedence case in wine. Yet I couldn't find these kind of macros
starting with an underscore. More on that point below.

> > And last but not
> > least: As I see it, a static inline would do the job too, and is always
> > preferrable to macros.
> The main reason of using ditry macros instead of functions was to
> preserve the __LINE__ number, otherwise the debug information provided
> is pretty much useless.
Yes, you are right of course. I didn't think of __LINE__ (also James
noted the same thing). Now you get to the point where a precedence case
for identifiers with an underscore are used: shell32/tests/shlexec.c.
They have inline functions, called _okFooBar which take file and line as
parameter, and have a macro okFooBar which calls the _okFooBar method,
so the test case writer does not get to see the identifier starting with
an underscore. Thats how I feel about them: They are internal
identifiers, telling you "if you use this directly, you better know very
well what you are doing; most probably there is a safer or easier way to
accomplish the goal". In this case, the _okFooBar function is meant just
for the easier okFooBar macro.

> >> +    heap = HeapCreate( 0, 0, 0 );
> > Do you have any reason to create your own heap? Each windows process
> > already comes equipped with a standard heap you can obtain using the API
> > function GetProcessHeap, so this line:
> >> +    IUnknown* pUnk = HeapAlloc( heap, 0, sizeof(LPDWORD) );
> > would then look like:
> > +    IUnknown* pUnk = HeapAlloc( GetProcessHeap(), 0, sizeof(LPDWORD) );
> 
> That's what I used in the begining, but I've got some HeapAllocs that
> I don't free.
OK. That's a point. I didn't expect it, though. I would like a comment
in there along
  /* Create an extra heap to throw away garbage after tests */

> This is intended, as freeing that memory would make the
> code unnecessarily complex, I can't use static local variables (I use
> the result of the function inside the same ok() or trace()), and
> loosing memory is always undesirable but not critical in a test case.
You can use static variables, if you go along the lines of of
get_temp_buffer in libs/wine/debug.c, where a static local array is
cyclical indexed. This might not be a good idea, though, depending on
how long the result has to stay alive.

> > Another point is that this line is totally wrong anyway. An IUnknown* is
> > a pointer to an COM object, which starts with a vtable pointer. You let
> > pUnk point to uninitialized memory. While it doesn't matter in this case
> > (as DirectPlayCreate will never access what the pointer points to), it
> > is really bad style. I understand the intention to test that aggregation
> > is not supported for DirectPlay, but you still should keep the contract
> > of the creation function that pUnk either is NULL or points to a valid
> > COM object. A basic COM object is just a pointer to a vtable that
> > contains a QueryInterface method that returns "this" for IUnknown and
> > fails in every other case, an AddRef method that returns 2 and does
> > nothing, and a Release method that returns 1 and does nothing else.
> I just wrote is to check the return value in case of pUnk != NULL, but
> there would be no problem in either removing that test (it's probably
> unneeded) or doing it the right way.
A test case is always good, if it does not take too much execution time.
These tests with NULL pointers seemed strange to me when I first
encountered, as sometimes NULL pointers are passed into functions where
they are not useful or even forbidden by the documented contract. But as
wine is all about fulfilling the same contract as the windows
implementation which might deviate from MSDN, the test case serves as
documentation of Windows behavior, which is good.

But in my opinion, testing of the reaction to contract violation should
not go into passing somehow valid-looking pointers, that do not point to
anything sensible. And the main point was, that I really don't get why
it must be a heap pointer. The an LPDWORD on the stack would do as well
as allocating sizeof(LPDWORD) area.

Regards,
  Michael Karcher