Hello,
some comments about this patch + if (hr != S_OK) return FALSE; + else return TRUE;
could be simplified as return SUCCEEDED(hr);
+ trace("HRESULT when creating instance (%x)\n", hr); Since tests are assumed to pass, we dont care about debug informations. So, traces are useless in tests. [It is my opinion. Better developpers could give an opposite opinion]
+ todo_wine ok(*(ULONG *)pDirectMusicPort != 0, "IDirectMusicPort not set\n");
Why is this cast useful?
A+ David
2010/8/9 Austin Lund austin.lund@gmail.com
On Wednesday, August 11, 2010 3:12:23 am David Adam wrote:
Hello,
some comments about this patch
- if (hr != S_OK) return FALSE;
- else return TRUE;
could be simplified as return SUCCEEDED(hr);
SUCCEEDED encompasses more than just S_OK (S_FALSE, for instance; basically anything that doesn't have the high bit set). Whether to test SUCCEEDED or S_OK explicitly depends on what the function can return and what it means.
The original behavior could instead be simplified as: return hr==S_OK;
On 11 August 2010 20:12, David Adam david.adam.cnrs@gmail.com wrote:
Hello,
thanks for the comments. I've resent.
+ todo_wine ok(*(ULONG *)pDirectMusicPort != 0, "IDirectMusicPort not set\n");
Why is this cast useful?
I wasn't sure of the best way to do this. But basically I want to check that the Vtbl had been set. I cannot cast to IDirectMusicPortImpl and I'm still confused by the COM macros.
On 08/11/2010 09:14 PM, Austin Lund wrote:
On 11 August 2010 20:12, David Adamdavid.adam.cnrs@gmail.com wrote: I wasn't sure of the best way to do this. But basically I want to check that the Vtbl had been set. I cannot cast to IDirectMusicPortImpl and I'm still confused by the COM macros.
You shouldn't be checking for any vtbl, it's all object's internal stuff. If you must check a pointer if it's null or not, then do that explicitly. Pointers have different size in 32-bit and 64-bit architectures.
Also you don't need two things pointing to one another. IDirectMusicPort is a pointer in a first place. So no need for memset either. And no need for all the extra traces that print the same stuff. You already checking if it's null or not. The value of the pointer won't be of any use.
Vitaliy.
On 12 August 2010 14:23, Vitaliy Margolen wine-devel@kievinfo.com wrote:
On 08/11/2010 09:14 PM, Austin Lund wrote:
On 11 August 2010 20:12, David Adamdavid.adam.cnrs@gmail.com wrote: I wasn't sure of the best way to do this. But basically I want to check that the Vtbl had been set. I cannot cast to IDirectMusicPortImpl and I'm still confused by the COM macros.
You shouldn't be checking for any vtbl, it's all object's internal stuff. If you must check a pointer if it's null or not, then do that explicitly. Pointers have different size in 32-bit and 64-bit architectures.
Also you don't need two things pointing to one another. IDirectMusicPort is a pointer in a first place. So no need for memset either. And no need for all the extra traces that print the same stuff. You already checking if it's null or not. The value of the pointer won't be of any use.
The setting (or not) of the Vtbl is exactly why the crash in bug 22598 occurs. So if I'm not allowed to test for this, then this bug shouldn't have a test written for it?
The test I wrote for this bug just ensures that the object has something put into it to avoid the segfault. The sanest way I could think of doing this is to zero out the memory (hence memset) and check that the function actually put something at the start of the object.
I got rid of the traces in the second go. They were left over from when I was using crosstest to see what was going on.
On 08/11/2010 11:11 PM, Austin Lund wrote:
On 12 August 2010 14:23, Vitaliy Margolenwine-devel@kievinfo.com wrote:
On 08/11/2010 09:14 PM, Austin Lund wrote:
On 11 August 2010 20:12, David Adamdavid.adam.cnrs@gmail.com wrote: I wasn't sure of the best way to do this. But basically I want to check that the Vtbl had been set. I cannot cast to IDirectMusicPortImpl and I'm still confused by the COM macros.
You shouldn't be checking for any vtbl, it's all object's internal stuff. If you must check a pointer if it's null or not, then do that explicitly. Pointers have different size in 32-bit and 64-bit architectures.
Also you don't need two things pointing to one another. IDirectMusicPort is a pointer in a first place. So no need for memset either. And no need for all the extra traces that print the same stuff. You already checking if it's null or not. The value of the pointer won't be of any use.
The setting (or not) of the Vtbl is exactly why the crash in bug 22598 occurs. So if I'm not allowed to test for this, then this bug shouldn't have a test written for it?
The IDirectMusicPort is a pointer. To zero it you do "ptr = NULL". Don't mention any vtbs in test. Think of it an (void*) pointer - what it points to shouldn't matter.
The test I wrote for this bug just ensures that the object has something put into it to avoid the segfault.
You can't test that. All you can test is that you got the IDirectMusicPort back or not. What exactly inside is irrelevant.
And if IDirectMusicPerformance8_PChannelInfo succeeds and you getting not null pDirectMusicPort back but it points to bogus data or is unchanged (as it is now), it's outright invalid behavior that should be fixed. No tests needed for that.
Vitaliy.
On 12 August 2010 15:48, Vitaliy Margolen wine-devel@kievinfo.com wrote:
You can't test that. All you can test is that you got the IDirectMusicPort back or not. What exactly inside is irrelevant.
And if IDirectMusicPerformance8_PChannelInfo succeeds and you getting not null pDirectMusicPort back but it points to bogus data or is unchanged (as it is now), it's outright invalid behavior that should be fixed. No tests needed for that.
OK. How about this:
static void test_PChannelInfo(void) { IDirectMusicPort *pDirectMusicPort; HRESULT hr;
pDirectMusicPort = NULL; hr = IDirectMusicPerformance8_PChannelInfo(idmusicperformance, 0, &pDirectMusicPort, NULL, NULL); ok(hr == S_OK, "Failed to call PChannelInfo (%x)\n", hr); todo_wine ok(pDirectMusicPort != NULL, "IDirectMusicPort not set\n"); }
I'll take on other comments and resubmit tomorrow as suggested.
Thanks.
Some compilers complain about the lack of a return in a void function. Don't forget to add it in each void function.
A+
David
________________________________ De : Austin Lund austin.lund@gmail.com À : Vitaliy Margolen wine-devel@kievinfo.com Cc : wine-devel@winehq.org Envoyé le : Jeu 12 août 2010, 8h 09min 44s Objet : Re: [PATCH 1/2] dmime/tests: Added tests for IDirectMusicPerformance.
On 12 August 2010 15:48, Vitaliy Margolen wine-devel@kievinfo.com wrote:
You can't test that. All you can test is that you got the IDirectMusicPort back or not. What exactly inside is irrelevant.
And if IDirectMusicPerformance8_PChannelInfo succeeds and you getting not null pDirectMusicPort back but it points to bogus data or is unchanged (as it is now), it's outright invalid behavior that should be fixed. No tests needed for that.
OK. How about this:
static void test_PChannelInfo(void) { IDirectMusicPort *pDirectMusicPort; HRESULT hr;
pDirectMusicPort = NULL; hr = IDirectMusicPerformance8_PChannelInfo(idmusicperformance, 0, &pDirectMusicPort, NULL, NULL); ok(hr == S_OK, "Failed to call PChannelInfo (%x)\n", hr); todo_wine ok(pDirectMusicPort != NULL, "IDirectMusicPort not set\n"); }
I'll take on other comments and resubmit tomorrow as suggested.
Thanks.
paulo lesgaz jeremielapuree@yahoo.fr wrote:
Some compilers complain about the lack of a return in a void function. Don't forget to add it in each void function.
That compilers are buggy, there is no need to add statements without a real reason. At least it helps to list those compilers, and send a bug report if you think that's appropriate.
On 08/12/2010 12:09 AM, Austin Lund wrote:
On 12 August 2010 15:48, Vitaliy Margolenwine-devel@kievinfo.com wrote:
You can't test that. All you can test is that you got the IDirectMusicPort back or not. What exactly inside is irrelevant.
And if IDirectMusicPerformance8_PChannelInfo succeeds and you getting not null pDirectMusicPort back but it points to bogus data or is unchanged (as it is now), it's outright invalid behavior that should be fixed. No tests needed for that.
OK. How about this:
static void test_PChannelInfo(void) { IDirectMusicPort *pDirectMusicPort; HRESULT hr;
pDirectMusicPort = NULL; hr = IDirectMusicPerformance8_PChannelInfo(idmusicperformance, 0,&pDirectMusicPort, NULL, NULL); ok(hr == S_OK, "Failed to call PChannelInfo (%x)\n", hr); todo_wine ok(pDirectMusicPort != NULL, "IDirectMusicPort not set\n"); }
This looks much better.
I'll take on other comments and resubmit tomorrow as suggested.
+static void test_GetDefaultAudioPath(void) +{
- hr = IDirectMusicPerformance8_GetDefaultAudioPath(idmusicperformance,&pDirectMusicAudioPath);
You leaking pDirectMusicAudioPath here. You should free it after test.
Same for idmusicperformance. And CoInitialize() needs a matching CoUninitialize().
Vitaliy.
-
Austin Lund -
Chris Robinson -
David Adam -
Dmitry Timoshkov -
paulo lesgaz -
Vitaliy Margolen