[PATCH 1/3 resend] setupapi/tests: Change return values from coinst functions.

[Zebediah Figura]

On 7/24/20 2:42 PM, Jeff Smith wrote:
> On Fri, Jul 24, 2020 at 11:10 AM Zebediah Figura <z.figura12 at gmail.com> wrote:
>> On 7/24/20 11:08 AM, Jeff Smith wrote:
>>> On Fri, Jul 24, 2020 at 9:53 AM Zebediah Figura <z.figura12 at gmail.com> wrote:
>>>>
>>>> On 7/24/20 9:34 AM, Jeff Smith wrote:
>>>>> As co_error and class_error both return 0xdeadbeef, use of 0xdeadbeef as
>>>>> a canary value as well can lead to ambiguous tests results.
>>>>>
>>>>> As 0xdeadbeef is a typical canary value in wine, change the return
>>>>> values for co_error and class_error, avoiding the potential ambiguity.
>>>>
>>>> Is there a reason why we can't just use a different canary value in 2/3?
>>>
>>> Technically, that would work as well, and that is what I did in the original
>>> version of this patchset.  There was an objection raised though.
>>> While I'm not strongly attached to one way or the other, I could see the
>>> point of using deadbeef for the canary, and returning a different value
>>> from co_error/class_error, as done in this patchset.
>>
>> I think that objection was rather specious.
> 
> Maybe.
> 
>> If nothing else, I don't
>> think E_FAIL is a very good canary value.
> 
> Going to go out on a limb and say that the value returned by
> co_error/class_error
> (which is different from the canary) isn't that important as long as
> it is unlikely to
> be ambiguous as to where it came from.  So I just chose about the most generic
> COM error that I could come up with.
> 
> As much as I want the tests to be the best they can be, TBH it's
> really the fix (patch 3)
> that I care about, so I'll quit splitting hairs.  You think forgetting
> patch 1 and using a
> different canary for patch 2 is the way to go?

I certainly don't want to give the impression that it matters very much.

I don't expect setupapi to throw E_FAIL, but it's less obvious than,
say, 0xdead**** that it's a sentinel.

> 
>>>>> Signed-off-by: Jeff Smith <whydoubt at gmail.com>
>>>>> ---
>>>>> Patches 2 and 3 will work without patch 1.  This patch just makes tests
>>>>> marginally more precise if an error does get caught.
>>>>>
>>>>>  dlls/setupapi/tests/coinst.c  |  4 ++--
>>>>>  dlls/setupapi/tests/devinst.c | 10 +++++-----
>>>>>  2 files changed, 7 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/dlls/setupapi/tests/coinst.c b/dlls/setupapi/tests/coinst.c
>>>>> index 5e8d890cfa5..604bf74d1eb 100644
>>>>> --- a/dlls/setupapi/tests/coinst.c
>>>>> +++ b/dlls/setupapi/tests/coinst.c
>>>>> @@ -47,7 +47,7 @@ DWORD WINAPI class_default(DI_FUNCTION function, HDEVINFO set, SP_DEVINFO_DATA *
>>>>>
>>>>>  DWORD WINAPI class_error(DI_FUNCTION function, HDEVINFO set, SP_DEVINFO_DATA *device)
>>>>>  {
>>>>> -    return 0xdeadbeef;
>>>>> +    return E_FAIL;
>>>>>  }
>>>>>
>>>>>  DWORD WINAPI co_success(DI_FUNCTION function, HDEVINFO set, SP_DEVINFO_DATA *device,
>>>>> @@ -67,5 +67,5 @@ DWORD WINAPI CoDeviceInstall(DI_FUNCTION function, HDEVINFO set, SP_DEVINFO_DATA
>>>>>  DWORD WINAPI co_error(DI_FUNCTION function, HDEVINFO set, SP_DEVINFO_DATA *device,
>>>>>          COINSTALLER_CONTEXT_DATA *context)
>>>>>  {
>>>>> -    return 0xdeadbeef;
>>>>> +    return E_FAIL;
>>>>>  }
>>>>> diff --git a/dlls/setupapi/tests/devinst.c b/dlls/setupapi/tests/devinst.c
>>>>> index d4c82dea1f6..80abc44029c 100644
>>>>> --- a/dlls/setupapi/tests/devinst.c
>>>>> +++ b/dlls/setupapi/tests/devinst.c
>>>>> @@ -2727,17 +2727,17 @@ static void test_class_installer(void)
>>>>>
>>>>>      ret = SetupDiCallClassInstaller(DIF_ALLOW_INSTALL, set, &device);
>>>>>      ok(!ret, "Expected failure.\n");
>>>>> -    ok(GetLastError() == 0xdeadbeef, "Got unexpected error %#x.\n", GetLastError());
>>>>> +    ok(GetLastError() == E_FAIL, "Got unexpected error %#x.\n", GetLastError());
>>>>>
>>>>>      ok(!device_is_registered(set, &device), "Expected device not to be registered.\n");
>>>>>      ret = SetupDiCallClassInstaller(DIF_REGISTERDEVICE, set, &device);
>>>>>      ok(!ret, "Expected failure.\n");
>>>>> -    ok(GetLastError() == 0xdeadbeef, "Got unexpected error %#x.\n", GetLastError());
>>>>> +    ok(GetLastError() == E_FAIL, "Got unexpected error %#x.\n", GetLastError());
>>>>>      ok(!device_is_registered(set, &device), "Expected device not to be registered.\n");
>>>>>
>>>>>      ret = SetupDiCallClassInstaller(DIF_REMOVE, set, &device);
>>>>>      ok(!ret, "Expected failure.\n");
>>>>> -    ok(GetLastError() == 0xdeadbeef, "Got unexpected error %#x.\n", GetLastError());
>>>>> +    ok(GetLastError() == E_FAIL, "Got unexpected error %#x.\n", GetLastError());
>>>>>      ok(!device_is_registered(set, &device), "Expected device not to be registered.\n");
>>>>>
>>>>>      SetupDiDestroyDeviceInfoList(set);
>>>>> @@ -2875,12 +2875,12 @@ static void test_class_coinstaller(void)
>>>>>
>>>>>      ret = SetupDiCallClassInstaller(DIF_ALLOW_INSTALL, set, &device);
>>>>>      ok(!ret, "Expected failure.\n");
>>>>> -    ok(GetLastError() == 0xdeadbeef, "Got unexpected error %#x.\n", GetLastError());
>>>>> +    ok(GetLastError() == E_FAIL, "Got unexpected error %#x.\n", GetLastError());
>>>>>
>>>>>      ok(!device_is_registered(set, &device), "Expected device not to be registered.\n");
>>>>>      ret = SetupDiCallClassInstaller(DIF_REGISTERDEVICE, set, &device);
>>>>>      ok(!ret, "Expected failure.\n");
>>>>> -    ok(GetLastError() == 0xdeadbeef, "Got unexpected error %#x.\n", GetLastError());
>>>>> +    ok(GetLastError() == E_FAIL, "Got unexpected error %#x.\n", GetLastError());
>>>>>      ok(!device_is_registered(set, &device), "Expected device not to be registered.\n");
>>>>>
>>>>>      SetupDiDestroyDeviceInfoList(set);
>>>>>
>>>>
>>>>
>>>
>>
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20200724/08058ab0/attachment.sig>