Re: [PATCH] winmm: Don't mask out SND_ALIAS_ID or SND_FILENAME in sndPlaySound
Andrew Eikum <aeikum(a)codeweavers.com> wrote:
+ br = sndPlaySoundA((LPCSTR)SND_ALIAS_SYSTEMASTERISK, SND_ALIAS_ID|SND_SYNC); + ok(br == TRUE || br == FALSE, "sndPlaySound gave strange return: %u\n", br);
This kind of test is broken. -- Dmitry.
On Wed, Dec 05, 2012 at 11:59:01AM +0800, Dmitry Timoshkov wrote:
Andrew Eikum <aeikum(a)codeweavers.com> wrote:
+ br = sndPlaySoundA((LPCSTR)SND_ALIAS_SYSTEMASTERISK, SND_ALIAS_ID|SND_SYNC); + ok(br == TRUE || br == FALSE, "sndPlaySound gave strange return: %u\n", br);
This kind of test is broken.
Care to elaborate? Being this terse helps no one. Andrew
Andrew Eikum <aeikum(a)codeweavers.com> wrote:
+ br = sndPlaySoundA((LPCSTR)SND_ALIAS_SYSTEMASTERISK, SND_ALIAS_ID|SND_SYNC); + ok(br == TRUE || br == FALSE, "sndPlaySound gave strange return: %u\n", br);
This kind of test is broken.
Care to elaborate? Being this terse helps no one.
What is this code testing the return value for? Why are there all these casts? -- Dmitry.
On Wed, Dec 05, 2012 at 11:06:48PM +0800, Dmitry Timoshkov wrote:
Andrew Eikum <aeikum(a)codeweavers.com> wrote:
+ br = sndPlaySoundA((LPCSTR)SND_ALIAS_SYSTEMASTERISK, SND_ALIAS_ID|SND_SYNC); + ok(br == TRUE || br == FALSE, "sndPlaySound gave strange return: %u\n", br);
This kind of test is broken.
Care to elaborate? Being this terse helps no one.
What is this code testing the return value for? Why are there all these casts?
The call is only supposed to return TRUE or FALSE, so I guess it's checking that the return value isn't 3 or something. Not very useful, sure, but it looked odd to just have a series of sndPlaySound calls in a row without ok() calls. The real test is to make sure sndPlaySound doesn't crash, as it does without my patch. The casts are to avoid compilation warnings. sndPlaySound takes a string as its first parameter, but it also accepts symbols like SND_ALIAS_SYSTEMASTERISK, which has the value 0x2A53. A cast is needed to avoid the warning about converting an integer to a pointer. Andrew
Andrew Eikum <aeikum(a)codeweavers.com> wrote:
+ br = sndPlaySoundA((LPCSTR)SND_ALIAS_SYSTEMASTERISK, SND_ALIAS_ID|SND_SYNC); + ok(br == TRUE || br == FALSE, "sndPlaySound gave strange return: %u\n", br);
This kind of test is broken.
Care to elaborate? Being this terse helps no one.
What is this code testing the return value for? Why are there all these casts?
The call is only supposed to return TRUE or FALSE, so I guess it's checking that the return value isn't 3 or something. Not very useful, sure, but it looked odd to just have a series of sndPlaySound calls in a row without ok() calls. The real test is to make sure sndPlaySound doesn't crash, as it does without my patch.
Then the test is completely useless, it actually tests nothing. -- Dmitry.
On Wed, Dec 05, 2012 at 11:21:39PM +0800, Dmitry Timoshkov wrote:
Andrew Eikum <aeikum(a)codeweavers.com> wrote:
The call is only supposed to return TRUE or FALSE, so I guess it's checking that the return value isn't 3 or something. Not very useful, sure, but it looked odd to just have a series of sndPlaySound calls in a row without ok() calls. The real test is to make sure sndPlaySound doesn't crash, as it does without my patch.
Then the test is completely useless, it actually tests nothing.
The ok calls are useless, I guess, but they're not doing any harm, either. You can send a patch to remove the ok calls if you like. Andrew
Andrew Eikum <aeikum(a)codeweavers.com> wrote:
The call is only supposed to return TRUE or FALSE, so I guess it's checking that the return value isn't 3 or something. Not very useful, sure, but it looked odd to just have a series of sndPlaySound calls in a row without ok() calls. The real test is to make sure sndPlaySound doesn't crash, as it does without my patch.
Then the test is completely useless, it actually tests nothing.
The ok calls are useless, I guess, but they're not doing any harm, either. You can send a patch to remove the ok calls if you like.
Without an ok() call a test is meaningless, I guess that Alexandre committed it by an accident. Since it's your patch I'll leave a revert to you. -- Dmitry.
On Thu, Dec 06, 2012 at 12:14:53AM +0800, Dmitry Timoshkov wrote:
Andrew Eikum <aeikum(a)codeweavers.com> wrote:
The call is only supposed to return TRUE or FALSE, so I guess it's checking that the return value isn't 3 or something. Not very useful, sure, but it looked odd to just have a series of sndPlaySound calls in a row without ok() calls. The real test is to make sure sndPlaySound doesn't crash, as it does without my patch.
Then the test is completely useless, it actually tests nothing.
The ok calls are useless, I guess, but they're not doing any harm, either. You can send a patch to remove the ok calls if you like.
Without an ok() call a test is meaningless, I guess that Alexandre committed it by an accident. Since it's your patch I'll leave a revert to you.
There is an implied test which is that the tests do not crash. Without the change to <dlls/winmm/playsound.c>, the tests you object to will crash. With the change, they do not crash. This is the behavior being tested. Andrew
Andrew Eikum <aeikum(a)codeweavers.com> wrote:
There is an implied test which is that the tests do not crash. Without the change to <dlls/winmm/playsound.c>, the tests you object to will crash. With the change, they do not crash. This is the behavior being tested.
I think that Alexandre said several times that the tests for crashes are useless. That's one of the reasons why there is no SEH support in the tests. -- Dmitry.
Dmitry Timoshkov <dmitry(a)baikal.ru> writes:
Andrew Eikum <aeikum(a)codeweavers.com> wrote:
There is an implied test which is that the tests do not crash. Without the change to <dlls/winmm/playsound.c>, the tests you object to will crash. With the change, they do not crash. This is the behavior being tested.
I think that Alexandre said several times that the tests for crashes are useless. That's one of the reasons why there is no SEH support in the tests.
That's for things that crash on Windows. If it doesn't crash on Windows, then it's legitimate to test that it also doesn't crash on Wine. -- Alexandre Julliard julliard(a)winehq.org
participants (3)
-
Alexandre Julliard -
Andrew Eikum -
Dmitry Timoshkov