Re: [PATCH] winmm: Don't mask out SND_ALIAS_ID or SND_FILENAME in sndPlaySound

[Dmitry Timoshkov]

Andrew Eikum  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.

[PATCH] winmm: Don't mask out SND_ALIAS_ID or SND_FILENAME in sndPlaySound

[Andrew Eikum]

On Wed, Dec 05, 2012 at 11:59:01AM +0800, Dmitry Timoshkov wrote:
> Andrew Eikum  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

[PATCH] winmm: Don't mask out SND_ALIAS_ID or SND_FILENAME in sndPlaySound

[Dmitry Timoshkov]

Andrew Eikum  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?

[PATCH] winmm: Don't mask out SND_ALIAS_ID or SND_FILENAME in sndPlaySound

[Andrew Eikum]

On Wed, Dec 05, 2012 at 11:06:48PM +0800, Dmitry Timoshkov wrote:
> Andrew Eikum  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

[PATCH] winmm: Don't mask out SND_ALIAS_ID or SND_FILENAME in sndPlaySound

[Dmitry Timoshkov]

Andrew Eikum  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.

[PATCH] winmm: Don't mask out SND_ALIAS_ID or SND_FILENAME in sndPlaySound

[Andrew Eikum]

On Wed, Dec 05, 2012 at 11:21:39PM +0800, Dmitry Timoshkov wrote:
> Andrew Eikum  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

[PATCH] winmm: Don't mask out SND_ALIAS_ID or SND_FILENAME in sndPlaySound

[Dmitry Timoshkov]

Andrew Eikum  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.

[PATCH] winmm: Don't mask out SND_ALIAS_ID or SND_FILENAME in sndPlaySound

[Andrew Eikum]

On Thu, Dec 06, 2012 at 12:14:53AM +0800, Dmitry Timoshkov wrote:
> Andrew Eikum  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

[PATCH] winmm: Don't mask out SND_ALIAS_ID or SND_FILENAME in sndPlaySound

[Dmitry Timoshkov]

Andrew Eikum  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.

[PATCH] winmm: Don't mask out SND_ALIAS_ID or SND_FILENAME in sndPlaySound

[Alexandre Julliard]

Dmitry Timoshkov  writes:
> Andrew Eikum  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.